diff mbox series

i2c: Replace all non-returning strlcpy with strscpy

Message ID 20230523021150.2406032-1-azeemshaikh38@gmail.com
State New
Headers show
Series i2c: Replace all non-returning strlcpy with strscpy | expand

Commit Message

Azeem Shaikh May 23, 2023, 2:11 a.m. UTC
strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
 drivers/leds/flash/leds-as3645a.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Lee Jones May 23, 2023, 9:05 a.m. UTC | #1
On Tue, 23 May 2023, Azeem Shaikh wrote:

> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> No return values were used, so direct replacement is safe.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
>  drivers/leds/flash/leds-as3645a.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Please resubmit, taking the time to check the subject line please.

> diff --git a/drivers/leds/flash/leds-as3645a.c b/drivers/leds/flash/leds-as3645a.c
> index bb2249771acb..7dc574b18f5f 100644
> --- a/drivers/leds/flash/leds-as3645a.c
> +++ b/drivers/leds/flash/leds-as3645a.c
> @@ -651,8 +651,8 @@ static int as3645a_v4l2_setup(struct as3645a *flash)
>  		},
>  	};
>  
> -	strlcpy(cfg.dev_name, led->dev->kobj.name, sizeof(cfg.dev_name));
> -	strlcpy(cfgind.dev_name, flash->iled_cdev.dev->kobj.name,
> +	strscpy(cfg.dev_name, led->dev->kobj.name, sizeof(cfg.dev_name));
> +	strscpy(cfgind.dev_name, flash->iled_cdev.dev->kobj.name,
>  		sizeof(cfgind.dev_name));
>  
>  	flash->vf = v4l2_flash_init(
>
Sakari Ailus May 23, 2023, 9:13 a.m. UTC | #2
Hi Lee, Azeem,

On Tue, May 23, 2023 at 10:05:40AM +0100, Lee Jones wrote:
> On Tue, 23 May 2023, Azeem Shaikh wrote:
> 
> > strlcpy() reads the entire source buffer first.
> > This read may exceed the destination size limit.
> > This is both inefficient and can lead to linear read
> > overflows if a source string is not NUL-terminated [1].
> > In an effort to remove strlcpy() completely [2], replace
> > strlcpy() here with strscpy().
> > No return values were used, so direct replacement is safe.
> > 
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > [2] https://github.com/KSPP/linux/issues/89
> > 
> > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> > ---
> >  drivers/leds/flash/leds-as3645a.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Please resubmit, taking the time to check the subject line please.

I'd say also shorter description will suffice. Nowadays people understand
the motivation replacing strlcpy() by strscpy() without too much
elaboration. Lines may be up to 74 characters long, too, and period isn't
automatically followed by a newline.

The patch itself seems fine.

I also prefer my @linux.intel.com address, as in MAINTAINERS for this
driver.
Sakari Ailus May 24, 2023, 12:15 p.m. UTC | #3
Hi Azeema,

On Tue, May 23, 2023 at 10:34:34AM -0400, Azeem Shaikh wrote:
> Thanks for the quick response Lee and Sakari.
> 
> On Tue, May 23, 2023 at 5:13 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Lee, Azeem,
> >
> > On Tue, May 23, 2023 at 10:05:40AM +0100, Lee Jones wrote:
> > > On Tue, 23 May 2023, Azeem Shaikh wrote:
> > >
> > > > strlcpy() reads the entire source buffer first.
> > > > This read may exceed the destination size limit.
> > > > This is both inefficient and can lead to linear read
> > > > overflows if a source string is not NUL-terminated [1].
> > > > In an effort to remove strlcpy() completely [2], replace
> > > > strlcpy() here with strscpy().
> > > > No return values were used, so direct replacement is safe.
> > > >
> > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > > > [2] https://github.com/KSPP/linux/issues/89
> > > >
> > > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> > > > ---
> > > >  drivers/leds/flash/leds-as3645a.c |    4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > Please resubmit, taking the time to check the subject line please.
> >
> > I'd say also shorter description will suffice. Nowadays people understand
> > the motivation replacing strlcpy() by strscpy() without too much
> > elaboration. Lines may be up to 74 characters long, too, and period isn't
> > automatically followed by a newline.
> >
> 
> Let me know if this commit log looks good to you both and I'll send over a v2.
> 
> Subject: [PATCH] leds: as3645a: Replace all non-returning strlcpy with strscpy

All instances are replaced, so you can drop "all non-returning ".

> 
> Part of a tree-wide effort to remove deprecated strlcpy()[1] and replace
> it with strscpy()[2]. No return values were used, so direct replacement
> is safe.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89

Looks good to me.

> 
> > I also prefer my @linux.intel.com address, as in MAINTAINERS for this
> > driver.
> 
> Fyi that the email address mentioned for this driver is not the
> @linux.intel.com -
> https://github.com/torvalds/linux/blob/44c026a73be8038f03dbdeef028b642880cf1511/MAINTAINERS#L3070.
> I'm happy to send the v2 patch to sakari.ailus@linux.intel.com if you
> prefer that instead.

Oops, my mistake then. :-) I thought I already had changed this. Oh well.
diff mbox series

Patch

diff --git a/drivers/leds/flash/leds-as3645a.c b/drivers/leds/flash/leds-as3645a.c
index bb2249771acb..7dc574b18f5f 100644
--- a/drivers/leds/flash/leds-as3645a.c
+++ b/drivers/leds/flash/leds-as3645a.c
@@ -651,8 +651,8 @@  static int as3645a_v4l2_setup(struct as3645a *flash)
 		},
 	};
 
-	strlcpy(cfg.dev_name, led->dev->kobj.name, sizeof(cfg.dev_name));
-	strlcpy(cfgind.dev_name, flash->iled_cdev.dev->kobj.name,
+	strscpy(cfg.dev_name, led->dev->kobj.name, sizeof(cfg.dev_name));
+	strscpy(cfgind.dev_name, flash->iled_cdev.dev->kobj.name,
 		sizeof(cfgind.dev_name));
 
 	flash->vf = v4l2_flash_init(