[PATCH] mtd: ubi: replace strcpy with strscpy in mtd parameter parser

[PATCH] mtd: ubi: replace strcpy with strscpy in mtd parameter parser

[Miguel García]

Replace the strcpy() calls used to copy the 'mtd=' parameter into local
buffers with strscpy() to avoid potential overflow and guarantee NUL
termination. Destinations are fixed-size arrays (buf and p->name), so
use sizeof().

While this code is currently safe due to prior length checks
(strnlen(val, MTD_PARAM_LEN_MAX) and early return on overflow),
replacing strcpy() with strscpy() follows current kernel best practices
and makes the code more robust to future changes. The sizeof() calls
correctly compute the buffer sizes, matching MTD_PARAM_LEN_MAX.

Tested in QEMU (initramfs + built-ins):
 - mtdram.total_size=16384 mtdram.erasesize=256 ubi.mtd=0
 - ubi.mtd="mtdram test device"
 - overly long name -> proper "parameter ... is too long" handling

No functional change intended.

Signed-off-by: Miguel García <miguelgarciaroman8@gmail.com>
---
 drivers/mtd/ubi/build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index ef6a22f372f9..0d9f31522356 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1497,7 +1497,7 @@ static int ubi_mtd_param_parse(const char *val, const struct kernel_param *kp)
 		return 0;
 	}
 
-	strcpy(buf, val);
+	strscpy(buf, val, sizeof(buf));
 
 	/* Get rid of the final newline */
 	if (buf[len - 1] == '\n')
@@ -1512,7 +1512,7 @@ static int ubi_mtd_param_parse(const char *val, const struct kernel_param *kp)
 	}
 
 	p = &mtd_dev_param[mtd_devs];
-	strcpy(&p->name[0], tokens[0]);
+	strscpy(&p->name[0], tokens[0], sizeof(p->name));
 
 	token = tokens[1];
 	if (token) {
-- 
2.34.1

Re: [PATCH] mtd: ubi: replace strcpy with strscpy in mtd parameter parser

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Miguel García" <miguelgarciaroman8@gmail.com>
> An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>
> CC: "chengzhihao1" <chengzhihao1@huawei.com>, "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel"
> <linux-kernel@vger.kernel.org>, "Shuah Khan" <skhan@linuxfoundation.org>, "Miguel García"
> <miguelgarciaroman8@gmail.com>
> Gesendet: Montag, 11. August 2025 14:09:12
> Betreff: [PATCH] mtd: ubi: replace strcpy with strscpy in mtd parameter parser

> Replace the strcpy() calls used to copy the 'mtd=' parameter into local
> buffers with strscpy() to avoid potential overflow and guarantee NUL
> termination. Destinations are fixed-size arrays (buf and p->name), so
> use sizeof().
> 
> While this code is currently safe due to prior length checks
> (strnlen(val, MTD_PARAM_LEN_MAX) and early return on overflow),
> replacing strcpy() with strscpy() follows current kernel best practices
> and makes the code more robust to future changes. The sizeof() calls
> correctly compute the buffer sizes, matching MTD_PARAM_LEN_MAX.

TBH, I'm not convinced. We're talking about kernel module parameters,
not hostile user input, etc...

By adding sizeof() you're replacing one foodgun with another one.
If buf is in future changed to a pointer, sizeof(buf) needs a fixup too.

Thanks,
//richard

Re: [PATCH] mtd: ubi: replace strcpy with strscpy in mtd parameter parser

[Miguel García Román]

El lun, 11 ago 2025 a las 14:46, Richard Weinberger (<richard@nod.at>) escribió:
>
> ----- Ursprüngliche Mail -----
> > Von: "Miguel García" <miguelgarciaroman8@gmail.com>
> > An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>
> > CC: "chengzhihao1" <chengzhihao1@huawei.com>, "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel"
> > <linux-kernel@vger.kernel.org>, "Shuah Khan" <skhan@linuxfoundation.org>, "Miguel García"
> > <miguelgarciaroman8@gmail.com>
> > Gesendet: Montag, 11. August 2025 14:09:12
> > Betreff: [PATCH] mtd: ubi: replace strcpy with strscpy in mtd parameter parser
>
> > Replace the strcpy() calls used to copy the 'mtd=' parameter into local
> > buffers with strscpy() to avoid potential overflow and guarantee NUL
> > termination. Destinations are fixed-size arrays (buf and p->name), so
> > use sizeof().
> >
> > While this code is currently safe due to prior length checks
> > (strnlen(val, MTD_PARAM_LEN_MAX) and early return on overflow),
> > replacing strcpy() with strscpy() follows current kernel best practices
> > and makes the code more robust to future changes. The sizeof() calls
> > correctly compute the buffer sizes, matching MTD_PARAM_LEN_MAX.
>
> TBH, I'm not convinced. We're talking about kernel module parameters,
> not hostile user input, etc...
>
> By adding sizeof() you're replacing one foodgun with another one.
That's true
> If buf is in future changed to a pointer, sizeof(buf) needs a fixup too.
what about using the two-parameters version of strscpy?
>
> Thanks,
> //richard