| Message ID | 20220120130605.55741-1-dzm91@hust.edu.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | drivers: net: remove a dangling pointer in peak_usb_create_dev | expand |
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
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
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
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
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
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
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
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
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: