LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
@ 2021-08-07 15:22 Len Baker
  2021-08-08 16:25 ` Jonathan Cameron
  0 siblings, 1 reply; 9+ messages in thread
From: Len Baker @ 2021-08-07 15:22 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen
  Cc: Len Baker, Kees Cook, linux-hardening, linux-iio, linux-kernel

strcpy() performs no bounds checking on the destination buffer. This
could result in linear overflows beyond the end of the buffer, leading
to all kinds of misbehaviors. The safe replacement is strscpy().

This patch is an effort to clean up the proliferation of str*()
functions in the kernel and a previous step in the path to remove
the strcpy function from the kernel entirely [1].

[1] https://github.com/KSPP/linux/issues/88

Signed-off-by: Len Baker <len.baker@gmx.com>
---
Changelog v1 -> v2
- Modify the commit changelog to inform that the motivation of this
  patch is to remove the strcpy() function from the kernel entirely
  (Jonathan Cameron).

 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
index f282e9cc34c5..3a6aa1c4bf6c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
@@ -264,6 +264,7 @@ int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st)
 	const char *orient;
 	char *str;
 	int i;
+	size_t n;

 	/* fill magnetometer orientation */
 	switch (st->chip_type) {
@@ -282,17 +283,18 @@ int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st)
 		for (i = 0; i < 3; ++i) {
 			orient = st->orientation.rotation[6 + i];
 			/* use length + 2 for adding minus sign if needed */
-			str = devm_kzalloc(regmap_get_device(st->map),
-					   strlen(orient) + 2, GFP_KERNEL);
+			n = strlen(orient) + 2;
+			str = devm_kzalloc(regmap_get_device(st->map), n,
+					   GFP_KERNEL);
 			if (str == NULL)
 				return -ENOMEM;
 			if (strcmp(orient, "0") == 0) {
-				strcpy(str, orient);
+				strscpy(str, orient, n);
 			} else if (orient[0] == '-') {
-				strcpy(str, &orient[1]);
+				strscpy(str, &orient[1], n);
 			} else {
 				str[0] = '-';
-				strcpy(&str[1], orient);
+				strscpy(&str[1], orient, n - 1);
 			}
 			st->magn_orient.rotation[6 + i] = str;
 		}
--
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-07 15:22 [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy() Len Baker
@ 2021-08-08 16:25 ` Jonathan Cameron
  2021-08-08 19:00   ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2021-08-08 16:25 UTC (permalink / raw)
  To: Len Baker
  Cc: Lars-Peter Clausen, Kees Cook, linux-hardening, linux-iio, linux-kernel

On Sat,  7 Aug 2021 17:22:25 +0200
Len Baker <len.baker@gmx.com> wrote:

> strcpy() performs no bounds checking on the destination buffer. This
> could result in linear overflows beyond the end of the buffer, leading
> to all kinds of misbehaviors. The safe replacement is strscpy().
> 
> This patch is an effort to clean up the proliferation of str*()
> functions in the kernel and a previous step in the path to remove
> the strcpy function from the kernel entirely [1].
> 
> [1] https://github.com/KSPP/linux/issues/88
> 
> Signed-off-by: Len Baker <len.baker@gmx.com>
Applied to the togreg branch of iio.git and pushed out as testing
so 0-day can poke at it and see if we missed anything.

Thanks,

Jonathan

> ---
> Changelog v1 -> v2
> - Modify the commit changelog to inform that the motivation of this
>   patch is to remove the strcpy() function from the kernel entirely
>   (Jonathan Cameron).
> 
>  drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
> index f282e9cc34c5..3a6aa1c4bf6c 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
> @@ -264,6 +264,7 @@ int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st)
>  	const char *orient;
>  	char *str;
>  	int i;
> +	size_t n;
> 
>  	/* fill magnetometer orientation */
>  	switch (st->chip_type) {
> @@ -282,17 +283,18 @@ int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st)
>  		for (i = 0; i < 3; ++i) {
>  			orient = st->orientation.rotation[6 + i];
>  			/* use length + 2 for adding minus sign if needed */
> -			str = devm_kzalloc(regmap_get_device(st->map),
> -					   strlen(orient) + 2, GFP_KERNEL);
> +			n = strlen(orient) + 2;
> +			str = devm_kzalloc(regmap_get_device(st->map), n,
> +					   GFP_KERNEL);
>  			if (str == NULL)
>  				return -ENOMEM;
>  			if (strcmp(orient, "0") == 0) {
> -				strcpy(str, orient);
> +				strscpy(str, orient, n);
>  			} else if (orient[0] == '-') {
> -				strcpy(str, &orient[1]);
> +				strscpy(str, &orient[1], n);
>  			} else {
>  				str[0] = '-';
> -				strcpy(&str[1], orient);
> +				strscpy(&str[1], orient, n - 1);
>  			}
>  			st->magn_orient.rotation[6 + i] = str;
>  		}
> --
> 2.25.1
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-08 16:25 ` Jonathan Cameron
@ 2021-08-08 19:00   ` Andy Shevchenko
  2021-08-09  9:21     ` Jonathan Cameron
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-08-08 19:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Len Baker, Lars-Peter Clausen, Kees Cook, linux-hardening,
	linux-iio, Linux Kernel Mailing List

On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat,  7 Aug 2021 17:22:25 +0200
> Len Baker <len.baker@gmx.com> wrote:
>
> > strcpy() performs no bounds checking on the destination buffer. This
> > could result in linear overflows beyond the end of the buffer, leading
> > to all kinds of misbehaviors. The safe replacement is strscpy().
> >
> > This patch is an effort to clean up the proliferation of str*()
> > functions in the kernel and a previous step in the path to remove
> > the strcpy function from the kernel entirely [1].
> >
> > [1] https://github.com/KSPP/linux/issues/88
> >
> > Signed-off-by: Len Baker <len.baker@gmx.com>
> Applied to the togreg branch of iio.git and pushed out as testing
> so 0-day can poke at it and see if we missed anything.

Isn't it too early? Or am I missing something (see below)?

...

> >                       /* use length + 2 for adding minus sign if needed */
> > -                     str = devm_kzalloc(regmap_get_device(st->map),
> > -                                        strlen(orient) + 2, GFP_KERNEL);
> > +                     n = strlen(orient) + 2;
> > +                     str = devm_kzalloc(regmap_get_device(st->map), n,
> > +                                        GFP_KERNEL);
> >                       if (str == NULL)
> >                               return -ENOMEM;
> >                       if (strcmp(orient, "0") == 0) {
> > -                             strcpy(str, orient);
> > +                             strscpy(str, orient, n);
> >                       } else if (orient[0] == '-') {
> > -                             strcpy(str, &orient[1]);
> > +                             strscpy(str, &orient[1], n);
> >                       } else {
> >                               str[0] = '-';
> > -                             strcpy(&str[1], orient);
> > +                             strscpy(&str[1], orient, n - 1);

Why n-1?

> >                       }

As far as I understood the logic, it  inverts the sign except the case
when it equals 0.

I have a question here, why can't we always use +/-?
Why can't 0 be prefixed with a sign?

If the above can be used, we may simplify this code.

Len, I think this task may be considered simple, but I recommend
thinking about each case and finding a way to simplify it more.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-08 19:00   ` Andy Shevchenko
@ 2021-08-09  9:21     ` Jonathan Cameron
  2021-08-09 16:14       ` Len Baker
  2021-08-10  8:30       ` David Laight
  0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Cameron @ 2021-08-09  9:21 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Len Baker, Lars-Peter Clausen, Kees Cook,
	linux-hardening, linux-iio, Linux Kernel Mailing List

On Sun, 8 Aug 2021 22:00:34 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat,  7 Aug 2021 17:22:25 +0200
> > Len Baker <len.baker@gmx.com> wrote:
> >  
> > > strcpy() performs no bounds checking on the destination buffer. This
> > > could result in linear overflows beyond the end of the buffer, leading
> > > to all kinds of misbehaviors. The safe replacement is strscpy().
> > >
> > > This patch is an effort to clean up the proliferation of str*()
> > > functions in the kernel and a previous step in the path to remove
> > > the strcpy function from the kernel entirely [1].
> > >
> > > [1] https://github.com/KSPP/linux/issues/88
> > >
> > > Signed-off-by: Len Baker <len.baker@gmx.com>  
> > Applied to the togreg branch of iio.git and pushed out as testing
> > so 0-day can poke at it and see if we missed anything.  
> 
> Isn't it too early? Or am I missing something (see below)?
> 
> ...
> 
> > >                       /* use length + 2 for adding minus sign if needed */
> > > -                     str = devm_kzalloc(regmap_get_device(st->map),
> > > -                                        strlen(orient) + 2, GFP_KERNEL);
> > > +                     n = strlen(orient) + 2;
> > > +                     str = devm_kzalloc(regmap_get_device(st->map), n,
> > > +                                        GFP_KERNEL);
> > >                       if (str == NULL)
> > >                               return -ENOMEM;
> > >                       if (strcmp(orient, "0") == 0) {
> > > -                             strcpy(str, orient);
> > > +                             strscpy(str, orient, n);
> > >                       } else if (orient[0] == '-') {
> > > -                             strcpy(str, &orient[1]);
> > > +                             strscpy(str, &orient[1], n);
> > >                       } else {
> > >                               str[0] = '-';
> > > -                             strcpy(&str[1], orient);
> > > +                             strscpy(&str[1], orient, n - 1);  
> 
> Why n-1?

n is the total length and this is printing from [1], so n - 1 is remaining
space.

> 
> > >                       }  
> 
> As far as I understood the logic, it  inverts the sign except the case
> when it equals 0.
> 
> I have a question here, why can't we always use +/-?
> Why can't 0 be prefixed with a sign?

Technically a userspace ABI change, but I agree it should be fairly
harmless unless someone is rolling their own string handling routines.

> 
> If the above can be used, we may simplify this code.
> 
> Len, I think this task may be considered simple, but I recommend
> thinking about each case and finding a way to simplify it more.
> 

It could be a little simpler doing this, so 'maybe' worth doing.

Jonathan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-09  9:21     ` Jonathan Cameron
@ 2021-08-09 16:14       ` Len Baker
  2021-08-10 12:06         ` Andy Shevchenko
  2021-08-10  8:30       ` David Laight
  1 sibling, 1 reply; 9+ messages in thread
From: Len Baker @ 2021-08-09 16:14 UTC (permalink / raw)
  To: Jonathan Cameron, Andy Shevchenko
  Cc: Len Baker, Lars-Peter Clausen, Kees Cook, linux-hardening,
	linux-iio, Linux Kernel Mailing List

Hi,

On Mon, Aug 09, 2021 at 10:21:31AM +0100, Jonathan Cameron wrote:
> On Sun, 8 Aug 2021 22:00:34 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Sat,  7 Aug 2021 17:22:25 +0200
> > > Len Baker <len.baker@gmx.com> wrote:
> > >
> > > > strcpy() performs no bounds checking on the destination buffer. This
> > > > could result in linear overflows beyond the end of the buffer, leading
> > > > to all kinds of misbehaviors. The safe replacement is strscpy().
> > > >
> > > > This patch is an effort to clean up the proliferation of str*()
> > > > functions in the kernel and a previous step in the path to remove
> > > > the strcpy function from the kernel entirely [1].
> > > >
> > > > [1] https://github.com/KSPP/linux/issues/88
> > > >
> > > > Signed-off-by: Len Baker <len.baker@gmx.com>
> > > Applied to the togreg branch of iio.git and pushed out as testing
> > > so 0-day can poke at it and see if we missed anything.
> >
> > Isn't it too early? Or am I missing something (see below)?
> >
> > ...
> >
> > > >                       /* use length + 2 for adding minus sign if needed */
> > > > -                     str = devm_kzalloc(regmap_get_device(st->map),
> > > > -                                        strlen(orient) + 2, GFP_KERNEL);
> > > > +                     n = strlen(orient) + 2;
> > > > +                     str = devm_kzalloc(regmap_get_device(st->map), n,
> > > > +                                        GFP_KERNEL);
> > > >                       if (str == NULL)
> > > >                               return -ENOMEM;
> > > >                       if (strcmp(orient, "0") == 0) {
> > > > -                             strcpy(str, orient);
> > > > +                             strscpy(str, orient, n);
> > > >                       } else if (orient[0] == '-') {
> > > > -                             strcpy(str, &orient[1]);
> > > > +                             strscpy(str, &orient[1], n);
> > > >                       } else {
> > > >                               str[0] = '-';
> > > > -                             strcpy(&str[1], orient);
> > > > +                             strscpy(&str[1], orient, n - 1);
> >
> > Why n-1?
>
> n is the total length and this is printing from [1], so n - 1 is remaining
> space.
>
> >
> > > >                       }
> >
> > As far as I understood the logic, it  inverts the sign except the case
> > when it equals 0.
> >
> > I have a question here, why can't we always use +/-?
> > Why can't 0 be prefixed with a sign?
>
> Technically a userspace ABI change, but I agree it should be fairly
> harmless unless someone is rolling their own string handling routines.

I personally don't like the idea of zero having a sign. It's my opinion.
But if you prefer it I have no problem.

> >
> > If the above can be used, we may simplify this code.
> >
> > Len, I think this task may be considered simple, but I recommend
> > thinking about each case and finding a way to simplify it more.
>
Andy, is what I try to do ;) Thanks for the advise.

> It could be a little simpler doing this, so 'maybe' worth doing.

Ok, I will send a new version if there are no objections.

Regards,
Len

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-09  9:21     ` Jonathan Cameron
  2021-08-09 16:14       ` Len Baker
@ 2021-08-10  8:30       ` David Laight
  1 sibling, 0 replies; 9+ messages in thread
From: David Laight @ 2021-08-10  8:30 UTC (permalink / raw)
  To: 'Jonathan Cameron', Andy Shevchenko
  Cc: Jonathan Cameron, Len Baker, Lars-Peter Clausen, Kees Cook,
	linux-hardening, linux-iio, Linux Kernel Mailing List

From: Jonathan Cameron
> Sent: 09 August 2021 10:22
> 
> On Sun, 8 Aug 2021 22:00:34 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Sat,  7 Aug 2021 17:22:25 +0200
> > > Len Baker <len.baker@gmx.com> wrote:
> > >
> > > > strcpy() performs no bounds checking on the destination buffer. This
> > > > could result in linear overflows beyond the end of the buffer, leading
> > > > to all kinds of misbehaviors. The safe replacement is strscpy().
> > > >
> > > > This patch is an effort to clean up the proliferation of str*()
> > > > functions in the kernel and a previous step in the path to remove
> > > > the strcpy function from the kernel entirely [1].
> > > >
> > > > [1] https://github.com/KSPP/linux/issues/88
> > > >
> > > > Signed-off-by: Len Baker <len.baker@gmx.com>
> > > Applied to the togreg branch of iio.git and pushed out as testing
> > > so 0-day can poke at it and see if we missed anything.
> >
> > Isn't it too early? Or am I missing something (see below)?
> >
> > ...
> >
> > > >                       /* use length + 2 for adding minus sign if needed */
> > > > -                     str = devm_kzalloc(regmap_get_device(st->map),
> > > > -                                        strlen(orient) + 2, GFP_KERNEL);
> > > > +                     n = strlen(orient) + 2;
> > > > +                     str = devm_kzalloc(regmap_get_device(st->map), n,
> > > > +                                        GFP_KERNEL);
> > > >                       if (str == NULL)
> > > >                               return -ENOMEM;
> > > >                       if (strcmp(orient, "0") == 0) {
> > > > -                             strcpy(str, orient);
> > > > +                             strscpy(str, orient, n);
> > > >                       } else if (orient[0] == '-') {
> > > > -                             strcpy(str, &orient[1]);
> > > > +                             strscpy(str, &orient[1], n);
> > > >                       } else {
> > > >                               str[0] = '-';
> > > > -                             strcpy(&str[1], orient);
> > > > +                             strscpy(&str[1], orient, n - 1);
> >
> > Why n-1?
> 
> n is the total length and this is printing from [1], so n - 1 is remaining
> space.

If you do:
	/* negate 'orient' */
	n = strlen(orient) + 1;
	str = malloc(n + 1);
	if (n == 2 && orient[0] == '0')
		memcpy(str, orient, n);
	else if (orient[0] == '-')
		memcpy(str, orient + 1, n - 1);
	else {
		str[0] = '-';
		memcpy(str + 1, orient, n);
	}
Then it is probably less confusing.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-09 16:14       ` Len Baker
@ 2021-08-10 12:06         ` Andy Shevchenko
  2021-08-10 12:11           ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-08-10 12:06 UTC (permalink / raw)
  To: Len Baker
  Cc: Jonathan Cameron, Lars-Peter Clausen, Kees Cook, linux-hardening,
	linux-iio, Linux Kernel Mailing List

On Mon, Aug 9, 2021 at 7:14 PM Len Baker <len.baker@gmx.com> wrote:
> On Mon, Aug 09, 2021 at 10:21:31AM +0100, Jonathan Cameron wrote:
> > On Sun, 8 Aug 2021 22:00:34 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > On Sat,  7 Aug 2021 17:22:25 +0200
> > > > Len Baker <len.baker@gmx.com> wrote:

...

> > > Isn't it too early? Or am I missing something (see below)?

> > > > >                       /* use length + 2 for adding minus sign if needed */
> > > > > -                     str = devm_kzalloc(regmap_get_device(st->map),
> > > > > -                                        strlen(orient) + 2, GFP_KERNEL);
> > > > > +                     n = strlen(orient) + 2;
> > > > > +                     str = devm_kzalloc(regmap_get_device(st->map), n,
> > > > > +                                        GFP_KERNEL);
> > > > >                       if (str == NULL)
> > > > >                               return -ENOMEM;
> > > > >                       if (strcmp(orient, "0") == 0) {
> > > > > -                             strcpy(str, orient);
> > > > > +                             strscpy(str, orient, n);
> > > > >                       } else if (orient[0] == '-') {
> > > > > -                             strcpy(str, &orient[1]);
> > > > > +                             strscpy(str, &orient[1], n);
> > > > >                       } else {
> > > > >                               str[0] = '-';
> > > > > -                             strcpy(&str[1], orient);
> > > > > +                             strscpy(&str[1], orient, n - 1);

Even if we leave the logic as is, this might be better

if (orient[0] == '-')
   str = devm_kstrdup(dev, orient + 1, GFP_KERNEL);
else if (orient[0] != '0' || orient[1] != '\0')
   str = devm_kasprintf(dev, GFP_KERNEL, "-%s", orient);
else
   str = devm_kstrdup(dev, orient, GFP_KERNEL);
if (!str)
   return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-10 12:06         ` Andy Shevchenko
@ 2021-08-10 12:11           ` Andy Shevchenko
  2021-08-10 15:43             ` Len Baker
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-08-10 12:11 UTC (permalink / raw)
  To: Len Baker
  Cc: Jonathan Cameron, Lars-Peter Clausen, Kees Cook, linux-hardening,
	linux-iio, Linux Kernel Mailing List

On Tue, Aug 10, 2021 at 3:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 9, 2021 at 7:14 PM Len Baker <len.baker@gmx.com> wrote:



> Even if we leave the logic as is, this might be better
>
> if (orient[0] == '-')
>    str = devm_kstrdup(dev, orient + 1, GFP_KERNEL);

> else if (orient[0] != '0' || orient[1] != '\0')
>    str = devm_kasprintf(dev, GFP_KERNEL, "-%s", orient);
> else
>    str = devm_kstrdup(dev, orient, GFP_KERNEL);

Or these two swapped for better reading

 else if (orient[0] == '0' **&& orient[1] == '\0')
    str = devm_kstrdup(dev, orient, GFP_KERNEL);
else
    str = devm_kasprintf(dev, GFP_KERNEL, "-%s", orient);

And with a comment added that we negate the result according to the
rules: 1) drop leading '-' (minus); 2) leave 0 as is; add leading '-'
(minus).

> if (!str)
>    return -ENOMEM;



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()
  2021-08-10 12:11           ` Andy Shevchenko
@ 2021-08-10 15:43             ` Len Baker
  0 siblings, 0 replies; 9+ messages in thread
From: Len Baker @ 2021-08-10 15:43 UTC (permalink / raw)
  To: Andy Shevchenko, David Laight, Jonathan Cameron
  Cc: Len Baker, Lars-Peter Clausen, Kees Cook, linux-hardening,
	linux-iio, Linux Kernel Mailing List

Hi Andy, David and Jonathan

First of all thanks you very much for your code snippets ;)
More below.

On Tue, Aug 10, 2021 at 03:11:01PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 10, 2021 at 3:06 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Aug 9, 2021 at 7:14 PM Len Baker <len.baker@gmx.com> wrote:
>
>
>
> > Even if we leave the logic as is, this might be better
> >
> > if (orient[0] == '-')
> >    str = devm_kstrdup(dev, orient + 1, GFP_KERNEL);
>
> > else if (orient[0] != '0' || orient[1] != '\0')
> >    str = devm_kasprintf(dev, GFP_KERNEL, "-%s", orient);
> > else
> >    str = devm_kstrdup(dev, orient, GFP_KERNEL);

I think the above snippet is the right one: Is compact and easy
to understand.

>
> Or these two swapped for better reading
>
>  else if (orient[0] == '0' **&& orient[1] == '\0')
>     str = devm_kstrdup(dev, orient, GFP_KERNEL);
> else
>     str = devm_kasprintf(dev, GFP_KERNEL, "-%s", orient);
>
> And with a comment added that we negate the result according to the
> rules: 1) drop leading '-' (minus); 2) leave 0 as is; add leading '-'
> (minus).
>
> > if (!str)
> >    return -ENOMEM;

Also, I think that it is better to leave the logic as is and don't
try to use always the +/- sign.

Again thanks to all for the feedback.

Regards,
Len

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-10 15:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-07 15:22 [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy() Len Baker
2021-08-08 16:25 ` Jonathan Cameron
2021-08-08 19:00   ` Andy Shevchenko
2021-08-09  9:21     ` Jonathan Cameron
2021-08-09 16:14       ` Len Baker
2021-08-10 12:06         ` Andy Shevchenko
2021-08-10 12:11           ` Andy Shevchenko
2021-08-10 15:43             ` Len Baker
2021-08-10  8:30       ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).