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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210801205528.211c4e9c@jic23-huawei>
Date:   Sun, 1 Aug 2021 20:55:28 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Len Baker <len.baker@....com>
Cc:     Kees Cook <keescook@...omium.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        linux-hardening@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/iio: Remove all strcpy() uses in favor of
 strscpy()

On Sun,  1 Aug 2021 19:11:57 +0200
Len Baker <len.baker@....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().
> 
> Signed-off-by: Len Baker <len.baker@....com>

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
>