| Message ID | 20210807152225.9403-1-len.baker@gmx.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] drivers/iio: Remove all strcpy() uses in favor of strscpy() | expand |
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 >
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.
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
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
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)
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;
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;
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
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; }
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(-) -- 2.25.1