Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221109105158.230081-1-zyytlz.wz@163.com> <Y2uLK6Zxsz9TD9WV@kroah.com>
In-Reply-To: <Y2uLK6Zxsz9TD9WV@kroah.com>
From:   Zheng Hacker <hackerzheng666@gmail.com>
Date:   Wed, 9 Nov 2022 20:04:04 +0800
Message-ID: <CAJedcCyN+D+gsVo10r-bGpAtnL4x_N3TpqhVYBi1P7=dD8fNCw@mail.gmail.com>
Subject: Re: [PATCH v8] misc: sgi-gru: fix use-after-free error in
 gru_set_context_option, gru_fault and gru_handle_user_call_os
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Zheng Wang <zyytlz.wz@163.com>, zhengyejian1@huawei.com,
        dimitri.sivanich@hpe.com, arnd@arndb.de,
        linux-kernel@vger.kernel.org, alex000young@gmail.com,
        security@kernel.org, sivanich@hpe.com, lkp@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Greg KH <gregkh@linuxfoundation.org> =E4=BA=8E2022=E5=B9=B411=E6=9C=889=E6=
=97=A5=E5=91=A8=E4=B8=89 19:12=E5=86=99=E9=81=93=EF=BC=9A
>
> 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 no=
t.
> > 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_threa=
d_state *gts)
> >        */
> >       gru =3D gts->ts_gru;
> >       if (!gru || gts->ts_tgid_owner !=3D 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