Fix coverity warning dereferencing before null check. _ep and desc is
dereferenced on all paths until the check for null. Move the
initializations after the check for null.
Coverity issue: 1518209
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Sebin Sebastian <[email protected]>
---
Changes since v1: Fix the build errors and warnings due to first patch.
Fix the undeclared 'ep' and 'maxpacket' error. Fix the ISO C90 warning.
drivers/usb/gadget/udc/aspeed_udc.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
index d75a4e070bf7..a43cf8dde2a8 100644
--- a/drivers/usb/gadget/udc/aspeed_udc.c
+++ b/drivers/usb/gadget/udc/aspeed_udc.c
@@ -341,26 +341,33 @@ static void ast_udc_stop_activity(struct ast_udc_dev *udc)
static int ast_udc_ep_enable(struct usb_ep *_ep,
const struct usb_endpoint_descriptor *desc)
{
- u16 maxpacket = usb_endpoint_maxp(desc);
- struct ast_udc_ep *ep = to_ast_ep(_ep);
- struct ast_udc_dev *udc = ep->udc;
- u8 epnum = usb_endpoint_num(desc);
unsigned long flags;
u32 ep_conf = 0;
u8 dir_in;
u8 type;
+ u16 maxpacket;
+ struct ast_udc_ep *ep;
+ struct ast_udc_dev *udc;
+ u8 epnum;
- if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
- maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
+ if (!_ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT) {
EP_DBG(ep, "Failed, invalid EP enable param\n");
return -EINVAL;
}
-
if (!udc->driver) {
EP_DBG(ep, "bogus device state\n");
return -ESHUTDOWN;
}
+ maxpacket = usb_endpoint_maxp(desc);
+ ep = to_ast_ep(_ep);
+ udc = ep->udc;
+ epnum = usb_endpoint_num(desc);
+ if (maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
+ EP_DBG(ep, "Failed, invalid EP enable param\n");
+ return -EINVAL;
+ }
+
EP_DBG(ep, "maxpacket:0x%x\n", maxpacket);
spin_lock_irqsave(&udc->lock, flags);
--
2.34.1
On Thu, Jun 30, 2022 at 10:17:06AM +0530, Sebin Sebastian wrote:
> Fix coverity warning dereferencing before null check. _ep and desc is
> dereferenced on all paths until the check for null. Move the
> initializations after the check for null.
How can those values ever be NULL?
> Coverity issue: 1518209
>
> Reported-by: kernel test robot <[email protected]>
kernel test robot did not find this issue.
> Signed-off-by: Sebin Sebastian <[email protected]>
What commit id does this change fix?
> ---
> Changes since v1: Fix the build errors and warnings due to first patch.
> Fix the undeclared 'ep' and 'maxpacket' error. Fix the ISO C90 warning.
>
> drivers/usb/gadget/udc/aspeed_udc.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> index d75a4e070bf7..a43cf8dde2a8 100644
> --- a/drivers/usb/gadget/udc/aspeed_udc.c
> +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> @@ -341,26 +341,33 @@ static void ast_udc_stop_activity(struct ast_udc_dev *udc)
> static int ast_udc_ep_enable(struct usb_ep *_ep,
> const struct usb_endpoint_descriptor *desc)
> {
> - u16 maxpacket = usb_endpoint_maxp(desc);
> - struct ast_udc_ep *ep = to_ast_ep(_ep);
checking that ep is NULL here is an impossible thing on its own. You
did change this so that you didn't check this anymore, which is odd as
you did not mention that in the changelog text :(
> - struct ast_udc_dev *udc = ep->udc;
> - u8 epnum = usb_endpoint_num(desc);
> unsigned long flags;
> u32 ep_conf = 0;
> u8 dir_in;
> u8 type;
> + u16 maxpacket;
> + struct ast_udc_ep *ep;
> + struct ast_udc_dev *udc;
> + u8 epnum;
Why did you reorder these?
>
> - if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
> - maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> + if (!_ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT) {
> EP_DBG(ep, "Failed, invalid EP enable param\n");
> return -EINVAL;
> }
> -
Why did you remove this line?
Also, your To: line is messed up somehow, please fix your email
client...
thanks,
gre gk-h
On Thu, Jun 30, 2022 at 04:33:42PM +0530, Sebin Sebastian wrote:
> On Thu, Jun 30, 2022 at 08:54:04AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 30, 2022 at 10:17:06AM +0530, Sebin Sebastian wrote:
> > > Fix coverity warning dereferencing before null check. _ep and desc is
> > > dereferenced on all paths until the check for null. Move the
> > > initializations after the check for null.
> >
> > How can those values ever be NULL?
> >
> > > Coverity issue: 1518209
> > >
> > > Reported-by: kernel test robot <[email protected]>
> >
> > kernel test robot did not find this issue.
> >
> After I submitted the PATCH v1, kernel test robot ran some tests and
> produced a report of the things that I broke while creating the patch.
> That's why I kept this tag.
Yes, but the kernel test robot reported your first patch was broken, not
that this commit itself was reported by that. Please drop that, it's
confusing I know, and trips lots of people up, but is not needed here.
>
> > > Signed-off-by: Sebin Sebastian <[email protected]>
> >
> > What commit id does this change fix?
> >
> So should I provide the commit ID of the patch v1 that kernel
> test robot referred to?
No, report the commit id that this commit you are creating fixes. It
had to be added to the tree sometime in the past, right?
>
> > > ---
> > > Changes since v1: Fix the build errors and warnings due to first patch.
> > > Fix the undeclared 'ep' and 'maxpacket' error. Fix the ISO C90 warning.
> > >
> > > drivers/usb/gadget/udc/aspeed_udc.c | 21 ++++++++++++++-------
> > > 1 file changed, 14 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> > > index d75a4e070bf7..a43cf8dde2a8 100644
> > > --- a/drivers/usb/gadget/udc/aspeed_udc.c
> > > +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> > > @@ -341,26 +341,33 @@ static void ast_udc_stop_activity(struct ast_udc_dev *udc)
> > > static int ast_udc_ep_enable(struct usb_ep *_ep,
> > > const struct usb_endpoint_descriptor *desc)
> > > {
> > > - u16 maxpacket = usb_endpoint_maxp(desc);
> > > - struct ast_udc_ep *ep = to_ast_ep(_ep);
> >
> > checking that ep is NULL here is an impossible thing on its own. You
> > did change this so that you didn't check this anymore, which is odd as
> > you did not mention that in the changelog text :(
> >
> Yes, I missed the checking for ep. I thought of checking it after
> initilizing ep.
>
> > > - struct ast_udc_dev *udc = ep->udc;
> > > - u8 epnum = usb_endpoint_num(desc);
> > > unsigned long flags;
> > > u32 ep_conf = 0;
> > > u8 dir_in;
> > > u8 type;
> > > + u16 maxpacket;
> > > + struct ast_udc_ep *ep;
> > > + struct ast_udc_dev *udc;
> > > + u8 epnum;
> >
> > Why did you reorder these?
> >
> This is actually the original order that these were in. I reordered it
> while creating the first patch, then I changed it back to the original
> order they were in the source tree for this patch.
So this patch does not apply cleanly on linux-next? We did not apply
your intermediate, broken, patch for obvious reasons, so you can not
send a change on top of that, right?
> > >
> > > - if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
> > > - maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> > > + if (!_ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT) {
> > > EP_DBG(ep, "Failed, invalid EP enable param\n");
> > > return -EINVAL;
> > > }
> > > -
> >
> > Why did you remove this line?
> >
> I removed the check for maxpacket because it is not initialized in this
> part, the check for the same thing comes after initialization.
> This is the check for that, this is also included in the patch.
> + if (maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> + EP_DBG(ep, "Failed, invalid EP enable param\n");
> + return -EINVAL;
> + }
> Should I add the check for 'ep' in this part?
>
> > Also, your To: line is messed up somehow, please fix your email
> > client...
> >
> Ok, I will surely do it.
>
> > thanks,
> >
> > gre gk-h
>
> I did many mistakes in the patch v1, so I had to bring this patch to the
> original state things were. I left all the declarations in the same
> order (which made it seem like reordering) and moved the initialization
> part after the check for _ep and desc.
Perhaps you might want to start out doing coding style cleanups in
drivers/staging/* to get the process of how to submit patches properly
and test your changes before sending them out, before going out into the
real part of the kernel.
thanks,
greg k-h
On Thu, Jun 30, 2022 at 08:54:04AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 30, 2022 at 10:17:06AM +0530, Sebin Sebastian wrote:
> > Fix coverity warning dereferencing before null check. _ep and desc is
> > dereferenced on all paths until the check for null. Move the
> > initializations after the check for null.
>
> How can those values ever be NULL?
>
> > Coverity issue: 1518209
> >
> > Reported-by: kernel test robot <[email protected]>
>
> kernel test robot did not find this issue.
>
After I submitted the PATCH v1, kernel test robot ran some tests and
produced a report of the things that I broke while creating the patch.
That's why I kept this tag.
> > Signed-off-by: Sebin Sebastian <[email protected]>
>
> What commit id does this change fix?
>
So should I provide the commit ID of the patch v1 that kernel
test robot referred to?
> > ---
> > Changes since v1: Fix the build errors and warnings due to first patch.
> > Fix the undeclared 'ep' and 'maxpacket' error. Fix the ISO C90 warning.
> >
> > drivers/usb/gadget/udc/aspeed_udc.c | 21 ++++++++++++++-------
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> > index d75a4e070bf7..a43cf8dde2a8 100644
> > --- a/drivers/usb/gadget/udc/aspeed_udc.c
> > +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> > @@ -341,26 +341,33 @@ static void ast_udc_stop_activity(struct ast_udc_dev *udc)
> > static int ast_udc_ep_enable(struct usb_ep *_ep,
> > const struct usb_endpoint_descriptor *desc)
> > {
> > - u16 maxpacket = usb_endpoint_maxp(desc);
> > - struct ast_udc_ep *ep = to_ast_ep(_ep);
>
> checking that ep is NULL here is an impossible thing on its own. You
> did change this so that you didn't check this anymore, which is odd as
> you did not mention that in the changelog text :(
>
Yes, I missed the checking for ep. I thought of checking it after
initilizing ep.
> > - struct ast_udc_dev *udc = ep->udc;
> > - u8 epnum = usb_endpoint_num(desc);
> > unsigned long flags;
> > u32 ep_conf = 0;
> > u8 dir_in;
> > u8 type;
> > + u16 maxpacket;
> > + struct ast_udc_ep *ep;
> > + struct ast_udc_dev *udc;
> > + u8 epnum;
>
> Why did you reorder these?
>
This is actually the original order that these were in. I reordered it
while creating the first patch, then I changed it back to the original
order they were in the source tree for this patch.
> >
> > - if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
> > - maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> > + if (!_ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT) {
> > EP_DBG(ep, "Failed, invalid EP enable param\n");
> > return -EINVAL;
> > }
> > -
>
> Why did you remove this line?
>
I removed the check for maxpacket because it is not initialized in this
part, the check for the same thing comes after initialization.
This is the check for that, this is also included in the patch.
+ if (maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
+ EP_DBG(ep, "Failed, invalid EP enable param\n");
+ return -EINVAL;
+ }
Should I add the check for 'ep' in this part?
> Also, your To: line is messed up somehow, please fix your email
> client...
>
Ok, I will surely do it.
> thanks,
>
> gre gk-h
I did many mistakes in the patch v1, so I had to bring this patch to the
original state things were. I left all the declarations in the same
order (which made it seem like reordering) and moved the initialization
part after the check for _ep and desc.
On Thu, Jun 30, 2022 at 01:09:28PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 30, 2022 at 04:33:42PM +0530, Sebin Sebastian wrote:
> > On Thu, Jun 30, 2022 at 08:54:04AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 30, 2022 at 10:17:06AM +0530, Sebin Sebastian wrote:
> > > > Fix coverity warning dereferencing before null check. _ep and desc is
> > > > dereferenced on all paths until the check for null. Move the
> > > > initializations after the check for null.
> > >
> > > How can those values ever be NULL?
> > >
> > > > Coverity issue: 1518209
> > > >
> > > > Reported-by: kernel test robot <[email protected]>
> > >
> > > kernel test robot did not find this issue.
> > >
> > After I submitted the PATCH v1, kernel test robot ran some tests and
> > produced a report of the things that I broke while creating the patch.
> > That's why I kept this tag.
>
> Yes, but the kernel test robot reported your first patch was broken, not
> that this commit itself was reported by that. Please drop that, it's
> confusing I know, and trips lots of people up, but is not needed here.
>
> >
> > > > Signed-off-by: Sebin Sebastian <[email protected]>
> > >
> > > What commit id does this change fix?
> > >
> > So should I provide the commit ID of the patch v1 that kernel
> > test robot referred to?
>
> No, report the commit id that this commit you are creating fixes. It
> had to be added to the tree sometime in the past, right?
>
> >
> > > > ---
> > > > Changes since v1: Fix the build errors and warnings due to first patch.
> > > > Fix the undeclared 'ep' and 'maxpacket' error. Fix the ISO C90 warning.
> > > >
> > > > drivers/usb/gadget/udc/aspeed_udc.c | 21 ++++++++++++++-------
> > > > 1 file changed, 14 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> > > > index d75a4e070bf7..a43cf8dde2a8 100644
> > > > --- a/drivers/usb/gadget/udc/aspeed_udc.c
> > > > +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> > > > @@ -341,26 +341,33 @@ static void ast_udc_stop_activity(struct ast_udc_dev *udc)
> > > > static int ast_udc_ep_enable(struct usb_ep *_ep,
> > > > const struct usb_endpoint_descriptor *desc)
> > > > {
> > > > - u16 maxpacket = usb_endpoint_maxp(desc);
> > > > - struct ast_udc_ep *ep = to_ast_ep(_ep);
> > >
> > > checking that ep is NULL here is an impossible thing on its own. You
> > > did change this so that you didn't check this anymore, which is odd as
> > > you did not mention that in the changelog text :(
> > >
> > Yes, I missed the checking for ep. I thought of checking it after
> > initilizing ep.
> >
> > > > - struct ast_udc_dev *udc = ep->udc;
> > > > - u8 epnum = usb_endpoint_num(desc);
> > > > unsigned long flags;
> > > > u32 ep_conf = 0;
> > > > u8 dir_in;
> > > > u8 type;
> > > > + u16 maxpacket;
> > > > + struct ast_udc_ep *ep;
> > > > + struct ast_udc_dev *udc;
> > > > + u8 epnum;
> > >
> > > Why did you reorder these?
> > >
> > This is actually the original order that these were in. I reordered it
> > while creating the first patch, then I changed it back to the original
> > order they were in the source tree for this patch.
>
> So this patch does not apply cleanly on linux-next? We did not apply
> your intermediate, broken, patch for obvious reasons, so you can not
> send a change on top of that, right?
>
> > > >
> > > > - if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
> > > > - maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> > > > + if (!_ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT) {
> > > > EP_DBG(ep, "Failed, invalid EP enable param\n");
> > > > return -EINVAL;
> > > > }
> > > > -
> > >
> > > Why did you remove this line?
> > >
> > I removed the check for maxpacket because it is not initialized in this
> > part, the check for the same thing comes after initialization.
> > This is the check for that, this is also included in the patch.
> > + if (maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
> > + EP_DBG(ep, "Failed, invalid EP enable param\n");
> > + return -EINVAL;
> > + }
> > Should I add the check for 'ep' in this part?
> >
> > > Also, your To: line is messed up somehow, please fix your email
> > > client...
> > >
> > Ok, I will surely do it.
> >
> > > thanks,
> > >
> > > gre gk-h
> >
> > I did many mistakes in the patch v1, so I had to bring this patch to the
> > original state things were. I left all the declarations in the same
> > order (which made it seem like reordering) and moved the initialization
> > part after the check for _ep and desc.
>
>
> Perhaps you might want to start out doing coding style cleanups in
> drivers/staging/* to get the process of how to submit patches properly
> and test your changes before sending them out, before going out into the
> real part of the kernel.
>
> thanks,
>
> greg k-h
I am sorry to mess things up like this. I understand the entire process
of sending out patches and to not waste maintainer's time on these kind
of things.
This patch does apply cleanly on linux-next. There are no warnings or
errors while building and no checkpatch errors. Can I just send one
final patch including proper commit ID and the missing check for ep.
Thanks for pointing out all these mistakes.