Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug

[Yu Hao <yhao016@xxxxxxx>]

Hi Hervé,

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 = mtu3_readl(mtu->mac_base, U3D_EP0CSR) & EP0_W1C_BITS;
    count = mtu3_readl(mtu->mac_base, U3D_RXCOUNT0);
+   if (count == 0)
+       return -EINVAL;

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

@@ -642,7 +644,8 @@ __acquires(mtu->lock)
    struct mtu3_request *mreq;
    int handled = 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) == 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 = IRQ_HANDLED;
        break;
    default:
-- 
2.34.1

Yu Hao

On Tue, Jul 4, 2023 at 11:06 PM Herve Codina <herve.codina@xxxxxxxxxxx> wrote:
>
> Hi Yu,
>
> On Tue, 4 Jul 2023 16:25:50 -0700
> Yu Hao <yhao016@xxxxxxx> 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@xxxxxxx>
> > ---
> >  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 = {};
> >     struct mtu3_request *mreq;
> >     int handled = 0;
> >
>
> Looks strange to me because, if ep0_read_setup() cannot read the setup data
> why don't we simply abort the operation ?
>
> With setup = {}, the following test is true:
>   if ((setup.bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD)
>         handled = 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 = 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 sequence
> was never received.
> Aborting the operation if ep0_read_setup() cannot read the setup data seems
> better to me.
>
> Best regards,
> Hervé
>
> --
> Hervé Codina, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com