Thread (8 messages) 8 messages, 4 authors, 2014-09-16

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.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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help