* Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
2014-09-14 17:31 [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy Rickard Strandqvist
@ 2014-09-15 8:01 ` Stephane Grosjean
2014-09-15 8:28 ` David Laight
2014-09-16 14:09 ` Marc Kleine-Budde
2 siblings, 0 replies; 8+ messages in thread
From: Stephane Grosjean @ 2014-09-15 8:01 UTC (permalink / raw)
To: Rickard Strandqvist, Wolfgang Grandegger, Marc Kleine-Budde
Cc: Oliver Hartkopp, Alexey Khoroshilov, Christopher R. Baker,
linux-can, netdev, linux-kernel
Acked-by: Stephane Grosjean <s.grosjean@peak-system.com>
Le 14/09/2014 19:31, Rickard Strandqvist a écrit :
> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
> drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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);
--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com
--
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
2014-09-14 17:31 [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy Rickard Strandqvist
2014-09-15 8:01 ` Stephane Grosjean
@ 2014-09-15 8:28 ` David Laight
2014-09-15 8:42 ` Marc Kleine-Budde
2014-09-16 14:09 ` Marc Kleine-Budde
2 siblings, 1 reply; 8+ messages in thread
From: David Laight @ 2014-09-15 8:28 UTC (permalink / raw)
To: 'Rickard Strandqvist', Wolfgang Grandegger,
Marc Kleine-Budde
Cc: Oliver Hartkopp, Stephane Grosjean, Alexey Khoroshilov,
Christopher R. Baker, linux-can@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
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);
David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
2014-09-15 8:28 ` David Laight
@ 2014-09-15 8:42 ` Marc Kleine-Budde
2014-09-15 8:47 ` David Laight
0 siblings, 1 reply; 8+ messages in thread
From: Marc Kleine-Budde @ 2014-09-15 8:42 UTC (permalink / raw)
To: David Laight, 'Rickard Strandqvist', Wolfgang Grandegger
Cc: Oliver Hartkopp, Stephane Grosjean, Alexey Khoroshilov,
Christopher R. Baker, linux-can@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]
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 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()?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
2014-09-15 8:42 ` Marc Kleine-Budde
@ 2014-09-15 8:47 ` David Laight
2014-09-15 21:47 ` Rickard Strandqvist
0 siblings, 1 reply; 8+ messages in thread
From: David Laight @ 2014-09-15 8:47 UTC (permalink / raw)
To: 'Marc Kleine-Budde', 'Rickard Strandqvist',
Wolfgang Grandegger
Cc: Oliver Hartkopp, Stephane Grosjean, Alexey Khoroshilov,
Christopher R. Baker, linux-can@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
2014-09-15 8:47 ` David Laight
@ 2014-09-15 21:47 ` Rickard Strandqvist
2014-09-16 8:40 ` David Laight
0 siblings, 1 reply; 8+ messages in thread
From: Rickard Strandqvist @ 2014-09-15 21:47 UTC (permalink / raw)
To: David Laight
Cc: Marc Kleine-Budde, Wolfgang Grandegger, Oliver Hartkopp,
Stephane Grosjean, Alexey Khoroshilov, Christopher R. Baker,
linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
2014-09-15 10:47 GMT+02:00 David Laight <David.Laight@aculab.com>:
> 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
Hi all
I have been more and more careful so I did not introduce the type of
error that can arise by blindly replacing strncpy to strlcpy.
But this is one of the most apparent where there will not be a problem.
I liked the variant:
char name[sizeof(netdev->name)];
But dislike and do not understand what the point would be with memcpy variant.
Kind regards
Rickard Strandqvist
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
2014-09-15 21:47 ` Rickard Strandqvist
@ 2014-09-16 8:40 ` David Laight
0 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2014-09-16 8:40 UTC (permalink / raw)
To: 'Rickard Strandqvist'
Cc: Marc Kleine-Budde, Wolfgang Grandegger, Oliver Hartkopp,
Stephane Grosjean, Alexey Khoroshilov, Christopher R. Baker,
linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
From: Rickard Strandqvist
> > 2014-09-15 10:47 GMT+02:00 David Laight <David.Laight@aculab.com>:
...
> > Or:
> > char name[sizeof netdev->name];
> > memcpy(name, netdev->name, sizeof netdev->name);
> >
...
> I liked the variant:
> char name[sizeof(netdev->name)];
>
> But dislike and do not understand what the point would be with memcpy variant.
The memcpy() will be a lot faster than any of the string copy functions
because it doesn't need to worry about the terminator.
In many cases it will be inlined to a short series of load and stores.
David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy
2014-09-14 17:31 [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy Rickard Strandqvist
2014-09-15 8:01 ` Stephane Grosjean
2014-09-15 8:28 ` David Laight
@ 2014-09-16 14:09 ` Marc Kleine-Budde
2 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2014-09-16 14:09 UTC (permalink / raw)
To: Rickard Strandqvist, Wolfgang Grandegger
Cc: Oliver Hartkopp, Stephane Grosjean, Alexey Khoroshilov,
Christopher R. Baker, linux-can, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]
On 09/14/2014 07:31 PM, Rickard Strandqvist wrote:
> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
> drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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);
>
What about removing the dev_info(), the name array and the str?cpy()
completely?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread