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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Jun 2022 08:54:04 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Sebin Sebastian <mailmesebin00@...il.com>
Cc:     kernel test robot <lkp@...el.com>,
        Neal Liu <neal_liu@...eedtech.com>,
        Felipe Balbi <balbi@...nel.org>, Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...id.au>,
        linux-aspeed@...ts.ozlabs.org, linux-usb@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 -next] usb: gadget: dereference before null check

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@...el.com>

kernel test robot did not find this issue.

> Signed-off-by: Sebin Sebastian <mailmesebin00@...il.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