linux-kernel - RE: [PATCH] block: fix length 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]

Date: Mon, 1 Jan 2024 21:47:23 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Guoxin Pu' <pugokushin@...il.com>, "axboe@...nel.dk" <axboe@...nel.dk>
CC: "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] block: fix length of strscpy()

From: Guoxin Pu
> Sent: 01 January 2024 17:51
> 
> In commit 146afeb235ccec10c17ad8ea26327c0c79dbd968 ("block: use strscpy()
> to instead of strncpy()") , the length that should now represent the length
> of the string with the terminating NULL was not updated alongside the
> change.
> 
> This has caused blkdevparts= definition on kernel cmdline to be not
> correctly recognized and partitions not correctly initialized, breaking any
> device relying on such partitions to boot, on stable releases since 6.6
> 
> This patch fixes the lengths to contain the terminating NULL.
> 
> Cc: stable@...r.kernel.org # 6.6.x
> Signed-off-by: Guoxin Pu <pugokushin@...il.com>
> ---
>  block/partitions/cmdline.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/partitions/cmdline.c b/block/partitions/cmdline.c
> index c03bc105e575..c2aac5f4ab82 100644
> --- a/block/partitions/cmdline.c
> +++ b/block/partitions/cmdline.c
> @@ -79,8 +79,8 @@ static int parse_subpart(struct cmdline_subpart **subpart, char *partdef)
>  			goto fail;
>  		}
> 
> -		length = min_t(int, next - partdef,
> -			       sizeof(new_subpart->name) - 1);
> +		length = min_t(int, next - partdef + 1,
> +			       sizeof(new_subpart->name));
>  		strscpy(new_subpart->name, partdef, length);

Shouldn't that be a memcpy() with the original length?
Since it looks as though there is something equivalent to:
		next = strchr(partdef, ',');
just above?
Maybe with:
		new_subpart->name[length] = '\0';
if the target isn't zero filled (which the strncpy() probably
relied on.)

> @@ -138,7 +138,7 @@ static int parse_parts(struct cmdline_parts **parts, const char *bdevdef)
>  		goto fail;
>  	}
> 
> -	length = min_t(int, next - bdevdef, sizeof(newparts->name) - 1);
> +	length = min_t(int, next - bdevdef + 1, sizeof(newparts->name));
>  	strscpy(newparts->name, bdevdef, length);

Same.

> @@ -148,8 +148,8 @@ static int parse_parts(struct cmdline_parts **parts, const char *bdevdef)
>  		bdevdef = next;
>  		next = strchr(bdevdef, ',');
> 
> -		length = (!next) ? (sizeof(buf) - 1) :
> -			min_t(int, next - bdevdef, sizeof(buf) - 1);
> +		length = (!next) ? sizeof(buf) :
> +			min_t(int, next - bdevdef + 1, sizeof(buf));
> 
>  		strscpy(buf, bdevdef, length);

Same

> @@ -262,7 +262,7 @@ static int add_part(int slot, struct cmdline_subpart *subpart,
> 
>  	info = &state->parts[slot].info;
> 
> -	label_min = min_t(int, sizeof(info->volname) - 1,
> +	label_min = min_t(int, sizeof(info->volname),
>  			  sizeof(subpart->name));
>  	strscpy(info->volname, subpart->name, label_min);

WTF?
That only makes any sense if subpart->name might not be '\0'
terminated - which strncpy() would have handled fine (with the -1).
Otherwise what is wrong with:
	strscpy(info->volname, subpart->name, sizeof (info->volname));

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)