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: <CA+UBctDxfb6+70+hzuXJ-gwb65E0uoNzXYEhpJT92sXr2CE7OA@mail.gmail.com>
 <20230705080625.02b2bac5@bootlin.com>
In-Reply-To: <20230705080625.02b2bac5@bootlin.com>
From:   Yu Hao <yhao016@ucr.edu>
Date:   Sun, 9 Jul 2023 17:48:15 -0700
Message-ID: <CA+UBctBqtSvyBWf9ZwKbecTrh9_6sCDm_TyU-ncb+6h5y19K5g@mail.gmail.com>
Subject: Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     chunfeng.yun@mediatek.com, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Herv=C3=A9,

Thanks for the comments. How about this patch?
---
 drivers/usb/mtu3/mtu3_gadget_ep0.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c
b/drivers/usb/mtu3/mtu3_gadget_ep0.c
index e4fd1bb14a55..af2884943c2a 100644
--- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
+++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
@@ -600,7 +600,7 @@ static void ep0_tx_state(struct mtu3 *mtu)
        mtu3_readl(mtu->mac_base, U3D_EP0CSR));
 }

-static void ep0_read_setup(struct mtu3 *mtu, struct usb_ctrlrequest *setup=
)
+static int ep0_read_setup(struct mtu3 *mtu, struct usb_ctrlrequest *setup)
 {
    struct mtu3_request *mreq;
    u32 count;
@@ -608,6 +608,8 @@ static void ep0_read_setup(struct mtu3 *mtu,
struct usb_ctrlrequest *setup)

    csr =3D mtu3_readl(mtu->mac_base, U3D_EP0CSR) & EP0_W1C_BITS;
    count =3D mtu3_readl(mtu->mac_base, U3D_RXCOUNT0);
+   if (count =3D=3D 0)
+       return -EINVAL;

    ep0_read_fifo(mtu->ep0, (u8 *)setup, count);

@@ -642,7 +644,8 @@ __acquires(mtu->lock)
    struct mtu3_request *mreq;
    int handled =3D 0;

-   ep0_read_setup(mtu, &setup);
+   if (ep0_read_setup(mtu, &setup))
+       return -EINVAL;
    trace_mtu3_handle_setup(&setup);

    if ((setup.bRequestType & USB_TYPE_MASK) =3D=3D USB_TYPE_STANDARD)
@@ -764,7 +767,9 @@ irqreturn_t mtu3_ep0_isr(struct mtu3 *mtu)
            break;
        }

-       ep0_handle_setup(mtu);
+       if (ep0_handle_setup(mtu))
+           break;
+
        ret =3D IRQ_HANDLED;
        break;
    default:
--=20
2.34.1

Yu Hao

On Tue, Jul 4, 2023 at 11:06=E2=80=AFPM Herve Codina <herve.codina@bootlin.=
com> wrote:
>
> Hi Yu,
>
> On Tue, 4 Jul 2023 16:25:50 -0700
> Yu Hao <yhao016@ucr.edu> wrote:
>
> > The struct usb_ctrlrequest setup should be initialized in the function
> > ep0_read_setup(mtu, &setup). However, inside that function,
> > the variable count could be 0 and the struct usb_ctrlrequest setup
> > is not initialized. But there is a read for setup.bRequestType.
> >
> > Signed-off-by: Yu Hao <yhao016@ucr.edu>
> > ---
> >  drivers/usb/mtu3/mtu3_gadget_ep0.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > index e4fd1bb14a55..67034fa515d0 100644
> > --- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > +++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > @@ -638,7 +638,7 @@ static int ep0_handle_setup(struct mtu3 *mtu)
> >  __releases(mtu->lock)
> >  __acquires(mtu->lock)
> >  {
> > -   struct usb_ctrlrequest setup;
> > +   struct usb_ctrlrequest setup =3D {};
> >     struct mtu3_request *mreq;
> >     int handled =3D 0;
> >
>
> Looks strange to me because, if ep0_read_setup() cannot read the setup da=
ta
> why don't we simply abort the operation ?
>
> With setup =3D {}, the following test is true:
>   if ((setup.bRequestType & USB_TYPE_MASK) =3D=3D USB_TYPE_STANDARD)
>         handled =3D handle_standard_request(mtu, &setup);
>
> handle_standard_request() is called and supposes an USB_REQ_GET_STATUS
> (0x00) request:
>    case USB_REQ_GET_STATUS:
>         handled =3D ep0_get_status(mtu, setup);
>         break;
>
> Then ep0_get_status() supposes USB_RECIP_DEVICE (0x00) and performs some
> operation sending the data related to the GET_STATUS.
>
> All of these are not correct as the setup data that triggered this sequen=
ce
> was never received.
> Aborting the operation if ep0_read_setup() cannot read the setup data see=
ms
> better to me.
>
> Best regards,
> Herv=C3=A9
>
> --
> Herv=C3=A9 Codina, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com