[PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Len Baker]

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

Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Jonathan Cameron]

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
> 

Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Andy Shevchenko]

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

Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Jonathan Cameron]

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

Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Len Baker]

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

Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Andy Shevchenko]

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

Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Andy Shevchenko]

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

Re: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[Len Baker]

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

RE: [PATCH v2] drivers/iio: Remove all strcpy() uses in favor of strscpy()

[David Laight]

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)