| Message ID | 20230628081511.186850-1-make_ruc2021@163.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ce9daa2efc0872a9a68ea51dc8000df05893ef2e |
| Headers | show |
| Series | usb: gadget: fsl_qe_udc: validate endpoint index for ch9 udc | expand |
> -----Original Message----- > From: Ma Ke <make_ruc2021@163.com> > Sent: Wednesday, June 28, 2023 3:15 AM > To: Leo Li <leoyang.li@nxp.com> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Ma Ke > <make_ruc2021@163.com> > Subject: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for ch9 > udc > > We should verify the bound of the array to assure that host may not > manipulate the index to point past endpoint array. > > Signed-off-by: Ma Ke <make_ruc2021@163.com> > --- > drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c > b/drivers/usb/gadget/udc/fsl_qe_udc.c > index 3b1cc8fa30c8..f4e5cbd193b7 100644 > --- a/drivers/usb/gadget/udc/fsl_qe_udc.c > +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c > @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc *udc, u8 > request_type, u16 value, > } else if ((request_type & USB_RECIP_MASK) == > USB_RECIP_ENDPOINT) { > /* Get endpoint status */ > int pipe = index & USB_ENDPOINT_NUMBER_MASK; > + if (pipe >= USB_MAX_ENDPOINTS) > + goto stall; Thanks. This seems to be the right thing to do. But normally we don't mix declarations with code within a code block. Could we re-arrange the code a little bit so declarations stay on top? > struct qe_ep *target_ep = &udc->eps[pipe]; > u16 usep; > > -- > 2.37.2
Le 28/06/2023 à 19:04, Leo Li a écrit : > > >> -----Original Message----- >> From: Ma Ke <make_ruc2021@163.com> >> Sent: Wednesday, June 28, 2023 3:15 AM >> To: Leo Li <leoyang.li@nxp.com> >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- >> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Ma Ke >> <make_ruc2021@163.com> >> Subject: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for ch9 >> udc >> >> We should verify the bound of the array to assure that host may not >> manipulate the index to point past endpoint array. >> >> Signed-off-by: Ma Ke <make_ruc2021@163.com> >> --- >> drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c >> b/drivers/usb/gadget/udc/fsl_qe_udc.c >> index 3b1cc8fa30c8..f4e5cbd193b7 100644 >> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c >> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c >> @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc *udc, u8 >> request_type, u16 value, >> } else if ((request_type & USB_RECIP_MASK) == >> USB_RECIP_ENDPOINT) { >> /* Get endpoint status */ >> int pipe = index & USB_ENDPOINT_NUMBER_MASK; >> + if (pipe >= USB_MAX_ENDPOINTS) >> + goto stall; > > Thanks. This seems to be the right thing to do. But normally we don't mix declarations with code within a code block. Could we re-arrange the code a little bit so declarations stay on top? But we are at the start of a code block aren't we ? The only missing thing is the blank line between the declarations and the code, so that we clearly see where declarations end and where code start. > >> struct qe_ep *target_ep = &udc->eps[pipe]; >> u16 usep; >> >> -- >> 2.37.2 >
> -----Original Message----- > From: Christophe Leroy <christophe.leroy@csgroup.eu> > Sent: Wednesday, June 28, 2023 2:40 PM > To: Leo Li <leoyang.li@nxp.com>; Ma Ke <make_ruc2021@163.com> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for > ch9 udc > > > > Le 28/06/2023 à 19:04, Leo Li a écrit : > > > > > >> -----Original Message----- > >> From: Ma Ke <make_ruc2021@163.com> > >> Sent: Wednesday, June 28, 2023 3:15 AM > >> To: Leo Li <leoyang.li@nxp.com> > >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- > >> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Ma Ke > >> <make_ruc2021@163.com> > >> Subject: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for > >> ch9 udc > >> > >> We should verify the bound of the array to assure that host may not > >> manipulate the index to point past endpoint array. > >> > >> Signed-off-by: Ma Ke <make_ruc2021@163.com> > >> --- > >> drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c > >> b/drivers/usb/gadget/udc/fsl_qe_udc.c > >> index 3b1cc8fa30c8..f4e5cbd193b7 100644 > >> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c > >> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c > >> @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc *udc, u8 > >> request_type, u16 value, > >> } else if ((request_type & USB_RECIP_MASK) == > >> USB_RECIP_ENDPOINT) { > >> /* Get endpoint status */ > >> int pipe = index & USB_ENDPOINT_NUMBER_MASK; > >> + if (pipe >= USB_MAX_ENDPOINTS) > >> + goto stall; > > > > Thanks. This seems to be the right thing to do. But normally we don't mix > declarations with code within a code block. Could we re-arrange the code a > little bit so declarations stay on top? > > But we are at the start of a code block aren't we ? But they were at the beginning of a { } block which is compliant with the C89 standard. I know gcc is more relaxed from this. But it is probably still good to stick to the standard? > > The only missing thing is the blank line between the declarations and the > code, so that we clearly see where declarations end and where code start. > > > > >> struct qe_ep *target_ep = &udc->eps[pipe]; > >> u16 usep; > >> > >> -- > >> 2.37.2 > >
Le 28/06/2023 à 23:10, Leo Li a écrit : > > >> -----Original Message----- >> From: Christophe Leroy <christophe.leroy@csgroup.eu> >> Sent: Wednesday, June 28, 2023 2:40 PM >> To: Leo Li <leoyang.li@nxp.com>; Ma Ke <make_ruc2021@163.com> >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- >> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for >> ch9 udc >> >> >> >> Le 28/06/2023 à 19:04, Leo Li a écrit : >>> >>> >>>> -----Original Message----- >>>> From: Ma Ke <make_ruc2021@163.com> >>>> Sent: Wednesday, June 28, 2023 3:15 AM >>>> To: Leo Li <leoyang.li@nxp.com> >>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- >>>> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Ma Ke >>>> <make_ruc2021@163.com> >>>> Subject: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for >>>> ch9 udc >>>> >>>> We should verify the bound of the array to assure that host may not >>>> manipulate the index to point past endpoint array. >>>> >>>> Signed-off-by: Ma Ke <make_ruc2021@163.com> >>>> --- >>>> drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c >>>> b/drivers/usb/gadget/udc/fsl_qe_udc.c >>>> index 3b1cc8fa30c8..f4e5cbd193b7 100644 >>>> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c >>>> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c >>>> @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc *udc, u8 >>>> request_type, u16 value, >>>> } else if ((request_type & USB_RECIP_MASK) == >>>> USB_RECIP_ENDPOINT) { >>>> /* Get endpoint status */ >>>> int pipe = index & USB_ENDPOINT_NUMBER_MASK; >>>> + if (pipe >= USB_MAX_ENDPOINTS) >>>> + goto stall; >>> >>> Thanks. This seems to be the right thing to do. But normally we don't mix >> declarations with code within a code block. Could we re-arrange the code a >> little bit so declarations stay on top? >> >> But we are at the start of a code block aren't we ? > > But they were at the beginning of a { } block which is compliant with the C89 standard. I know gcc is more relaxed from this. But it is probably still good to stick to the standard? Sorry I misread the patch and failed to see that the declaration block was continuing after the change. So yes don't interleave code with declarations. Leave declaration at the top of a block with a blank line between declarations and code. > >> >> The only missing thing is the blank line between the declarations and the >> code, so that we clearly see where declarations end and where code start. >> >>> >>>> struct qe_ep *target_ep = &udc->eps[pipe]; >>>> u16 usep; >>>> >>>> -- >>>> 2.37.2 >>>
On Thu, Jun 29, 2023 at 05:56:30AM +0000, Christophe Leroy wrote: > > > Le 28/06/2023 à 23:10, Leo Li a écrit : > > > > > >> -----Original Message----- > >> From: Christophe Leroy <christophe.leroy@csgroup.eu> > >> Sent: Wednesday, June 28, 2023 2:40 PM > >> To: Leo Li <leoyang.li@nxp.com>; Ma Ke <make_ruc2021@163.com> > >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- > >> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for > >> ch9 udc > >> > >> > >> > >> Le 28/06/2023 à 19:04, Leo Li a écrit : > >>> > >>> > >>>> -----Original Message----- > >>>> From: Ma Ke <make_ruc2021@163.com> > >>>> Sent: Wednesday, June 28, 2023 3:15 AM > >>>> To: Leo Li <leoyang.li@nxp.com> > >>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linuxppc- > >>>> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Ma Ke > >>>> <make_ruc2021@163.com> > >>>> Subject: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for > >>>> ch9 udc > >>>> > >>>> We should verify the bound of the array to assure that host may not > >>>> manipulate the index to point past endpoint array. > >>>> > >>>> Signed-off-by: Ma Ke <make_ruc2021@163.com> > >>>> --- > >>>> drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++ > >>>> 1 file changed, 2 insertions(+) > >>>> > >>>> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c > >>>> b/drivers/usb/gadget/udc/fsl_qe_udc.c > >>>> index 3b1cc8fa30c8..f4e5cbd193b7 100644 > >>>> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c > >>>> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c > >>>> @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc *udc, u8 > >>>> request_type, u16 value, > >>>> } else if ((request_type & USB_RECIP_MASK) == > >>>> USB_RECIP_ENDPOINT) { > >>>> /* Get endpoint status */ > >>>> int pipe = index & USB_ENDPOINT_NUMBER_MASK; > >>>> + if (pipe >= USB_MAX_ENDPOINTS) > >>>> + goto stall; > >>> > >>> Thanks. This seems to be the right thing to do. But normally we don't mix > >> declarations with code within a code block. Could we re-arrange the code a > >> little bit so declarations stay on top? > >> > >> But we are at the start of a code block aren't we ? > > > > But they were at the beginning of a { } block which is compliant with the C89 standard. I know gcc is more relaxed from this. But it is probably still good to stick to the standard? > > Sorry I misread the patch and failed to see that the declaration block > was continuing after the change. > > So yes don't interleave code with declarations. Leave declaration at the > top of a block with a blank line between declarations and code. This is fine as-is, no need to change anything. greg k-h
> -----Original Message----- > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> > Sent: Thursday, June 29, 2023 3:41 AM > To: Christophe Leroy <christophe.leroy@csgroup.eu> > Cc: Leo Li <leoyang.li@nxp.com>; Ma Ke <make_ruc2021@163.com>; linux- > usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index for > ch9 udc > > On Thu, Jun 29, 2023 at 05:56:30AM +0000, Christophe Leroy wrote: > > > > > > Le 28/06/2023 à 23:10, Leo Li a écrit : > > > > > > > > >> -----Original Message----- > > >> From: Christophe Leroy <christophe.leroy@csgroup.eu> > > >> Sent: Wednesday, June 28, 2023 2:40 PM > > >> To: Leo Li <leoyang.li@nxp.com>; Ma Ke <make_ruc2021@163.com> > > >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > >> linuxppc- dev@lists.ozlabs.org; linux-kernel@vger.kernel.org > > >> Subject: Re: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint > > >> index for > > >> ch9 udc > > >> > > >> > > >> > > >> Le 28/06/2023 à 19:04, Leo Li a écrit : > > >>> > > >>> > > >>>> -----Original Message----- > > >>>> From: Ma Ke <make_ruc2021@163.com> > > >>>> Sent: Wednesday, June 28, 2023 3:15 AM > > >>>> To: Leo Li <leoyang.li@nxp.com> > > >>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > >>>> linuxppc- dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Ma > > >>>> Ke <make_ruc2021@163.com> > > >>>> Subject: [PATCH] usb: gadget: fsl_qe_udc: validate endpoint index > > >>>> for > > >>>> ch9 udc > > >>>> > > >>>> We should verify the bound of the array to assure that host may > > >>>> not manipulate the index to point past endpoint array. > > >>>> > > >>>> Signed-off-by: Ma Ke <make_ruc2021@163.com> > > >>>> --- > > >>>> drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++ > > >>>> 1 file changed, 2 insertions(+) > > >>>> > > >>>> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c > > >>>> b/drivers/usb/gadget/udc/fsl_qe_udc.c > > >>>> index 3b1cc8fa30c8..f4e5cbd193b7 100644 > > >>>> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c > > >>>> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c > > >>>> @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc > > >>>> *udc, u8 request_type, u16 value, > > >>>> } else if ((request_type & USB_RECIP_MASK) == > > >>>> USB_RECIP_ENDPOINT) { > > >>>> /* Get endpoint status */ > > >>>> int pipe = index & USB_ENDPOINT_NUMBER_MASK; > > >>>> + if (pipe >= USB_MAX_ENDPOINTS) > > >>>> + goto stall; > > >>> > > >>> Thanks. This seems to be the right thing to do. But normally we > > >>> don't mix > > >> declarations with code within a code block. Could we re-arrange > > >> the code a little bit so declarations stay on top? > > >> > > >> But we are at the start of a code block aren't we ? > > > > > > But they were at the beginning of a { } block which is compliant with the > C89 standard. I know gcc is more relaxed from this. But it is probably still > good to stick to the standard? > > > > Sorry I misread the patch and failed to see that the declaration block > > was continuing after the change. > > > > So yes don't interleave code with declarations. Leave declaration at > > the top of a block with a blank line between declarations and code. > > This is fine as-is, no need to change anything. With the approval from Greg, I have no objection to the patch. Acked-by: Li Yang <leoyang.li@nxp.com> Regards, Leo
diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c b/drivers/usb/gadget/udc/fsl_qe_udc.c index 3b1cc8fa30c8..f4e5cbd193b7 100644 --- a/drivers/usb/gadget/udc/fsl_qe_udc.c +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c @@ -1959,6 +1959,8 @@ static void ch9getstatus(struct qe_udc *udc, u8 request_type, u16 value, } else if ((request_type & USB_RECIP_MASK) == USB_RECIP_ENDPOINT) { /* Get endpoint status */ int pipe = index & USB_ENDPOINT_NUMBER_MASK; + if (pipe >= USB_MAX_ENDPOINTS) + goto stall; struct qe_ep *target_ep = &udc->eps[pipe]; u16 usep;
We should verify the bound of the array to assure that host may not manipulate the index to point past endpoint array. Signed-off-by: Ma Ke <make_ruc2021@163.com> --- drivers/usb/gadget/udc/fsl_qe_udc.c | 2 ++ 1 file changed, 2 insertions(+)