linux-kernel - Re: [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw

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:   Tue, 24 Aug 2021 09:58:25 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     Larry.Finger@...inger.net, phil@...lpotter.co.uk,
        gregkh@...uxfoundation.org, straube.linux@...il.com,
        fmdefrancesco@...il.com, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v2 5/6] staging: r8188eu: add error handling of
 rtw_read32

On Sun, Aug 22, 2021 at 05:36:01PM +0300, Pavel Skripkin wrote:
> -static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> +static int usb_read32(struct intf_hdl *pintfhdl, u32 addr, u32 *data)
>  {
>  	u8 requesttype;
>  	u16 wvalue;
>  	u16 len;
> -	__le32 data;
> +	int res;
> +	__le32 tmp;
> +
> +	if (WARN_ON(unlikely(!data)))
> +		return -EINVAL;
>  
>  	requesttype = 0x01;/* read_in */
>  
>  	wvalue = (u16)(addr & 0x0000ffff);
>  	len = 4;
>  
> -	usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> +	res = usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> +	if (res < 0) {
> +		dev_err(dvobj_to_dev(pintfhdl->pintf_dev), "Failed to read 32 bytes: %d\n", res);

Add a return here.  Try to keep the success path and the failure path
as separate as possible.  Try to keep the success path indented at one
tab so the code looks like this:

	success();
	success();
	if (fail)
		handle_failure();
	success();
	success();

Try to deal with exceptions as quickly as possible so that the reader
has less to remember.

> +	} else {
> +		/* Noone cares about positive return value */

Ugh...  That's unfortunate.  We should actually care.  The
usbctrl_vendorreq() has an information leak where it copies len (4)
bytes of data even if usb_control_msg() is not able to read len bytes.

The best fix would be to remove the information leak and make
usbctrl_vendorreq() return zero on success.  In other words something
like:

	status = usb_control_msg();
	if (status < 0)
		return status;
	if (status != len)
		return -EIO;
	status = 0;

> +		*data = le32_to_cpu(tmp);
> +		res = 0;
> +	}
>  
> -	return le32_to_cpu(data);
> +	return res;
>  }