diff mbox series

[-next] usb: gadget: dereference before null check

Message ID 20220629080726.107297-1-mailmesebin00@gmail.com
State Handled Elsewhere, archived
Headers show
Series [-next] usb: gadget: dereference before null check | expand

Commit Message

Sebin Sebastian June 29, 2022, 8:07 a.m. UTC 
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

Signed-off-by: SebinSebastian <mailmesebin00@gmail.com>
---
 drivers/usb/gadget/udc/aspeed_udc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--
2.34.1

Comments

Sebin Sebastian June 29, 2022, 12:14 p.m. UTC | #1 
On Wed, Jun 29, 2022 at 10:24:07AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 29, 2022 at 01:37:25PM +0530, SebinSebastian 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.
> > Coverity issue: 1518209
> > 
> > Signed-off-by: SebinSebastian <mailmesebin00@gmail.com>
> > ---
> >  drivers/usb/gadget/udc/aspeed_udc.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> > index d75a4e070bf7..96f8193fca15 100644
> > --- a/drivers/usb/gadget/udc/aspeed_udc.c
> > +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> > @@ -341,10 +341,6 @@ 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;
> > @@ -356,6 +352,11 @@ static int ast_udc_ep_enable(struct usb_ep *_ep,
> >  		return -EINVAL;
> >  	}
> > 
> > +	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);
> > +
> >  	if (!udc->driver) {
> >  		EP_DBG(ep, "bogus device state\n");
> >  		return -ESHUTDOWN;
> > --
> > 2.34.1
> > 
> 
> Hi,
> 
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
> 
> You are receiving this message because of the following common error(s)
> as indicated below:
> 
> - Your patch breaks the build.
> 
> - Your patch contains warnings and/or errors noticed by the
>   scripts/checkpatch.pl tool.
> 
> - This looks like a new version of a previously submitted patch, but you
>   did not list below the --- line any changes from the previous version.
>   Please read the section entitled "The canonical patch format" in the
>   kernel file, Documentation/SubmittingPatches for what needs to be done
>   here to properly describe this.
> 
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
> 
> thanks,
> 
> greg k-h's patch email bot

I am sorry to keep on bothering with this incorrect patches. I am
running the checkpatch script everytime before I sent any patches. It is
not showing any warnings or errors. Is it because of my name that my
patches are getting rejected? I can see a space missing.
kernel test robot June 29, 2022, 12:39 p.m. UTC | #2 
Hi SebinSebastian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20220628]

url:    https://github.com/intel-lab-lkp/linux/commits/SebinSebastian/usb-gadget-dereference-before-null-check/20220629-161008
base:    cb71b93c2dc36d18a8b05245973328d018272cdf
config: mips-allyesconfig
compiler: mips-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/97ebbd93f269a58b3b5a003898d6e09c29a73ab0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review SebinSebastian/usb-gadget-dereference-before-null-check/20220629-161008
        git checkout 97ebbd93f269a58b3b5a003898d6e09c29a73ab0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/usb/gadget/udc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/gadget/udc/aspeed_udc.c: In function 'ast_udc_ep_enable':
   drivers/usb/gadget/udc/aspeed_udc.c:349:22: error: 'ep' undeclared (first use in this function); did you mean '_ep'?
     349 |         if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
         |                      ^~
         |                      _ep
   drivers/usb/gadget/udc/aspeed_udc.c:349:22: note: each undeclared identifier is reported only once for each function it appears in
   drivers/usb/gadget/udc/aspeed_udc.c:350:13: error: 'maxpacket' undeclared (first use in this function)
     350 |             maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
         |             ^~~~~~~~~
>> drivers/usb/gadget/udc/aspeed_udc.c:355:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     355 |         u16 maxpacket = usb_endpoint_maxp(desc);
         |         ^~~


