linux-kernel - Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac"

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]

Message-ID: <20220414154949.GM3293@kadam>
Date:   Thu, 14 Apr 2022 18:49:50 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Wang Cheng <wanngchenng@...il.com>
Cc:     Larry.Finger@...inger.net, florian.c.schilhabel@...glemail.com,
        gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac"

On Thu, Apr 14, 2022 at 06:42:15PM +0300, Dan Carpenter wrote:
> > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c
> > index f984a5ab2c6f..e321ca4453ca 100644
> > --- a/drivers/staging/rtl8712/usb_ops_linux.c
> > +++ b/drivers/staging/rtl8712/usb_ops_linux.c
> > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value,
> >  	}
> >  	status = usb_control_msg(udev, pipe, request, reqtype, value, index,
> >  				 pIo_buf, len, 500);
> > -	if (status > 0) {  /* Success this control transfer. */
> > -		if (requesttype == 0x01) {
> > -			/* For Control read transfer, we have to copy the read
> > -			 * data from pIo_buf to pdata.
> > -			 */
> > -			memcpy(pdata, pIo_buf,  status);
> > +	/* For Control read transfer, copy the read data from pIo_buf to pdata
> > +	 * when control transfer success; otherwise init *pdata with 0.
> > +	 */
> > +	if (requesttype == 0x01) {
> > +		if (status > 0)
> > +			memcpy(pdata, pIo_buf, status);
> > +		else
> > +			*(u32 *)pdata = 0;
> >  		}
> 
> This isn't really correct.  In many cases status is "len" is less than 4.
> I'm slightly surprised that nothing complains about that as an
> uninitialized access.  But then another problem is that "status" can be
> less than "len".
> 
> A better fix instead of setting pdata to zero would be to add error
> checking in the callers and then change this code to use
> usb_control_msg_send/recv().  Probably just initialize "data" in the
> callers as well.

A different, less good option would be to, still add error handling in
the callers, but do:

	status = usb_control_msg();
	if (status < 0)
		goto free;
	if (status != len) {
		status = -EREMOTEIO;
		goto free;
	}
	if (requesttype == 0x01)
		memcpy(pdata, pIo_buf, status);

	status = 0;
free:
	kfree(palloc_buf);
	return status;

regards,
dan carpenter