* [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev @ 2022-01-20 13:05 Dongliang Mu 2022-01-20 14:27 ` Pavel Skripkin 0 siblings, 1 reply; 9+ messages in thread From: Dongliang Mu @ 2022-01-20 13:05 UTC (permalink / raw) To: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Pavel Skripkin, Vincent Mailhol Cc: Dongliang Mu, linux-can, netdev, linux-kernel From: Dongliang Mu <mudongliangabcd@gmail.com> The error handling code of peak_usb_create_dev forgets to reset the next_siblings of previous entry. Fix this by nullifying the (dev->prev_siblings)->next_siblings in the error handling code. Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/net/can/usb/peak_usb/pcan_usb_core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c index b850ff8fe4bd..f858810221b6 100644 --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c @@ -894,6 +894,9 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter, dev->adapter->dev_free(dev); lbl_unregister_candev: + /* remove the dangling pointer in next_siblings */ + if (dev->prev_siblings) + (dev->prev_siblings)->next_siblings = NULL; unregister_candev(netdev); lbl_restore_intf_data: -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-20 13:05 [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev Dongliang Mu @ 2022-01-20 14:27 ` Pavel Skripkin 2022-01-21 0:09 ` Dongliang Mu 0 siblings, 1 reply; 9+ messages in thread From: Pavel Skripkin @ 2022-01-20 14:27 UTC (permalink / raw) To: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol Cc: Dongliang Mu, linux-can, netdev, linux-kernel Hi Dongliang, On 1/20/22 16:05, Dongliang Mu wrote: > From: Dongliang Mu <mudongliangabcd@gmail.com> > > The error handling code of peak_usb_create_dev forgets to reset the > next_siblings of previous entry. > > Fix this by nullifying the (dev->prev_siblings)->next_siblings in the > error handling code. > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/can/usb/peak_usb/pcan_usb_core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > index b850ff8fe4bd..f858810221b6 100644 > --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > @@ -894,6 +894,9 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter, > dev->adapter->dev_free(dev); > > lbl_unregister_candev: > + /* remove the dangling pointer in next_siblings */ > + if (dev->prev_siblings) > + (dev->prev_siblings)->next_siblings = NULL; > unregister_candev(netdev); > > lbl_restore_intf_data: Is this pointer used somewhere? I see, that couple of struct peak_usb_adapter::dev_free() functions use it, but peak_usb_disconnect() sets dev->next_siblings to NULL before calling ->dev_free(). Do you have a calltrace or oops log? With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-20 14:27 ` Pavel Skripkin @ 2022-01-21 0:09 ` Dongliang Mu 2022-01-21 3:36 ` Dongliang Mu 0 siblings, 1 reply; 9+ messages in thread From: Dongliang Mu @ 2022-01-21 0:09 UTC (permalink / raw) To: Pavel Skripkin Cc: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol, linux-can, open list:NETWORKING [GENERAL], linux-kernel On Thu, Jan 20, 2022 at 10:27 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > Hi Dongliang, > > On 1/20/22 16:05, Dongliang Mu wrote: > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > The error handling code of peak_usb_create_dev forgets to reset the > > next_siblings of previous entry. > > > > Fix this by nullifying the (dev->prev_siblings)->next_siblings in the > > error handling code. > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/net/can/usb/peak_usb/pcan_usb_core.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > index b850ff8fe4bd..f858810221b6 100644 > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > @@ -894,6 +894,9 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter, > > dev->adapter->dev_free(dev); > > > > lbl_unregister_candev: > > + /* remove the dangling pointer in next_siblings */ > > + if (dev->prev_siblings) > > + (dev->prev_siblings)->next_siblings = NULL; > > unregister_candev(netdev); > > > > lbl_restore_intf_data: > > > Is this pointer used somewhere? I see, that couple of > struct peak_usb_adapter::dev_free() functions use it, but > peak_usb_disconnect() sets dev->next_siblings to NULL before calling > ->dev_free(). > > Do you have a calltrace or oops log? Hi Pavel, I have no calltrace or log since this dangling pointer may not be dereferenced in the following code. But I am not sure. So the commit title of this patch is "remove a dangling pointer in peak_usb_create_dev". > > > > > With regards, > Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-21 0:09 ` Dongliang Mu @ 2022-01-21 3:36 ` Dongliang Mu 2022-01-21 5:58 ` Dongliang Mu 0 siblings, 1 reply; 9+ messages in thread From: Dongliang Mu @ 2022-01-21 3:36 UTC (permalink / raw) To: Pavel Skripkin Cc: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol, linux-can, open list:NETWORKING [GENERAL], linux-kernel On Fri, Jan 21, 2022 at 8:09 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > On Thu, Jan 20, 2022 at 10:27 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > > > Hi Dongliang, > > > > On 1/20/22 16:05, Dongliang Mu wrote: > > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > > The error handling code of peak_usb_create_dev forgets to reset the > > > next_siblings of previous entry. > > > > > > Fix this by nullifying the (dev->prev_siblings)->next_siblings in the > > > error handling code. > > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > drivers/net/can/usb/peak_usb/pcan_usb_core.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > > index b850ff8fe4bd..f858810221b6 100644 > > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > > @@ -894,6 +894,9 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter, > > > dev->adapter->dev_free(dev); > > > > > > lbl_unregister_candev: > > > + /* remove the dangling pointer in next_siblings */ > > > + if (dev->prev_siblings) > > > + (dev->prev_siblings)->next_siblings = NULL; > > > unregister_candev(netdev); > > > > > > lbl_restore_intf_data: > > > > > > Is this pointer used somewhere? I see, that couple of > > struct peak_usb_adapter::dev_free() functions use it, but > > peak_usb_disconnect() sets dev->next_siblings to NULL before calling > > ->dev_free(). > > > > Do you have a calltrace or oops log? > > Hi Pavel, > > I have no calltrace or log since this dangling pointer may not be > dereferenced in the following code. But I am not sure. So the commit > title of this patch is "remove a dangling pointer in > peak_usb_create_dev". BTW, as you mentioned, dev->next_siblings is used in struct peak_usb_adapter::dev_free() (i.e., pcan_usb_fd_free or pcan_usb_pro_free), how about the following path? peak_usb_probe -> peak_usb_create_dev (goto adap_dev_free;) -> dev->adapter->dev_free() -> pcan_usb_fd_free or pcan_usb_pro_free (This function uses next_siblings as condition elements) static void pcan_usb_fd_free(struct peak_usb_device *dev) { /* last device: can free shared objects now */ if (!dev->prev_siblings && !dev->next_siblings) { struct pcan_usb_fd_device *pdev = container_of(dev, struct pcan_usb_fd_device, dev); /* free commands buffer */ kfree(pdev->cmd_buffer_addr); /* free usb interface object */ kfree(pdev->usb_if); } } If next_siblings is not NULL, will it lead to the missing free of cmd_buffer_addr and usb_if? Please let me know if I made any mistakes. > > > > > > > > > > With regards, > > Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-21 3:36 ` Dongliang Mu @ 2022-01-21 5:58 ` Dongliang Mu 2022-01-21 19:36 ` Pavel Skripkin 0 siblings, 1 reply; 9+ messages in thread From: Dongliang Mu @ 2022-01-21 5:58 UTC (permalink / raw) To: Pavel Skripkin Cc: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol, linux-can, open list:NETWORKING [GENERAL], linux-kernel On Fri, Jan 21, 2022 at 11:36 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > On Fri, Jan 21, 2022 at 8:09 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > On Thu, Jan 20, 2022 at 10:27 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > > > > > Hi Dongliang, > > > > > > On 1/20/22 16:05, Dongliang Mu wrote: > > > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > > > > The error handling code of peak_usb_create_dev forgets to reset the > > > > next_siblings of previous entry. > > > > > > > > Fix this by nullifying the (dev->prev_siblings)->next_siblings in the > > > > error handling code. > > > > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > --- > > > > drivers/net/can/usb/peak_usb/pcan_usb_core.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > > > index b850ff8fe4bd..f858810221b6 100644 > > > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > > > > @@ -894,6 +894,9 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter, > > > > dev->adapter->dev_free(dev); > > > > > > > > lbl_unregister_candev: > > > > + /* remove the dangling pointer in next_siblings */ > > > > + if (dev->prev_siblings) > > > > + (dev->prev_siblings)->next_siblings = NULL; > > > > unregister_candev(netdev); > > > > > > > > lbl_restore_intf_data: > > > > > > > > > Is this pointer used somewhere? I see, that couple of > > > struct peak_usb_adapter::dev_free() functions use it, but > > > peak_usb_disconnect() sets dev->next_siblings to NULL before calling > > > ->dev_free(). > > > > > > Do you have a calltrace or oops log? > > > > Hi Pavel, > > > > I have no calltrace or log since this dangling pointer may not be > > dereferenced in the following code. But I am not sure. So the commit > > title of this patch is "remove a dangling pointer in > > peak_usb_create_dev". > > BTW, as you mentioned, dev->next_siblings is used in struct > peak_usb_adapter::dev_free() (i.e., pcan_usb_fd_free or > pcan_usb_pro_free), how about the following path? > > peak_usb_probe > -> peak_usb_create_dev (goto adap_dev_free;) > -> dev->adapter->dev_free() > -> pcan_usb_fd_free or pcan_usb_pro_free (This function uses > next_siblings as condition elements) > > static void pcan_usb_fd_free(struct peak_usb_device *dev) > { > /* last device: can free shared objects now */ > if (!dev->prev_siblings && !dev->next_siblings) { > struct pcan_usb_fd_device *pdev = > container_of(dev, struct pcan_usb_fd_device, dev); > > /* free commands buffer */ > kfree(pdev->cmd_buffer_addr); > > /* free usb interface object */ > kfree(pdev->usb_if); > } > } > > If next_siblings is not NULL, will it lead to the missing free of > cmd_buffer_addr and usb_if? The answer is No. Forget my silly thought. > > Please let me know if I made any mistakes. > > > > > > > > > > > > > > > > With regards, > > > Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-21 5:58 ` Dongliang Mu @ 2022-01-21 19:36 ` Pavel Skripkin 2022-01-22 6:45 ` Dongliang Mu 0 siblings, 1 reply; 9+ messages in thread From: Pavel Skripkin @ 2022-01-21 19:36 UTC (permalink / raw) To: Dongliang Mu Cc: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol, linux-can, open list:NETWORKING [GENERAL], linux-kernel Hi Dongliang, On 1/21/22 08:58, Dongliang Mu wrote: [...]>> BTW, as you mentioned, dev->next_siblings is used in struct >> peak_usb_adapter::dev_free() (i.e., pcan_usb_fd_free or >> pcan_usb_pro_free), how about the following path? >> >> peak_usb_probe >> -> peak_usb_create_dev (goto adap_dev_free;) >> -> dev->adapter->dev_free() >> -> pcan_usb_fd_free or pcan_usb_pro_free (This function uses >> next_siblings as condition elements) >> >> static void pcan_usb_fd_free(struct peak_usb_device *dev) >> { >> /* last device: can free shared objects now */ >> if (!dev->prev_siblings && !dev->next_siblings) { >> struct pcan_usb_fd_device *pdev = >> container_of(dev, struct pcan_usb_fd_device, dev); >> >> /* free commands buffer */ >> kfree(pdev->cmd_buffer_addr); >> >> /* free usb interface object */ >> kfree(pdev->usb_if); >> } >> } >> >> If next_siblings is not NULL, will it lead to the missing free of >> cmd_buffer_addr and usb_if? > > The answer is No. Forget my silly thought. > Yeah, it seems like (at least based on code), that this dangling pointer is not dangerous, since nothing accesses it. And next_siblings _guaranteed_ to be NULL, since dev->next_siblings is set NULL in disconnect() With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-21 19:36 ` Pavel Skripkin @ 2022-01-22 6:45 ` Dongliang Mu 2022-01-23 13:48 ` Pavel Skripkin 0 siblings, 1 reply; 9+ messages in thread From: Dongliang Mu @ 2022-01-22 6:45 UTC (permalink / raw) To: Pavel Skripkin Cc: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol, linux-can, open list:NETWORKING [GENERAL], linux-kernel On Sat, Jan 22, 2022 at 3:36 AM Pavel Skripkin <paskripkin@gmail.com> wrote: > > Hi Dongliang, > > On 1/21/22 08:58, Dongliang Mu wrote: > [...]>> BTW, as you mentioned, dev->next_siblings is used in struct > >> peak_usb_adapter::dev_free() (i.e., pcan_usb_fd_free or > >> pcan_usb_pro_free), how about the following path? > >> > >> peak_usb_probe > >> -> peak_usb_create_dev (goto adap_dev_free;) > >> -> dev->adapter->dev_free() > >> -> pcan_usb_fd_free or pcan_usb_pro_free (This function uses > >> next_siblings as condition elements) > >> > >> static void pcan_usb_fd_free(struct peak_usb_device *dev) > >> { > >> /* last device: can free shared objects now */ > >> if (!dev->prev_siblings && !dev->next_siblings) { > >> struct pcan_usb_fd_device *pdev = > >> container_of(dev, struct pcan_usb_fd_device, dev); > >> > >> /* free commands buffer */ > >> kfree(pdev->cmd_buffer_addr); > >> > >> /* free usb interface object */ > >> kfree(pdev->usb_if); > >> } > >> } > >> > >> If next_siblings is not NULL, will it lead to the missing free of > >> cmd_buffer_addr and usb_if? > > > > The answer is No. Forget my silly thought. > > > > Yeah, it seems like (at least based on code), that this dangling pointer > is not dangerous, since nothing accesses it. And next_siblings > _guaranteed_ to be NULL, since dev->next_siblings is set NULL in > disconnect() Yes, you're right. As a security researcher, I am sensitive to such dangling pointers. As its nullifying site is across functions, I suggest developers remove this dangling pointer in case that any newly added code in this function or before the nullifying location would touch next_siblings. If Pavel and others think it's fine, then it's time to close this patch. > > > > > With regards, > Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-22 6:45 ` Dongliang Mu @ 2022-01-23 13:48 ` Pavel Skripkin 2022-01-24 6:04 ` Dongliang Mu 0 siblings, 1 reply; 9+ messages in thread From: Pavel Skripkin @ 2022-01-23 13:48 UTC (permalink / raw) To: Dongliang Mu Cc: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol, linux-can, open list:NETWORKING [GENERAL], linux-kernel Hi Dongliang, On 1/22/22 09:45, Dongliang Mu wrote: [...] >> Yeah, it seems like (at least based on code), that this dangling pointer >> is not dangerous, since nothing accesses it. And next_siblings >> _guaranteed_ to be NULL, since dev->next_siblings is set NULL in >> disconnect() > > Yes, you're right. As a security researcher, I am sensitive to such > dangling pointers. > > As its nullifying site is across functions, I suggest developers > remove this dangling pointer in case that any newly added code in this > function or before the nullifying location would touch next_siblings. > Based on git blame this driver is very old (was added in 2012), so, I guess, nothing really new will come up. Anyway, I am absolutely not a security person and if you think, that this dangling pointer can be somehow used in exploitation you should state it in commit message. > If Pavel and others think it's fine, then it's time to close this patch. > I don't have any big objections on the code itself. Maybe only 'if' can be removed to just speed up the code, but I don't see why this change is needed :) With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev 2022-01-23 13:48 ` Pavel Skripkin @ 2022-01-24 6:04 ` Dongliang Mu 0 siblings, 0 replies; 9+ messages in thread From: Dongliang Mu @ 2022-01-24 6:04 UTC (permalink / raw) To: Pavel Skripkin Cc: Dongliang Mu, Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller, Jakub Kicinski, Stephane Grosjean, Stefan Mätje, Vincent Mailhol, linux-can, open list:NETWORKING [GENERAL], linux-kernel On Sun, Jan 23, 2022 at 9:48 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > Hi Dongliang, > > On 1/22/22 09:45, Dongliang Mu wrote: > [...] > > >> Yeah, it seems like (at least based on code), that this dangling pointer > >> is not dangerous, since nothing accesses it. And next_siblings > >> _guaranteed_ to be NULL, since dev->next_siblings is set NULL in > >> disconnect() > > > > Yes, you're right. As a security researcher, I am sensitive to such > > dangling pointers. > > > > As its nullifying site is across functions, I suggest developers > > remove this dangling pointer in case that any newly added code in this > > function or before the nullifying location would touch next_siblings. > > > > Based on git blame this driver is very old (was added in 2012), so, I > guess, nothing really new will come up. > > Anyway, I am absolutely not a security person and if you think, that > this dangling pointer can be somehow used in exploitation you should > state it in commit message. > > > > If Pavel and others think it's fine, then it's time to close this patch. > > > > I don't have any big objections on the code itself. Maybe only 'if' can > be removed to just speed up the code, but I don't see why this change is > needed :) OK, let's move on. Leave alone this patch. > > > > > With regards, > Pavel Skripkin ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-24 6:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-20 13:05 [PATCH] drivers: net: remove a dangling pointer in peak_usb_create_dev Dongliang Mu 2022-01-20 14:27 ` Pavel Skripkin 2022-01-21 0:09 ` Dongliang Mu 2022-01-21 3:36 ` Dongliang Mu 2022-01-21 5:58 ` Dongliang Mu 2022-01-21 19:36 ` Pavel Skripkin 2022-01-22 6:45 ` Dongliang Mu 2022-01-23 13:48 ` Pavel Skripkin 2022-01-24 6:04 ` Dongliang Mu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.