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
