RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
From: David Laight <hidden>
Date: 2014-09-15 08:49:15
Also in:
linux-can, lkml
From: Marc Kleine-Budde [
On 09/15/2014 10:28 AM, David Laight wrote:quoted
From: Rickard Strandqvist ...quoted
Replacing strncpy with strlcpy to avoid strings that lacks null terminate....quoted
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.cb/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