netdev - Re: [PATCH] net: ax88179: add proper error handling of usb read errors

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] [day] [month] [year] [list]

Date:   Mon, 16 May 2022 12:31:01 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     David Kahurani <k.kahurani@...il.com>
Cc:     netdev@...r.kernel.org,
        syzbot+d3dbdf31fbe9d8f5f311@...kaller.appspotmail.com,
        davem@...emloft.net, jgg@...pe.ca, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org, phil@...lpotter.co.uk,
        syzkaller-bugs@...glegroups.com, arnd@...db.de,
        dan.carpenter@...cle.com
Subject: Re: [PATCH] net: ax88179: add proper error handling of usb read
 errors

On Sat, 14 May 2022 16:32:34 +0300 David Kahurani wrote:
> -			ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> -					 1, 1, &buf);
> +			ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> +					       1, 1, &buf);
> +			if (ret) {
> +				netdev_dbg(dev->net,
> +					   "Failed to read SROM_CMD: %d\n",
> +					   ret);
> +				return ret;
> +			}
>  
>  			if (time_after(jiffies, jtimeout))
>  				return -EINVAL;
>  
>  		} while (buf & EEP_BUSY);

I agree with Pavel, this seems unnecessarily strict. If the error is
not ENODEV we can keep looping.

> @@ -1581,7 +1731,13 @@ static int ax88179_link_reset(struct usbnet *dev)
>  				  &ax179_data->rxctl);
>  
>  		/*link up, check the usb device control TX FIFO full or empty*/
> -		ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> +		ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> +
> +		if (ret) {
> +			netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
> +				   ret);
> +			return ret;
> +		}

Please don't add any empty lines within the error checking.
Empty lines are supposed to separate logically separate blocks of code.
Error checking is very much logically part of the call. And no empty
line betwee netdev_dbg() and return ret; either. In this submission you
have all possible configurations of having or not having empty lines
before the if or before the return. None of them should be there, and
please be consistent.