* [PATCH v2 -next] usb: gadget: dereference before null check @ 2022-06-30 4:47 Sebin Sebastian 2022-06-30 6:54 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Sebin Sebastian @ 2022-06-30 4:47 UTC (permalink / raw) Cc: mailmesebin00, skhan, kernel test robot, Neal Liu, Felipe Balbi, Greg Kroah-Hartman, Joel Stanley, Andrew Jeffery, linux-aspeed, linux-usb, linux-arm-kernel, linux-kernel 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 <lkp@intel.com> Signed-off-by: Sebin Sebastian <mailmesebin00@gmail.com> --- 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 _______________________________________________ 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] 5+ messages in thread
* Re: [PATCH v2 -next] usb: gadget: dereference before null check 2022-06-30 4:47 [PATCH v2 -next] usb: gadget: dereference before null check Sebin Sebastian @ 2022-06-30 6:54 ` Greg Kroah-Hartman 2022-06-30 11:03 ` Sebin Sebastian 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2022-06-30 6:54 UTC (permalink / raw) To: Sebin Sebastian Cc: kernel test robot, Neal Liu, Felipe Balbi, Joel Stanley, Andrew Jeffery, linux-aspeed, linux-usb, linux-arm-kernel, linux-kernel 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 <lkp@intel.com> kernel test robot did not find this issue. > Signed-off-by: Sebin Sebastian <mailmesebin00@gmail.com> 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 _______________________________________________ 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] 5+ messages in thread
* Re: [PATCH v2 -next] usb: gadget: dereference before null check 2022-06-30 6:54 ` Greg Kroah-Hartman @ 2022-06-30 11:03 ` Sebin Sebastian 2022-06-30 11:09 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Sebin Sebastian @ 2022-06-30 11:03 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: kernel test robot, Neal Liu, Felipe Balbi, Joel Stanley, Andrew Jeffery, linux-aspeed, linux-usb, linux-arm-kernel, linux-kernel 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 <lkp@intel.com> > > 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 <mailmesebin00@gmail.com> > > 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. _______________________________________________ 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] 5+ messages in thread
* Re: [PATCH v2 -next] usb: gadget: dereference before null check 2022-06-30 11:03 ` Sebin Sebastian @ 2022-06-30 11:09 ` Greg Kroah-Hartman 2022-06-30 12:25 ` Sebin Sebastian 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2022-06-30 11:09 UTC (permalink / raw) To: Sebin Sebastian Cc: kernel test robot, Neal Liu, Felipe Balbi, Joel Stanley, Andrew Jeffery, linux-aspeed, linux-usb, linux-arm-kernel, linux-kernel 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 <lkp@intel.com> > > > > 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 <mailmesebin00@gmail.com> > > > > 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 _______________________________________________ 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] 5+ messages in thread
* Re: [PATCH v2 -next] usb: gadget: dereference before null check 2022-06-30 11:09 ` Greg Kroah-Hartman @ 2022-06-30 12:25 ` Sebin Sebastian 0 siblings, 0 replies; 5+ messages in thread From: Sebin Sebastian @ 2022-06-30 12:25 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Neal Liu, Felipe Balbi, Joel Stanley, Andrew Jeffery, linux-aspeed, linux-usb, linux-arm-kernel, linux-kernel 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 <lkp@intel.com> > > > > > > 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 <mailmesebin00@gmail.com> > > > > > > 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. _______________________________________________ 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] 5+ messages in thread
end of thread, other threads:[~2022-06-30 12:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-06-30 4:47 [PATCH v2 -next] usb: gadget: dereference before null check Sebin Sebastian 2022-06-30 6:54 ` Greg Kroah-Hartman 2022-06-30 11:03 ` Sebin Sebastian 2022-06-30 11:09 ` Greg Kroah-Hartman 2022-06-30 12:25 ` Sebin Sebastian
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).