Received-SPF: pass (google.com: domain of linux-kernel+bounces-13949-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: David Laight <David.Laight@ACULAB.COM>
To: 'Guoxin Pu' <pugokushin@gmail.com>, "axboe@kernel.dk" <axboe@kernel.dk>
CC: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: RE: [PATCH] block: fix length of strscpy()
Thread-Topic: [PATCH] block: fix length of strscpy()
Thread-Index: AQHaPNtiMZZ0F2vqTUGoSDgTJCHi3bDFesXA
Date: Mon, 1 Jan 2024 21:47:23 +0000
Message-ID: <ed0b9dd45fca4f6e910a9e1ffa756180@AcuMS.aculab.com>
References: <20240101175051.38479-2-pugokushin@gmail.com>
In-Reply-To: <20240101175051.38479-2-pugokushin@gmail.com>
Accept-Language: en-GB, en-US
Precedence: bulk
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

From: Guoxin Pu
> Sent: 01 January 2024 17:51
>=20
> In commit 146afeb235ccec10c17ad8ea26327c0c79dbd968 ("block: use strscpy()
> to instead of strncpy()") , the length that should now represent the leng=
th
> of the string with the terminating NULL was not updated alongside the
> change.
>=20
> This has caused blkdevparts=3D definition on kernel cmdline to be not
> correctly recognized and partitions not correctly initialized, breaking a=
ny
> device relying on such partitions to boot, on stable releases since 6.6
>=20
> This patch fixes the lengths to contain the terminating NULL.
>=20
> 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(-)
>=20
> 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 **subpa=
rt, char *partdef)
>  =09=09=09goto fail;
>  =09=09}
>=20
> -=09=09length =3D min_t(int, next - partdef,
> -=09=09=09       sizeof(new_subpart->name) - 1);
> +=09=09length =3D min_t(int, next - partdef + 1,
> +=09=09=09       sizeof(new_subpart->name));
>  =09=09strscpy(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:
=09=09next =3D strchr(partdef, ',');
just above?
Maybe with:
=09=09new_subpart->name[length] =3D '\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)
>  =09=09goto fail;
>  =09}
>=20
> -=09length =3D min_t(int, next - bdevdef, sizeof(newparts->name) - 1);
> +=09length =3D min_t(int, next - bdevdef + 1, sizeof(newparts->name));
>  =09strscpy(newparts->name, bdevdef, length);

Same.

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

Same

> @@ -262,7 +262,7 @@ static int add_part(int slot, struct cmdline_subpart =
*subpart,
>=20
>  =09info =3D &state->parts[slot].info;
>=20
> -=09label_min =3D min_t(int, sizeof(info->volname) - 1,
> +=09label_min =3D min_t(int, sizeof(info->volname),
>  =09=09=09  sizeof(subpart->name));
>  =09strscpy(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:
=09strscpy(info->volname, subpart->name, sizeof (info->volname));

=09David

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