* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
@ 2023-07-10 0:48 ` Yu Hao
0 siblings, 0 replies; 16+ messages in thread
From: Yu Hao @ 2023-07-10 0:48 UTC (permalink / raw)
To: Herve Codina
Cc: chunfeng.yun, gregkh, linux-usb, linux-arm-kernel, linux-mediatek,
linux-kernel
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@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 = {};
> > 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
2023-07-10 0:48 ` Yu Hao
@ 2023-07-10 6:25 ` Herve Codina
-1 siblings, 0 replies; 16+ messages in thread
From: Herve Codina @ 2023-07-10 6:25 UTC (permalink / raw)
To: Yu Hao
Cc: chunfeng.yun, gregkh, linux-usb, linux-arm-kernel, linux-mediatek,
linux-kernel
Hi Yu,
On Sun, 9 Jul 2023 17:48:15 -0700
Yu Hao <yhao016@ucr.edu> wrote:
> 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;
'count' should be tested against sizeof(*setup). Indeed, we need to have a
setup data packet in the fifo.
What do you think about:
if (count < sizef(*setup))
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;
Forward the error code to the caller ?
ret = ep0_read_setup(mtu, &setup)
if (ret < 0)
return ret;
> 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;
> +
Ok
> ret = IRQ_HANDLED;
> break;
> default:
Be careful, your patch is wrongly indented.
tabs replaced by 4 spaces. You need to keep tabs.
Regards,
Hervé Codina
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
@ 2023-07-10 6:25 ` Herve Codina
0 siblings, 0 replies; 16+ messages in thread
From: Herve Codina @ 2023-07-10 6:25 UTC (permalink / raw)
To: Yu Hao
Cc: chunfeng.yun, gregkh, linux-usb, linux-arm-kernel, linux-mediatek,
linux-kernel
Hi Yu,
On Sun, 9 Jul 2023 17:48:15 -0700
Yu Hao <yhao016@ucr.edu> wrote:
> 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;
'count' should be tested against sizeof(*setup). Indeed, we need to have a
setup data packet in the fifo.
What do you think about:
if (count < sizef(*setup))
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;
Forward the error code to the caller ?
ret = ep0_read_setup(mtu, &setup)
if (ret < 0)
return ret;
> 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;
> +
Ok
> ret = IRQ_HANDLED;
> break;
> default:
Be careful, your patch is wrongly indented.
tabs replaced by 4 spaces. You need to keep tabs.
Regards,
Hervé Codina
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
2023-07-10 6:25 ` Herve Codina
@ 2023-07-11 8:48 ` Chunfeng Yun (云春峰)
-1 siblings, 0 replies; 16+ messages in thread
From: Chunfeng Yun (云春峰) @ 2023-07-11 8:48 UTC (permalink / raw)
To: yhao016@ucr.edu, herve.codina@bootlin.com
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
On Mon, 2023-07-10 at 08:25 +0200, Herve Codina wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hi Yu,
>
> On Sun, 9 Jul 2023 17:48:15 -0700
> Yu Hao <yhao016@ucr.edu> wrote:
>
> > 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;
>
> 'count' should be tested against sizeof(*setup). Indeed, we need to
> have a
> setup data packet in the fifo.
>
> What do you think about:
> if (count < sizef(*setup))
> return -EINVAL;
before call this function, already check the data length in fifo, it
should be 8 bytes.
see mtu3_ep0_isr(), about line 761.
I think no need this patch
Thanks a lot
>
> >
> > 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;
>
> Forward the error code to the caller ?
>
> ret = ep0_read_setup(mtu, &setup)
> if (ret < 0)
> return ret;
>
>
> > 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;
> > +
>
> Ok
>
> > ret = IRQ_HANDLED;
> > break;
> > default:
>
> Be careful, your patch is wrongly indented.
> tabs replaced by 4 spaces. You need to keep tabs.
>
> Regards,
> Hervé Codina
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
@ 2023-07-11 8:48 ` Chunfeng Yun (云春峰)
0 siblings, 0 replies; 16+ messages in thread
From: Chunfeng Yun (云春峰) @ 2023-07-11 8:48 UTC (permalink / raw)
To: yhao016@ucr.edu, herve.codina@bootlin.com
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
On Mon, 2023-07-10 at 08:25 +0200, Herve Codina wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hi Yu,
>
> On Sun, 9 Jul 2023 17:48:15 -0700
> Yu Hao <yhao016@ucr.edu> wrote:
>
> > 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;
>
> 'count' should be tested against sizeof(*setup). Indeed, we need to
> have a
> setup data packet in the fifo.
>
> What do you think about:
> if (count < sizef(*setup))
> return -EINVAL;
before call this function, already check the data length in fifo, it
should be 8 bytes.
see mtu3_ep0_isr(), about line 761.
I think no need this patch
Thanks a lot
>
> >
> > 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;
>
> Forward the error code to the caller ?
>
> ret = ep0_read_setup(mtu, &setup)
> if (ret < 0)
> return ret;
>
>
> > 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;
> > +
>
> Ok
>
> > ret = IRQ_HANDLED;
> > break;
> > default:
>
> Be careful, your patch is wrongly indented.
> tabs replaced by 4 spaces. You need to keep tabs.
>
> Regards,
> Hervé Codina
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
2023-07-11 8:48 ` Chunfeng Yun (云春峰)
@ 2023-07-11 9:41 ` Herve Codina
-1 siblings, 0 replies; 16+ messages in thread
From: Herve Codina @ 2023-07-11 9:41 UTC (permalink / raw)
To: Chunfeng Yun (云春峰)
Cc: yhao016@ucr.edu, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
Hi Chunfeng,
On Tue, 11 Jul 2023 08:48:35 +0000
Chunfeng Yun (云春峰) <Chunfeng.Yun@mediatek.com> wrote:
> On Mon, 2023-07-10 at 08:25 +0200, Herve Codina wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > Hi Yu,
> >
> > On Sun, 9 Jul 2023 17:48:15 -0700
> > Yu Hao <yhao016@ucr.edu> wrote:
> >
> > > 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;
> >
> > 'count' should be tested against sizeof(*setup). Indeed, we need to
> > have a
> > setup data packet in the fifo.
> >
> > What do you think about:
> > if (count < sizef(*setup))
> > return -EINVAL;
> before call this function, already check the data length in fifo, it
> should be 8 bytes.
> see mtu3_ep0_isr(), about line 761.
Indeed, I missed that point.
Thanks for pointing it.
Regards,
Hervé
>
> I think no need this patch
>
> Thanks a lot
>
> >
> > >
> > > 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;
> >
> > Forward the error code to the caller ?
> >
> > ret = ep0_read_setup(mtu, &setup)
> > if (ret < 0)
> > return ret;
> >
> >
> > > 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;
> > > +
> >
> > Ok
> >
> > > ret = IRQ_HANDLED;
> > > break;
> > > default:
> >
> > Be careful, your patch is wrongly indented.
> > tabs replaced by 4 spaces. You need to keep tabs.
> >
> > Regards,
> > Hervé Codina
> >
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
@ 2023-07-11 9:41 ` Herve Codina
0 siblings, 0 replies; 16+ messages in thread
From: Herve Codina @ 2023-07-11 9:41 UTC (permalink / raw)
To: Chunfeng Yun (云春峰)
Cc: yhao016@ucr.edu, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
Hi Chunfeng,
On Tue, 11 Jul 2023 08:48:35 +0000
Chunfeng Yun (云春峰) <Chunfeng.Yun@mediatek.com> wrote:
> On Mon, 2023-07-10 at 08:25 +0200, Herve Codina wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > Hi Yu,
> >
> > On Sun, 9 Jul 2023 17:48:15 -0700
> > Yu Hao <yhao016@ucr.edu> wrote:
> >
> > > 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;
> >
> > 'count' should be tested against sizeof(*setup). Indeed, we need to
> > have a
> > setup data packet in the fifo.
> >
> > What do you think about:
> > if (count < sizef(*setup))
> > return -EINVAL;
> before call this function, already check the data length in fifo, it
> should be 8 bytes.
> see mtu3_ep0_isr(), about line 761.
Indeed, I missed that point.
Thanks for pointing it.
Regards,
Hervé
>
> I think no need this patch
>
> Thanks a lot
>
> >
> > >
> > > 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;
> >
> > Forward the error code to the caller ?
> >
> > ret = ep0_read_setup(mtu, &setup)
> > if (ret < 0)
> > return ret;
> >
> >
> > > 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;
> > > +
> >
> > Ok
> >
> > > ret = IRQ_HANDLED;
> > > break;
> > > default:
> >
> > Be careful, your patch is wrongly indented.
> > tabs replaced by 4 spaces. You need to keep tabs.
> >
> > Regards,
> > Hervé Codina
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
2023-07-10 0:48 ` Yu Hao
@ 2023-07-11 8:40 ` Chunfeng Yun (云春峰)
-1 siblings, 0 replies; 16+ messages in thread
From: Chunfeng Yun (云春峰) @ 2023-07-11 8:40 UTC (permalink / raw)
To: yhao016@ucr.edu, herve.codina@bootlin.com
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
On Sun, 2023-07-09 at 17:48 -0700, Yu Hao wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 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;
>
Which SoC encounter this issue?
if there is no data in fifo, no interrupt will arise, so don't read
setup packet.
> 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:
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
@ 2023-07-11 8:40 ` Chunfeng Yun (云春峰)
0 siblings, 0 replies; 16+ messages in thread
From: Chunfeng Yun (云春峰) @ 2023-07-11 8:40 UTC (permalink / raw)
To: yhao016@ucr.edu, herve.codina@bootlin.com
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
On Sun, 2023-07-09 at 17:48 -0700, Yu Hao wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 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;
>
Which SoC encounter this issue?
if there is no data in fifo, no interrupt will arise, so don't read
setup packet.
> 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:
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread