[PATCH] block: fix length of strscpy()

[PATCH] block: fix length of strscpy()

[Guoxin Pu]

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@vger.kernel.org # 6.6.x
Signed-off-by: Guoxin Pu <pugokushin@gmail.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);
 
 		partdef = ++next;
@@ -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);
 	newparts->nr_subparts = 0;
 
@@ -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);
 
@@ -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);
 
-- 
2.43.0

Re: [PATCH] block: fix length of strscpy()

[Jens Axboe]

On 1/1/24 10:50 AM, Guoxin Pu wrote:
> 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.

This needs a Fixes line.

-- 
Jens Axboe

Re: [PATCH] block: fix length of strscpy()

[Guoxin Pu]

On 02/01/2024 05:26, Jens Axboe wrote:
> On 1/1/24 10:50 AM, Guoxin Pu wrote:
>> 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.
> This needs a Fixes line.
>
Sorry for the late reply.

Thank you for the review. The Fixes line was added and I've sent the new 
patch as "[PATCH v2] block: fix length of strscpy()" earlier on Jan, 2nd.

And since 6.7 is out, do I need to rework the v2 patch and add cc stable 
6.7.x?

RE: [PATCH] block: fix length of strscpy()

[David Laight]

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@vger.kernel.org # 6.6.x
> Signed-off-by: Guoxin Pu <pugokushin@gmail.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)

Re: [PATCH] block: fix length of strscpy()

[Guoxin Pu]

Thank you for the review. Sorry if this is the duplicated reply, as I 
didn't configure my mail client to send text-only message and the 
previous mail was rejected by the list.

On 02/01/2024 05:47, David Laight wrote:
>> @@ -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.)

Yes that would be better. But since I'm fixing the issue caused by the 
mentioned commit, which was an accepted change to use strscpy instead of 
strncpy and seems a part of a series of changes to do that, I think 
there might be a reason the maintainers preferred strscpy over strncpy 
over memcpy? Otherwise we could just revert that commit and keep using 
the original strncpy + setting NULL method, and then potentially swap 
strncpy with memcpy.

On 02/01/2024 05:47, David Laight wrote:

 > Same

Same.

On 02/01/2024 05:47, David Laight wrote:
>> @@ -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

Yes, there is no need to calculate label_min here. We could remove int 
label_min altogether in this function and use a single line of strscpy.

RE: [PATCH] block: fix length of strscpy()

[David Laight]

From: Guoxin Pu 
> Sent: 02 January 2024 02:31
> 
> Thank you for the review. Sorry if this is the duplicated reply, as I
> didn't configure my mail client to send text-only message and the
> previous mail was rejected by the list.
> 
> On 02/01/2024 05:47, David Laight wrote:
> >> @@ -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.)
> 
> Yes that would be better. But since I'm fixing the issue caused by the
> mentioned commit, which was an accepted change to use strscpy instead of
> strncpy and seems a part of a series of changes to do that, I think
> there might be a reason the maintainers preferred strscpy over strncpy
> over memcpy? Otherwise we could just revert that commit and keep using
> the original strncpy + setting NULL method, and then potentially swap
> strncpy with memcpy.

I suspect they accepted the change without realising just how
creative some of the strncpy() calls are.
While strscpy() is a better function than strncpy() (or strlcpy())
extreme care has to be taken to avoid adding bugs to code that
was actually fine.

	David

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