[PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[George Kennedy]

If ax88172a_unbind() fails, make sure that the return code is
less than zero so that cleanup is done properly and avoid UAF.

Signed-off-by: George Kennedy <george.kennedy@oracle.com>
Reported-by: syzbot+4cd84f527bf4a10fc9c1@syzkaller.appspotmail.com
---
 drivers/net/usb/ax88172a.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c
index 4e514f5..fd9faf2 100644
--- a/drivers/net/usb/ax88172a.c
+++ b/drivers/net/usb/ax88172a.c
@@ -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;
 }
 
-- 
1.8.3.1

Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[David Miller]

From: George Kennedy <george.kennedy@oracle.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"?

Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[Dan Carpenter]

On Mon, Jul 13, 2020 at 05:08:59PM -0700, David Miller wrote:
> From: George Kennedy <george.kennedy@oracle.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.

regards,
dan carpenter

Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.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@oracle.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.

Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[George Kennedy]

On 7/14/2020 5:03 PM, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.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@oracle.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

Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[David Miller]

From: George Kennedy <george.kennedy@oracle.com>
Date: Tue, 14 Jul 2020 17:34:33 -0400

> 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"

Then this is the spot that should set 'ret' to -EINVAL or similar?

Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures

[George Kennedy]

On 7/14/2020 5:37 PM, David Miller wrote:
> From: George Kennedy <george.kennedy@oracle.com>
> Date: Tue, 14 Jul 2020 17:34:33 -0400
>
>> 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"
> Then this is the spot that should set 'ret' to -EINVAL or similar?

Made the suggested fix and sent the updated patch with following:

Subject: [PATCH v2 1/1] ax88172a: fix ax88172a_unbind() failures

George