[PATCH] usb: mtu3: Fix possible use-before-initialization bug
Yu Hao
yhao016 at ucr.edu
Sun Jul 9 17:48:15 PDT 2023
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 at bootlin.com> wrote:
>
> Hi Yu,
>
> On Tue, 4 Jul 2023 16:25:50 -0700
> Yu Hao <yhao016 at 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 at 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 = {};
> > 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
More information about the linux-arm-kernel
mailing list