can: bcm: Replace strcpy() with strscpy(), simplify call

[PATCH] can: bcm: Replace strcpy() with strscpy(), simplify call

Posted by Ethan Carter Edwards 1 month, 4 weeks ago

strcpy() is deprecated; use the safer strscpy() instead.

While the src is char array at bcm_proc_getifname's call sites, the src
is passed to the function as a char*. Including the IFNAMSIZ length
makes the call safer.

Use turnary over if/else to simplify code.

Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
---
 net/can/bcm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index a4bef2c48a55..c133dab1202e 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -196,10 +196,7 @@ static char *bcm_proc_getifname(struct net *net, char *result, int ifindex)
 
 	rcu_read_lock();
 	dev = dev_get_by_index_rcu(net, ifindex);
-	if (dev)
-		strcpy(result, dev->name);
-	else
-		strcpy(result, "???");
+	strscpy(result, dev ? dev->name : "???", IFNAMSIZ);
 	rcu_read_unlock();
 
 	return result;

---
base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
change-id: 20260418-can-bcm-strscpy-56a0e402a660

Best regards,
-- 
Ethan Carter Edwards <ethan@ethancedwards.com>

Re: [PATCH] can: bcm: Replace strcpy() with strscpy(), simplify call

Posted by Oliver Hartkopp 1 month, 3 weeks ago

Hello Ethan,

On 19.04.26 04:00, Ethan Carter Edwards wrote:
> strcpy() is deprecated; use the safer strscpy() instead.
> 
> While the src is char array at bcm_proc_getifname's call sites, the src
> is passed to the function as a char*. Including the IFNAMSIZ length
> makes the call safer.

I'm not sure what makes the call "safer" here.

The use of IFNAMSIZ in dev->name and in ifname[IFNAMSIZ] in 
bcm_proc_show() makes it 100% clear that we only can copy data between 
identical sized buffers. Adding strscpy() with IFNAMSIZ is a nop.

The only issue could be that someone reduces IFNAMSIZ to a value smaller 
than sizeof("???") - which is not addressed here.

I see no real benefit changing this code in the proposed way.

Especially as we are not talking about some dynamic content here.

Best regards,
Oliver

> 
> Use turnary over if/else to simplify code.
> 
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
> ---
>   net/can/bcm.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a55..c133dab1202e 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -196,10 +196,7 @@ static char *bcm_proc_getifname(struct net *net, char *result, int ifindex)
>   
>   	rcu_read_lock();
>   	dev = dev_get_by_index_rcu(net, ifindex);
> -	if (dev)
> -		strcpy(result, dev->name);
> -	else
> -		strcpy(result, "???");
> +	strscpy(result, dev ? dev->name : "???", IFNAMSIZ);
>   	rcu_read_unlock();
>   
>   	return result;
> 
> ---
> base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
> change-id: 20260418-can-bcm-strscpy-56a0e402a660
> 
> Best regards,

Re: [PATCH] can: bcm: Replace strcpy() with strscpy(), simplify call

Posted by Ethan Carter Edwards 1 month, 4 weeks ago

On 26/04/18 10:00PM, Ethan Carter Edwards wrote:
> strcpy() is deprecated; use the safer strscpy() instead.
> 
> While the src is char array at bcm_proc_getifname's call sites, the src
> is passed to the function as a char*. Including the IFNAMSIZ length
> makes the call safer.
> 
> Use turnary over if/else to simplify code.
> 
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
> ---
>  net/can/bcm.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a55..c133dab1202e 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -196,10 +196,7 @@ static char *bcm_proc_getifname(struct net *net, char *result, int ifindex)
>  
>  	rcu_read_lock();
>  	dev = dev_get_by_index_rcu(net, ifindex);
> -	if (dev)
> -		strcpy(result, dev->name);
> -	else
> -		strcpy(result, "???");
> +	strscpy(result, dev ? dev->name : "???", IFNAMSIZ);

On second thought:

would it just be better here to do:

if (dev)
    strscpy(result, dev->name, IFNAMSIZ);
else
    memcpy(result, "???\0", 4);

for the sake of compiler optimizations/speed?

reading this: https://lore.kernel.org/lkml/20251023092046.4f556e0f@pumpkin/

I'm not sure.

>  	rcu_read_unlock();
>  
>  	return result;
> 
> ---
> base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
> change-id: 20260418-can-bcm-strscpy-56a0e402a660
> 
> Best regards,
> -- 
> Ethan Carter Edwards <ethan@ethancedwards.com>
>