2021-08-01 17:16:40

by Len Baker

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

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().

Signed-off-by: Len Baker <[email protected]>
---
This is a task of the KSPP [1]

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

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

2021-08-01 19:55:17

by Jonathan Cameron

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

On Sun, 1 Aug 2021 19:11:57 +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().
>
> Signed-off-by: Len Baker <[email protected]>

Hi Len,

I'm not convinced this is terribly useful in this particular case.
As the code stands today, it is easy to verify that the buffer is
large enough by looking up a few lines. As you need to do that
anyway to check that n, n -1 in the strscpy is correct, what do we gain?

I don't mind this if it's part of a general removal of strcpy(), but
if so the patch description should state that.

Jonathan

> ---
> This is a task of the KSPP [1]
>
> [1] https://github.com/KSPP/linux/issues/88
>
> 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
>

2021-08-07 15:03:18

by Len Baker

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

Hi,

On Sun, Aug 01, 2021 at 08:55:28PM +0100, Jonathan Cameron wrote:
> On Sun, 1 Aug 2021 19:11:57 +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().
> >
> > Signed-off-by: Len Baker <[email protected]>
>
> Hi Len,
>
> I'm not convinced this is terribly useful in this particular case.
> As the code stands today, it is easy to verify that the buffer is
> large enough by looking up a few lines. As you need to do that
> anyway to check that n, n -1 in the strscpy is correct, what do we gain?
>
> I don't mind this if it's part of a general removal of strcpy(), but
> if so the patch description should state that.

Ok, I will send a new version with the commit changelog updated.

Regards,
Len