Re: [PATCH v8] misc: sgi-gru: fix use-after-free error in gru_set_context_option, gru_fault and gru_handle_user_call_os

[Zheng Hacker]

Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 于2022年11月9日周三 19:12写道：
>
> On Wed, Nov 09, 2022 at 06:51:58PM +0800, Zheng Wang wrote:
> > Gts may be freed in gru_check_chiplet_assignment.
> > The caller still use it after that, UAF happens.
>
> I do not understand what this text means, sorry.  Can you try to make it
> more descriptive?
>

Sorry for my unclear description. The new candidate is as follows:
In some bad situation, the gts may be freed gru_check_chiplet_assignment.
The call chain can be gru_unload_context->gru_free_gru_context->gts_drop
and kfree finally. However, the caller didn't know if the gts is freed
or not and
use it afterwards. This will trigger a Use after Free bug.

> >
> > Fix it by introducing a return value to see if it's in error path or not.
> > Free the gts in caller if gru_check_chiplet_assignment check failed.
>
> Please wrap all of your changelog text at 72 columns, you have 2
> paragraphs with different wrappings.
>

Get it, sorry for that.

> >       /*
> >        * If the current task is the context owner, verify that the
> > @@ -727,14 +728,16 @@ void gru_check_context_placement(struct gru_thread_state *gts)
> >        */
> >       gru = gts->ts_gru;
> >       if (!gru || gts->ts_tgid_owner != current->tgid)
> > -             return;
> > +             return ret;
>
> Why does this check return "all is good!" ?
>
> Shouldn't that be an error?
>
This check is something like "if the gts has been initiiazed properly".
If it's not, I thinks we shouldn't treat the gts like something very
bad happend. Because in the later request, the gts can still have a
chance to be configed/updated properly. This is different from "it's
too bad so we have to unload gts right now". This is just my personal
point of view. Besides, the caller of this function have token it into
consider. In gru_fault, it will try again and in gru_handle_user_call_os,
it will return -EAGAIN. In gru_set_context_option, it will be fine
because there won't be any operation on gts->ts_gru or gts->ts_tgid_owner

Best regards,

Zheng Wang