vim +355 drivers/usb/gadget/udc/aspeed_udc.c

   340	
   341	static int ast_udc_ep_enable(struct usb_ep *_ep,
   342				     const struct usb_endpoint_descriptor *desc)
   343	{
   344		unsigned long flags;
   345		u32 ep_conf = 0;
   346		u8 dir_in;
   347		u8 type;
   348	
   349		if (!_ep || !ep || !desc || desc->bDescriptorType != USB_DT_ENDPOINT ||
   350		    maxpacket == 0 || maxpacket > ep->ep.maxpacket) {
   351			EP_DBG(ep, "Failed, invalid EP enable param\n");
   352			return -EINVAL;
   353		}
   354	
 > 355		u16 maxpacket = usb_endpoint_maxp(desc);
   356		struct ast_udc_ep *ep = to_ast_ep(_ep);
   357		struct ast_udc_dev *udc = ep->udc;
   358		u8 epnum = usb_endpoint_num(desc);
   359	
   360		if (!udc->driver) {
   361			EP_DBG(ep, "bogus device state\n");
   362			return -ESHUTDOWN;
   363		}
   364	
   365		EP_DBG(ep, "maxpacket:0x%x\n", maxpacket);
   366	
   367		spin_lock_irqsave(&udc->lock, flags);
   368	
   369		ep->desc = desc;
   370		ep->stopped = 0;
   371		ep->ep.maxpacket = maxpacket;
   372		ep->chunk_max = AST_EP_DMA_DESC_MAX_LEN;
   373	
   374		if (maxpacket < AST_UDC_EPn_MAX_PACKET)
   375			ep_conf = EP_SET_MAX_PKT(maxpacket);
   376	
   377		ep_conf |= EP_SET_EP_NUM(epnum);
   378	
   379		type = usb_endpoint_type(desc);
   380		dir_in = usb_endpoint_dir_in(desc);
   381		ep->dir_in = dir_in;
   382		if (!ep->dir_in)
   383			ep_conf |= EP_DIR_OUT;
   384	
   385		EP_DBG(ep, "type %d, dir_in %d\n", type, dir_in);
   386		switch (type) {
   387		case USB_ENDPOINT_XFER_ISOC:
   388			ep_conf |= EP_SET_TYPE_MASK(EP_TYPE_ISO);
   389			break;
   390	
   391		case USB_ENDPOINT_XFER_BULK:
   392			ep_conf |= EP_SET_TYPE_MASK(EP_TYPE_BULK);
   393			break;
   394	
   395		case USB_ENDPOINT_XFER_INT:
   396			ep_conf |= EP_SET_TYPE_MASK(EP_TYPE_INT);
   397			break;
   398		}
   399	
   400		ep->desc_mode = udc->desc_mode && ep->descs_dma && ep->dir_in;
   401		if (ep->desc_mode) {
   402			ast_ep_write(ep, EP_DMA_CTRL_RESET, AST_UDC_EP_DMA_CTRL);
   403			ast_ep_write(ep, 0, AST_UDC_EP_DMA_STS);
   404			ast_ep_write(ep, ep->descs_dma, AST_UDC_EP_DMA_BUFF);
   405	
   406			/* Enable Long Descriptor Mode */
   407			ast_ep_write(ep, EP_DMA_CTRL_IN_LONG_MODE | EP_DMA_DESC_MODE,
   408				     AST_UDC_EP_DMA_CTRL);
   409	
   410			ep->descs_wptr = 0;
   411	
   412		} else {
   413			ast_ep_write(ep, EP_DMA_CTRL_RESET, AST_UDC_EP_DMA_CTRL);
   414			ast_ep_write(ep, EP_DMA_SINGLE_STAGE, AST_UDC_EP_DMA_CTRL);
   415			ast_ep_write(ep, 0, AST_UDC_EP_DMA_STS);
   416		}
   417	
   418		/* Cleanup data toggle just in case */
   419		ast_udc_write(udc, EP_TOGGLE_SET_EPNUM(epnum), AST_VHUB_EP_DATA);
   420	
   421		/* Enable EP */
   422		ast_ep_write(ep, ep_conf | EP_ENABLE, AST_UDC_EP_CONFIG);
   423	
   424		EP_DBG(ep, "ep_config: 0x%x\n", ast_ep_read(ep, AST_UDC_EP_CONFIG));
   425	
   426		spin_unlock_irqrestore(&udc->lock, flags);
   427	
   428		return 0;
   429	}
   430
