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 <[email protected]>
---
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
On Sat, 7 Aug 2021 17:22:25 +0200
Len Baker <[email protected]> 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 <[email protected]>
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
>
On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 7 Aug 2021 17:22:25 +0200
> Len Baker <[email protected]> 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 <[email protected]>
> 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
On Sun, 8 Aug 2021 22:00:34 +0300
Andy Shevchenko <[email protected]> wrote:
> On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <[email protected]> wrote:
> >
> > On Sat, 7 Aug 2021 17:22:25 +0200
> > Len Baker <[email protected]> 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 <[email protected]>
> > 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
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 <[email protected]> wrote:
>
> > On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <[email protected]> wrote:
> > >
> > > On Sat, 7 Aug 2021 17:22:25 +0200
> > > Len Baker <[email protected]> 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 <[email protected]>
> > > 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
From: Jonathan Cameron
> Sent: 09 August 2021 10:22
>
> On Sun, 8 Aug 2021 22:00:34 +0300
> Andy Shevchenko <[email protected]> wrote:
>
> > On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <[email protected]> wrote:
> > >
> > > On Sat, 7 Aug 2021 17:22:25 +0200
> > > Len Baker <[email protected]> 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 <[email protected]>
> > > 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)
On Mon, Aug 9, 2021 at 7:14 PM Len Baker <[email protected]> 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 <[email protected]> wrote:
> > > On Sun, Aug 8, 2021 at 7:25 PM Jonathan Cameron <[email protected]> wrote:
> > > > On Sat, 7 Aug 2021 17:22:25 +0200
> > > > Len Baker <[email protected]> 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
On Tue, Aug 10, 2021 at 3:06 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Aug 9, 2021 at 7:14 PM Len Baker <[email protected]> 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
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
> <[email protected]> wrote:
> > On Mon, Aug 9, 2021 at 7:14 PM Len Baker <[email protected]> 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