2024-05-03 07:49:36

by David Yang

Subject: [PATCH] block: fix buf size for strscpy()

strscpy() takes the total size of destination buffer as the argument,
including the space for the terminating null character.

The actual length of the buffer should be len(str) + 1, which can be
seen from the indexes where null characters are written in the code
before the commit in question, and 'sizeof(buf) - 1' right above
the problematic codes.

Without the additional 1 size and the absence of checkes against -E2BIG,
strscpy() will angrily eat the last character of the source string. In
my situation, strscpy() will take away one character before the comma
"," (which is presumably the right bracket ")") in parse_parts(), making
parse_subpart() unable to 'strchr(++partdef, ')')' and producing the
following error message:

cmdline partition format is invalid.

Fixes: 146afeb235cc ("block: use strscpy() to instead of strncpy()")
Signed-off-by: David Yang <[email protected]>
---
block/partitions/cmdline.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/partitions/cmdline.c b/block/partitions/cmdline.c
index c03bc105e575..6d8401ce2943 100644
--- a/block/partitions/cmdline.c
+++ b/block/partitions/cmdline.c
@@ -81,7 +81,7 @@ static int parse_subpart(struct cmdline_subpart **subpart, char *partdef)

length = min_t(int, next - partdef,
sizeof(new_subpart->name) - 1);
- strscpy(new_subpart->name, partdef, length);
+ strscpy(new_subpart->name, partdef, length + 1);

partdef = ++next;
} else
@@ -139,7 +139,7 @@ static int parse_parts(struct cmdline_parts **parts, const char *bdevdef)
}

length = min_t(int, next - bdevdef, sizeof(newparts->name) - 1);
- strscpy(newparts->name, bdevdef, length);
+ strscpy(newparts->name, bdevdef, length + 1);
newparts->nr_subparts = 0;

next_subpart = &newparts->subpart;
@@ -151,7 +151,7 @@ static int parse_parts(struct cmdline_parts **parts, const char *bdevdef)
length = (!next) ? (sizeof(buf) - 1) :
min_t(int, next - bdevdef, sizeof(buf) - 1);

- strscpy(buf, bdevdef, length);
+ strscpy(buf, bdevdef, length + 1);

ret = parse_subpart(next_subpart, buf);
if (ret)
@@ -264,7 +264,7 @@ static int add_part(int slot, struct cmdline_subpart *subpart,

label_min = min_t(int, sizeof(info->volname) - 1,
sizeof(subpart->name));
- strscpy(info->volname, subpart->name, label_min);
+ strscpy(info->volname, subpart->name, label_min + 1);

snprintf(tmp, sizeof(tmp), "(%s)", info->volname);
strlcat(state->pp_buf, tmp, PAGE_SIZE);
--
2.43.0

2024-05-03 12:37:21

by James Bottomley

Subject: Re: [PATCH] block: fix buf size for strscpy()

On Fri, 2024-05-03 at 15:48 +0800, David Yang wrote:
> strscpy() takes the total size of destination buffer as the argument,
> including the space for the terminating null character.
>
> The actual length of the buffer should be len(str) + 1, which can be
> seen from the indexes where null characters are written in the code
> before the commit in question, and 'sizeof(buf) - 1' right above
> the problematic codes.
>
> Without the additional 1 size and the absence of checkes against -
> E2BIG, strscpy() will angrily eat the last character of the source
> string. In my situation, strscpy() will take away one character
> before the comma "," (which is presumably the right bracket ")") in
> parse_parts(), making parse_subpart() unable to 'strchr(++partdef,
> ')')' and producing the following error message:
>
>   cmdline partition format is invalid.

This is the same problem we brought up with the strscpy conversions in
scsi:

https://lore.kernel.org/all/784db8a20a3ddeb6c0498f2b31719e5198da6581.camel@HansenPartnership.com/

strscpy doesn't correctly replace a function of strncpy we used to get
a zero termination for a possibly unterminated string with a
destination that's one byte larger than the source. The current
proposed fix is this one:

https://lore.kernel.org/all/[email protected]/

James