| Message ID | 1594641537-1288-1-git-send-email-george.kennedy@oracle.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/1] ax88172a: fix ax88172a_unbind() failures | expand |
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"?
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
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.
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
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?
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
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; }
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(+)