Re: [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
>  }





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux