Re: [PATCH v2 -next] usb: gadget: dereference before null check

[Sebin Sebastian]

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@xxxxxxxxx>
> > > 
> > > 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@xxxxxxxxx>
> > > 
> > > 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.