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

From: Jonathan Cameron
Date: Sun Aug 01 2021 - 15:53:15 EST


On Sun, 1 Aug 2021 19:11:57 +0200
Len Baker <len.baker@xxxxxxx> 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 <len.baker@xxxxxxx>

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
>