[PATCH] staging: vme_user: Replace strcpy with strscpy

[PATCH] staging: vme_user: Replace strcpy with strscpy

[Jonathan Bergh]

Replace strcpy with strscpy as preferred by checkpatch in vme_fake.c to
prevent warnings.

Signed-off-by: Jonathan Bergh <bergh.jonathan@gmail.com>
---
 drivers/staging/vme_user/vme_fake.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
index 9bcb89a84d53..0cf5700e151f 100644
--- a/drivers/staging/vme_user/vme_fake.c
+++ b/drivers/staging/vme_user/vme_fake.c
@@ -1093,7 +1093,7 @@ static int __init fake_init(void)
 	tasklet_init(&fake_device->int_tasklet, fake_VIRQ_tasklet,
 			(unsigned long) fake_bridge);
 
-	strcpy(fake_bridge->name, driver_name);
+	strscpy(fake_bridge->name, driver_name, sizeof(fake_bridge->name));
 
 	/* Add master windows to list */
 	INIT_LIST_HEAD(&fake_bridge->master_resources);
-- 
2.34.1

Re: [PATCH] staging: vme_user: Replace strcpy with strscpy

[Greg KH]

On Sun, Sep 17, 2023 at 05:43:02PM +0200, Jonathan Bergh wrote:
> Replace strcpy with strscpy as preferred by checkpatch in vme_fake.c to
> prevent warnings.
> 
> Signed-off-by: Jonathan Bergh <bergh.jonathan@gmail.com>
> ---
>  drivers/staging/vme_user/vme_fake.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
> index 9bcb89a84d53..0cf5700e151f 100644
> --- a/drivers/staging/vme_user/vme_fake.c
> +++ b/drivers/staging/vme_user/vme_fake.c
> @@ -1093,7 +1093,7 @@ static int __init fake_init(void)
>  	tasklet_init(&fake_device->int_tasklet, fake_VIRQ_tasklet,
>  			(unsigned long) fake_bridge);
>  
> -	strcpy(fake_bridge->name, driver_name);
> +	strscpy(fake_bridge->name, driver_name, sizeof(fake_bridge->name));

If it were this easy, we would have swept the tree and done so, right?
Are you sure this is correct?  If so, please document exactly why it is
correct in the changelog text when you resend this.

thanks,

greg k-h

Re: [PATCH] staging: vme_user: Replace strcpy with strscpy

[Jonathan Bergh]

On Sun, Sep 17, 2023 at 06:24:35PM +0200, Greg KH wrote:
> On Sun, Sep 17, 2023 at 05:43:02PM +0200, Jonathan Bergh wrote:
> > Replace strcpy with strscpy as preferred by checkpatch in vme_fake.c to
> > prevent warnings.
> > 
> > Signed-off-by: Jonathan Bergh <bergh.jonathan@gmail.com>
> > ---
> >  drivers/staging/vme_user/vme_fake.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
> > index 9bcb89a84d53..0cf5700e151f 100644
> > --- a/drivers/staging/vme_user/vme_fake.c
> > +++ b/drivers/staging/vme_user/vme_fake.c
> > @@ -1093,7 +1093,7 @@ static int __init fake_init(void)
> >  	tasklet_init(&fake_device->int_tasklet, fake_VIRQ_tasklet,
> >  			(unsigned long) fake_bridge);
> >  
> > -	strcpy(fake_bridge->name, driver_name);
> > +	strscpy(fake_bridge->name, driver_name, sizeof(fake_bridge->name));
> 

Hi, thanks a lot for the review + comment.

> If it were this easy, we would have swept the tree and done so, right?
> Are you sure this is correct?  If so, please document exactly why it is
> correct in the changelog text when you resend this.

Thought it better to discuss before sending an updated patch
that might be wrong. 

I think it is correct since driver_name:
  * In: vme_fake.c:
	static const char driver_name[] = "vme_fake": 

will always fit into vme_bridge->name:
  * In: vme_bridge.h:
	...
	#define VMENAMSIZ 16
	...
	struct vme_bridge {
		...
		char name[VMENAMSIZ];
		...
	}

but I could have missed something. This is in the module __init
method and i dont see that name pointer being reassigned before
the strcpy call (which was changed).

Maybe its not worth changing, but (if it is correct) it would get
rid of the checkpatch warning and convert to the "preferred" API?

> 
> thanks,
> 
> greg k-h

FWIW Wouldnt it always be better to use the "safe" option since it 
at worst its going to truncate the destination string rather than 
write off the end of the array? 

thanks in advance
cheers
Jonathan

[PATCH] staging: vme_user: replace strcpy with strscpy

[Calvince Otieno]

Checkpatch suggests using strscpy() instead of strncpy().

