lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Sep 2014 08:47:18 +0000 From: David Laight <David.Laight@...LAB.COM> To: 'Marc Kleine-Budde' <mkl@...gutronix.de>, 'Rickard Strandqvist' <rickard_strandqvist@...ctrumdigital.se>, Wolfgang Grandegger <wg@...ndegger.com> CC: Oliver Hartkopp <socketcan@...tkopp.net>, Stephane Grosjean <s.grosjean@...k-system.com>, Alexey Khoroshilov <khoroshilov@...ras.ru>, "Christopher R. Baker" <cbaker@....ri.cmu.edu>, "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy From: Marc Kleine-Budde [ > On 09/15/2014 10:28 AM, David Laight wrote: > > From: Rickard Strandqvist > > ... > >> Replacing strncpy with strlcpy to avoid strings that lacks null terminate. > > ... > >> 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 644e6ab..d4fe8ac 100644 > >> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c > >> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c > >> @@ -830,7 +830,7 @@ static void peak_usb_disconnect(struct usb_interface *intf) > >> char name[IFNAMSIZ]; > >> > >> dev->state &= ~PCAN_USB_STATE_CONNECTED; > >> - strncpy(name, netdev->name, IFNAMSIZ); > >> + strlcpy(name, netdev->name, IFNAMSIZ); > >> > >> unregister_netdev(netdev); > >> free_candev(netdev); > > > > Or: > > char name[sizeof netdev->name]; > > memcpy(name, netdev->name, sizeof netdev->name); > > I would be "sizeof(foo)" in kernel coding style, But not in mine :-) sizeof is an operator, not a function, it's argument can be (type). > but let's have a look at the original code: > > struct net_device *netdev = dev->netdev; > char name[IFNAMSIZ]; > > dev->state &= ~PCAN_USB_STATE_CONNECTED; > strncpy(name, netdev->name, IFNAMSIZ); > > unregister_netdev(netdev); > free_candev(netdev); > > kfree(dev->cmd_buf); > dev->next_siblings = NULL; > if (dev->adapter->dev_free) > dev->adapter->dev_free(dev); > > dev_info(&intf->dev, "%s removed\n", name); > > I think it's save to use: > > dev_info(&intf->dev, "%s removed\n", netdev_name(dev->netdev)); > > instead of doing the str?cpy() in the first place. But why not use: > > netdev_info(dev->netdev, "removed\n"); > > Is the USB device information lost when using netdev_info()? My guess is it avoids a 'use after free' - but I'm not going to dig that far. Another issue with blindly replacing strncpy() with strlcpy() (which doesn't affect the above) is when copying status to userspace. David
Powered by blists - more mailing lists