Sebin Sebastian June 30, 2022, 3:51 a.m. UTC | #3 
On Wed, Jun 29, 2022 at 05:31:57PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 29, 2022 at 05:44:55PM +0530, Sebin Sebastian wrote:
> > On Wed, Jun 29, 2022 at 10:24:07AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Jun 29, 2022 at 01:37:25PM +0530, SebinSebastian 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.
> > > > Coverity issue: 1518209
> > > > 
> > > > Signed-off-by: SebinSebastian <mailmesebin00@gmail.com>
> > > > ---
> > > >  drivers/usb/gadget/udc/aspeed_udc.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> > > > index d75a4e070bf7..96f8193fca15 100644
> > > > --- a/drivers/usb/gadget/udc/aspeed_udc.c
> > > > +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> > > > @@ -341,10 +341,6 @@ 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;
> > > > @@ -356,6 +352,11 @@ static int ast_udc_ep_enable(struct usb_ep *_ep,
> > > >  		return -EINVAL;
> > > >  	}
> > > > 
> > > > +	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);
> > > > +
> > > >  	if (!udc->driver) {
> > > >  		EP_DBG(ep, "bogus device state\n");
> > > >  		return -ESHUTDOWN;
> > > > --
> > > > 2.34.1
> > > > 
> > > 
> > > Hi,
> > > 
> > > This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> > > a patch that has triggered this response.  He used to manually respond
> > > to these common problems, but in order to save his sanity (he kept
> > > writing the same thing over and over, yet to different people), I was
> > > created.  Hopefully you will not take offence and will fix the problem
> > > in your patch and resubmit it so that it can be accepted into the Linux
> > > kernel tree.
> > > 
> > > You are receiving this message because of the following common error(s)
> > > as indicated below:
> > > 
> > > - Your patch breaks the build.
> > > 
> > > - Your patch contains warnings and/or errors noticed by the
> > >   scripts/checkpatch.pl tool.
> > > 
> > > - This looks like a new version of a previously submitted patch, but you
> > >   did not list below the --- line any changes from the previous version.
> > >   Please read the section entitled "The canonical patch format" in the
> > >   kernel file, Documentation/SubmittingPatches for what needs to be done
> > >   here to properly describe this.
> > > 
> > > If you wish to discuss this problem further, or you have questions about
> > > how to resolve this issue, please feel free to respond to this email and
> > > Greg will reply once he has dug out from the pending patches received
> > > from other developers.
> > > 
> > > thanks,
> > > 
> > > greg k-h's patch email bot
> > 
> > I am sorry to keep on bothering with this incorrect patches. I am
> > running the checkpatch script everytime before I sent any patches. It is
> > not showing any warnings or errors. Is it because of my name that my
> > patches are getting rejected? I can see a space missing.
> 
> Did you test build your patch?  If not, why not?
> 
> thanks,
> 
> greg k-h

Ok, now I understand the source of all errors. I did build the entire
tree, but make never touched udc. I have fixed all errors and warnings,
build the patch properly, ran through checkpatch and is now ready for
submission.
diff mbox series

Patch

diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
index d75a4e070bf7..96f8193fca15 100644
--- a/drivers/usb/gadget/udc/aspeed_udc.c
+++ b/drivers/usb/gadget/udc/aspeed_udc.c
@@ -341,10 +341,6 @@  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;
@@ -356,6 +352,11 @@  static int ast_udc_ep_enable(struct usb_ep *_ep,
 		return -EINVAL;
 	}

+	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);
+
 	if (!udc->driver) {
 		EP_DBG(ep, "bogus device state\n");
 		return -ESHUTDOWN;