The advantages of strscpy() are that it always adds a NUL terminator
and prevents read overflows if the source string is not properly
terminated. One potential disadvantage is that it doesn't zero pad the
string like strncpy() does.

In this code, strscpy() and strncpy() are equivalent and do not affect
runtime behavior. strscpy() simply copies the known string value of the
variable driver_name into the fake_bridge->name variable, which also
has a fixed size.

While using strscpy() does not address any bugs, it is considered a better
practice and aligns with checkpatch recommendations.

Signed-off-by: Calvince Otieno <calvncce@gmail.com>
---
 drivers/staging/vme_user/vme_fake.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
index 0e02c194298d..09b05861017a 100644
--- a/drivers/staging/vme_user/vme_fake.c
+++ b/drivers/staging/vme_user/vme_fake.c
@@ -67,6 +67,7 @@ struct fake_driver {
 	unsigned long long lm_base;
 	u32 lm_aspace;
 	u32 lm_cycle;
+
 	void (*lm_callback[4])(void *);
 	void *lm_data[4];
 	struct tasklet_struct int_tasklet;
@@ -1091,7 +1092,7 @@ static int __init fake_init(void)
 	tasklet_init(&fake_device->int_tasklet, fake_VIRQ_tasklet,
 		     (unsigned long)fake_bridge);
 
-	strcpy(fake_bridge->name, driver_name);
+	strscpy(fake_bridge->name, driver_name, sizeof(fake_bridge->name))
 
 	/* Add master windows to list */
 	INIT_LIST_HEAD(&fake_bridge->master_resources);
-- 
2.34.1

Re: [PATCH] staging: vme_user: replace strcpy with strscpy

[Greg Kroah-Hartman]

On Wed, Oct 18, 2023 at 10:29:51AM +0300, Calvince Otieno wrote:
> Checkpatch suggests using strscpy() instead of strncpy().
> 
> The advantages of strscpy() are that it always adds a NUL terminator
> and prevents read overflows if the source string is not properly
> terminated. One potential disadvantage is that it doesn't zero pad the
> string like strncpy() does.
> 
> In this code, strscpy() and strncpy() are equivalent and do not affect
> runtime behavior. strscpy() simply copies the known string value of the
> variable driver_name into the fake_bridge->name variable, which also
> has a fixed size.
> 
> While using strscpy() does not address any bugs, it is considered a better
> practice and aligns with checkpatch recommendations.
> 
> Signed-off-by: Calvince Otieno <calvncce@gmail.com>
> ---
>  drivers/staging/vme_user/vme_fake.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
> index 0e02c194298d..09b05861017a 100644
> --- a/drivers/staging/vme_user/vme_fake.c
> +++ b/drivers/staging/vme_user/vme_fake.c
> @@ -67,6 +67,7 @@ struct fake_driver {
>  	unsigned long long lm_base;
>  	u32 lm_aspace;
>  	u32 lm_cycle;
> +
>  	void (*lm_callback[4])(void *);
>  	void *lm_data[4];

Why did you make this extra line change?

>  	struct tasklet_struct int_tasklet;
> @@ -1091,7 +1092,7 @@ static int __init fake_init(void)
>  	tasklet_init(&fake_device->int_tasklet, fake_VIRQ_tasklet,
>  		     (unsigned long)fake_bridge);
>  
> -	strcpy(fake_bridge->name, driver_name);
> +	strscpy(fake_bridge->name, driver_name, sizeof(fake_bridge->name))

Are you sure this change is identical?  You need to document how you
have proved that.

thanks,

greg k-h

Re: [PATCH] staging: vme_user: replace strcpy with strscpy

[Dan Carpenter]

On Wed, Oct 18, 2023 at 10:29:51AM +0300, Calvince Otieno wrote:
> Checkpatch suggests using strscpy() instead of strncpy().
> 
> The advantages of strscpy() are that it always adds a NUL terminator
> and prevents read overflows if the source string is not properly
> terminated. One potential disadvantage is that it doesn't zero pad the
> string like strncpy() does.

You're not replacing strncpy(), you're replacing strcpy().  There is
never a downside to replacing strcpy() with strspy() beyond that the
secure function is probably slightly slower.

> 
> In this code, strscpy() and strncpy() are equivalent and do not affect
> runtime behavior. strscpy() simply copies the known string value of the
> variable driver_name into the fake_bridge->name variable, which also
> has a fixed size.
> 
> While using strscpy() does not address any bugs, it is considered a better
> practice and aligns with checkpatch recommendations.

This analysis does not say where driver_name is set, or how big it is,
or what the size of the fake_bridge->name buffer is.  I would like to
see that sort of analysis in the commit message.

regards,
dan carpenter