* [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures @ 2020-07-13 11:58 George Kennedy 2020-07-14 0:08 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: George Kennedy @ 2020-07-13 11:58 UTC (permalink / raw) To: george.kennedy, davem, kuba, dan.carpenter, dhaval.giani, netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures 2020-07-13 11:58 [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures George Kennedy @ 2020-07-14 0:08 ` David Miller 2020-07-14 8:00 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2020-07-14 0:08 UTC (permalink / raw) To: george.kennedy; +Cc: kuba, dan.carpenter, dhaval.giani, netdev 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"? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures 2020-07-14 0:08 ` David Miller @ 2020-07-14 8:00 ` Dan Carpenter 2020-07-14 21:03 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2020-07-14 8:00 UTC (permalink / raw) To: David Miller; +Cc: george.kennedy, kuba, dhaval.giani, netdev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures 2020-07-14 8:00 ` Dan Carpenter @ 2020-07-14 21:03 ` David Miller 2020-07-14 21:34 ` George Kennedy 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2020-07-14 21:03 UTC (permalink / raw) To: dan.carpenter; +Cc: george.kennedy, kuba, dhaval.giani, netdev 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures 2020-07-14 21:03 ` David Miller @ 2020-07-14 21:34 ` George Kennedy 2020-07-14 21:37 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: George Kennedy @ 2020-07-14 21:34 UTC (permalink / raw) To: David Miller, dan.carpenter; +Cc: kuba, dhaval.giani, netdev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures 2020-07-14 21:34 ` George Kennedy @ 2020-07-14 21:37 ` David Miller 2020-07-15 14:01 ` George Kennedy 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2020-07-14 21:37 UTC (permalink / raw) To: george.kennedy; +Cc: dan.carpenter, kuba, dhaval.giani, netdev 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures 2020-07-14 21:37 ` David Miller @ 2020-07-15 14:01 ` George Kennedy 0 siblings, 0 replies; 7+ messages in thread From: George Kennedy @ 2020-07-15 14:01 UTC (permalink / raw) To: David Miller; +Cc: dan.carpenter, kuba, dhaval.giani, netdev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-15 14:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-13 11:58 [PATCH 1/1] ax88172a: fix ax88172a_unbind() failures George Kennedy 2020-07-14 0:08 ` David Miller 2020-07-14 8:00 ` Dan Carpenter 2020-07-14 21:03 ` David Miller 2020-07-14 21:34 ` George Kennedy 2020-07-14 21:37 ` David Miller 2020-07-15 14:01 ` George Kennedy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).