[PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[Rickard Strandqvist]

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);
-- 
1.7.10.4

Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[Stephane Grosjean]

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
--

RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[David Laight]

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

Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[Marc Kleine-Budde]

[-- 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 --]

RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[David Laight]

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

RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2316 bytes --]

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

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[Rickard Strandqvist]

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

RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[David Laight]

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

RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 725 bytes --]

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

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

[Marc Kleine-Budde]

[-- 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 --]