netdev - Re: [PATCH 1/1] ax88172a: fix ax88172a

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, 14 Jul 2020 17:34:33 -0400
From:   George Kennedy <george.kennedy@...cle.com>
To:     David Miller <davem@...emloft.net>, dan.carpenter@...cle.com
Cc:     kuba@...nel.org, dhaval.giani@...cle.com, netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures



On 7/14/2020 5:03 PM, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@...cle.com>
> Date: Tue, 14 Jul 2020 11:00:38 +0300
>
>> On Mon, Jul 13, 2020 at 05:08:59PM -0700, David Miller wrote:
>>> From: George Kennedy <george.kennedy@...cle.com>
>>> Date: Mon, 13 Jul 2020 07:58:57 -0400
>>>
>>>> @@ -237,6 +237,8 @@ static int ax88172a_bind(struct usbnet *dev, struct usb_interface *intf)
>>>>   
>>>>   free:
>>>>   	kfree(priv);
>>>> +	if (ret >= 0)
>>>> +		ret = -EIO;
>>>>   	return ret;
>>> Success paths reach here, so ">= 0" is not appropriate.  Maybe you
>>> meant "> 0"?
>> No, the success path is the "return 0;" one line before the start of the
>> diff.  This is always a failure path.
> Is zero ever a possibility, therefore?
>
> You have two cases, one with an explicit -EIO and another which jumps
> here "if (ret)"
>
> So it seems the answer is no.
The "free:" label is the failure path. The "free:" label can be gotten 
to with "ret" >= 0, but the failure path must exit with ret < 0 for 
proper failure cleanup.

For example, the failing case here has "ret" = 0 (#define ETH_ALEN 6):

     172 static int ax88172a_bind(struct usbnet *dev, struct 
usb_interface *intf)
     173 {
...
     186         /* Get the MAC address */
     187         ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID, 0, 0, 
ETH_ALEN, buf, 0);
     188         if (ret < ETH_ALEN) {
     189                 netdev_err(dev->net, "Failed to read MAC 
address: %d\n", ret);
     190                 goto free;
     191         }
"drivers/net/usb/ax88172a.c"

The caller to ax88172a_bind() is usbnet_probe() and in the case of 
failure, it needs the return value to be < 0.

    1653 int
    1654 usbnet_probe (struct usb_interface *udev, const struct 
usb_device_id *prod)
    1655 {
...
    1736         if (info->bind) {
    1737                 status = info->bind (dev, udev);
    1738                 if (status < 0)
    1739                         goto out1;
"drivers/net/usb/usbnet.c"

George