* [PATCH] Fix possible strscpy() buffer overflows
@ 2026-05-10 18:24 Alexander A. Klimov
2026-05-10 22:08 ` David Laight
0 siblings, 1 reply; 12+ messages in thread
From: Alexander A. Klimov @ 2026-05-10 18:24 UTC (permalink / raw)
To: Shubhrajyoti Datta, Borislav Petkov, Tony Luck, Kees Cook,
Arnd Bergmann, Greg Kroah-Hartman, Nick Li, Liam Girdwood,
Mark Brown, Jaroslav Kysela, Takashi Iwai
Cc: linux-edac, Linux Kernel Mailing List, linux-sound
In the changed files, strings were copied like this:
strscpy(DST, SRC, strlen(SRC));
A buffer overflow would happen if strlen(SRC) > sizeof(DST).
Actually, strscpy() must be used this way:
strscpy(DST, SRC, sizeof(DST));
strscpy(DST, SRC); // defaults to sizeof(DST)
Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
---
drivers/edac/versalnet_edac.c | 3 +--
drivers/misc/lkdtm/fortify.c | 6 +-----
sound/soc/codecs/fs210x.c | 2 +-
3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index ec13155824..daa140f4db 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -728,8 +728,7 @@ static int rpmsg_probe(struct rpmsg_device *rpdev)
pg = (struct mc_priv *)amd_rpmsg_id_table[0].driver_data;
chinfo.src = RPMSG_ADDR_ANY;
chinfo.dst = rpdev->dst;
- strscpy(chinfo.name, amd_rpmsg_id_table[0].name,
- strlen(amd_rpmsg_id_table[0].name));
+ strscpy(chinfo.name, amd_rpmsg_id_table[0].name);
pg->ept = rpmsg_create_ept(rpdev, rpmsg_cb, NULL, chinfo);
if (!pg->ept)
diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c
index 7615a02dfc..9a9159a120 100644
--- a/drivers/misc/lkdtm/fortify.c
+++ b/drivers/misc/lkdtm/fortify.c
@@ -174,11 +174,7 @@ static void lkdtm_FORTIFY_STRSCPY(void)
/* Restore src to its initial value. */
src[3] = 'b';
- /*
- * Use strlen here so size cannot be known at compile time and there is
- * a runtime write overflow.
- */
- strscpy(dst, src, strlen(src));
+ strscpy(dst, src);
pr_err("FAIL: strscpy() overflow not detected!\n");
pr_expected_config(CONFIG_FORTIFY_SOURCE);
diff --git a/sound/soc/codecs/fs210x.c b/sound/soc/codecs/fs210x.c
index e6195b71ad..eda716f817 100644
--- a/sound/soc/codecs/fs210x.c
+++ b/sound/soc/codecs/fs210x.c
@@ -968,7 +968,7 @@ static int fs210x_effect_scene_info(struct snd_kcontrol *kcontrol,
if (scene->name)
name = scene->name;
- strscpy(uinfo->value.enumerated.name, name, strlen(name) + 1);
+ strscpy(uinfo->value.enumerated.name, name);
return 0;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-10 18:24 [PATCH] Fix possible strscpy() buffer overflows Alexander A. Klimov
@ 2026-05-10 22:08 ` David Laight
2026-05-11 1:30 ` Geraldo Nascimento
0 siblings, 1 reply; 12+ messages in thread
From: David Laight @ 2026-05-10 22:08 UTC (permalink / raw)
To: Alexander A. Klimov
Cc: Shubhrajyoti Datta, Borislav Petkov, Tony Luck, Kees Cook,
Arnd Bergmann, Greg Kroah-Hartman, Nick Li, Liam Girdwood,
Mark Brown, Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
On Sun, 10 May 2026 20:24:41 +0200
"Alexander A. Klimov" <grandmaster@al2klimov.de> wrote:
> In the changed files, strings were copied like this:
>
> strscpy(DST, SRC, strlen(SRC));
>
> A buffer overflow would happen if strlen(SRC) > sizeof(DST).
> Actually, strscpy() must be used this way:
>
> strscpy(DST, SRC, sizeof(DST));
> strscpy(DST, SRC); // defaults to sizeof(DST)
Nak.
This is test code and deliberately doing things 'wrong'.
-- David
>
> Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
> ---
> drivers/edac/versalnet_edac.c | 3 +--
> drivers/misc/lkdtm/fortify.c | 6 +-----
> sound/soc/codecs/fs210x.c | 2 +-
> 3 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index ec13155824..daa140f4db 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -728,8 +728,7 @@ static int rpmsg_probe(struct rpmsg_device *rpdev)
> pg = (struct mc_priv *)amd_rpmsg_id_table[0].driver_data;
> chinfo.src = RPMSG_ADDR_ANY;
> chinfo.dst = rpdev->dst;
> - strscpy(chinfo.name, amd_rpmsg_id_table[0].name,
> - strlen(amd_rpmsg_id_table[0].name));
> + strscpy(chinfo.name, amd_rpmsg_id_table[0].name);
>
> pg->ept = rpmsg_create_ept(rpdev, rpmsg_cb, NULL, chinfo);
> if (!pg->ept)
> diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c
> index 7615a02dfc..9a9159a120 100644
> --- a/drivers/misc/lkdtm/fortify.c
> +++ b/drivers/misc/lkdtm/fortify.c
> @@ -174,11 +174,7 @@ static void lkdtm_FORTIFY_STRSCPY(void)
> /* Restore src to its initial value. */
> src[3] = 'b';
>
> - /*
> - * Use strlen here so size cannot be known at compile time and there is
> - * a runtime write overflow.
> - */
> - strscpy(dst, src, strlen(src));
> + strscpy(dst, src);
>
> pr_err("FAIL: strscpy() overflow not detected!\n");
> pr_expected_config(CONFIG_FORTIFY_SOURCE);
> diff --git a/sound/soc/codecs/fs210x.c b/sound/soc/codecs/fs210x.c
> index e6195b71ad..eda716f817 100644
> --- a/sound/soc/codecs/fs210x.c
> +++ b/sound/soc/codecs/fs210x.c
> @@ -968,7 +968,7 @@ static int fs210x_effect_scene_info(struct snd_kcontrol *kcontrol,
> if (scene->name)
> name = scene->name;
>
> - strscpy(uinfo->value.enumerated.name, name, strlen(name) + 1);
> + strscpy(uinfo->value.enumerated.name, name);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-10 22:08 ` David Laight
@ 2026-05-11 1:30 ` Geraldo Nascimento
2026-05-11 6:46 ` Andrei Purdea
0 siblings, 1 reply; 12+ messages in thread
From: Geraldo Nascimento @ 2026-05-11 1:30 UTC (permalink / raw)
To: David Laight
Cc: Alexander A. Klimov, Shubhrajyoti Datta, Borislav Petkov,
Tony Luck, Kees Cook, Arnd Bergmann, Greg Kroah-Hartman, Nick Li,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
linux-edac, Linux Kernel Mailing List, linux-sound
Hi David,
On Sun, May 10, 2026 at 11:08:53PM +0100, David Laight wrote:
> On Sun, 10 May 2026 20:24:41 +0200
> "Alexander A. Klimov" <grandmaster@al2klimov.de> wrote:
>
> > In the changed files, strings were copied like this:
> >
> > strscpy(DST, SRC, strlen(SRC));
> >
> > A buffer overflow would happen if strlen(SRC) > sizeof(DST).
> > Actually, strscpy() must be used this way:
> >
> > strscpy(DST, SRC, sizeof(DST));
> > strscpy(DST, SRC); // defaults to sizeof(DST)
>
> Nak.
>
> This is test code and deliberately doing things 'wrong'.
>
> -- David
while the change to fortify.c is what you described, the other two look
like good catches to me.
Thanks,
Geraldo Nascimento
>
> >
> > Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
> > ---
> > drivers/edac/versalnet_edac.c | 3 +--
> > drivers/misc/lkdtm/fortify.c | 6 +-----
> > sound/soc/codecs/fs210x.c | 2 +-
> > 3 files changed, 3 insertions(+), 8 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 1:30 ` Geraldo Nascimento
@ 2026-05-11 6:46 ` Andrei Purdea
2026-05-11 10:38 ` Borislav Petkov
0 siblings, 1 reply; 12+ messages in thread
From: Andrei Purdea @ 2026-05-11 6:46 UTC (permalink / raw)
To: Geraldo Nascimento
Cc: david.laight.linux, Alexander A. Klimov, Shubhrajyoti Datta,
Borislav Petkov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
Hi all,
Furthermore the one in versalnet_edac.c looks like beyond fixing a
buffer overflow risk and code smell, it also introduces a behavior
change (bugfix?), because the old code I believe cuts off the last
character from the copied string. (Because it was using just strlen()
and not using strlen() + 1)
So I think the effects of this behavioral change should be documented
in the commit message. (I.e. where did the incorrect string end up
being visible, and could there be side effects from fixing this? e.g.
Could some userspace scripts/applications start breaking that were
expecting the incorrect shorter string? Etc...)
Andrei.
On Mon, May 11, 2026 at 4:30 AM Geraldo Nascimento
<geraldogabriel@gmail.com> wrote:
>
> Hi David,
>
> On Sun, May 10, 2026 at 11:08:53PM +0100, David Laight wrote:
> > On Sun, 10 May 2026 20:24:41 +0200
> > "Alexander A. Klimov" <grandmaster@al2klimov.de> wrote:
> >
> > > In the changed files, strings were copied like this:
> > >
> > > strscpy(DST, SRC, strlen(SRC));
> > >
> > > A buffer overflow would happen if strlen(SRC) > sizeof(DST).
> > > Actually, strscpy() must be used this way:
> > >
> > > strscpy(DST, SRC, sizeof(DST));
> > > strscpy(DST, SRC); // defaults to sizeof(DST)
> >
> > Nak.
> >
> > This is test code and deliberately doing things 'wrong'.
> >
> > -- David
>
> while the change to fortify.c is what you described, the other two look
> like good catches to me.
>
> Thanks,
> Geraldo Nascimento
> >
> > >
> > > Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
> > > ---
> > > drivers/edac/versalnet_edac.c | 3 +--
> > > drivers/misc/lkdtm/fortify.c | 6 +-----
> > > sound/soc/codecs/fs210x.c | 2 +-
> > > 3 files changed, 3 insertions(+), 8 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 6:46 ` Andrei Purdea
@ 2026-05-11 10:38 ` Borislav Petkov
2026-05-11 11:58 ` David Laight
2026-05-11 11:59 ` Andrei Purdea
0 siblings, 2 replies; 12+ messages in thread
From: Borislav Petkov @ 2026-05-11 10:38 UTC (permalink / raw)
To: Andrei Purdea, Shubhrajyoti Datta
Cc: Geraldo Nascimento, david.laight.linux, Alexander A. Klimov,
Shubhrajyoti Datta, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
On Mon, May 11, 2026 at 09:46:53AM +0300, Andrei Purdea wrote:
> Hi all,
Hi,
first of all, please do not top-post when replying to a public mailing list.
> Furthermore the one in versalnet_edac.c looks like beyond fixing a
> buffer overflow risk and code smell, it also introduces a behavior
> change (bugfix?), because the old code I believe cuts off the last
> character from the copied string. (Because it was using just strlen()
> and not using strlen() + 1)
The versalnet_edac is innocuous because, AFAICT, that silly code is copying
"error_ipc" into
struct rpmsg_channel_info {
char name[RPMSG_NAME_SIZE];
with that buffer size being 32.
So no overflow here.
However, just to make this safer, we should min the size.
Also, Shubhrajyoti, why aren't you padding the rest of the name string with \0
*especially* since you're allocating it on the stack?!
IOW, this (totally untested ofc):
strscpy_pad(chinfo.name,
amd_rpmsg_id_table[0].name,
min_t(size_t, strlen(amd_rpmsg_id_table[0].name), RPMSG_NAME_SIZE));
I still don't like it because we're noodling with this string as if it is
super special and copying it back'n'forth just because this rpmsg is
MODULE_DEVICE_TABLE and apparently is used to autoload the driver or so...
Why do we need it?
Can we simplify that gunk there?
Please have a look.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 10:38 ` Borislav Petkov
@ 2026-05-11 11:58 ` David Laight
2026-05-11 11:59 ` Andrei Purdea
1 sibling, 0 replies; 12+ messages in thread
From: David Laight @ 2026-05-11 11:58 UTC (permalink / raw)
To: Borislav Petkov
Cc: Andrei Purdea, Shubhrajyoti Datta, Geraldo Nascimento,
Alexander A. Klimov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
On Mon, 11 May 2026 12:38:54 +0200
Borislav Petkov <bp@alien8.de> wrote:
> On Mon, May 11, 2026 at 09:46:53AM +0300, Andrei Purdea wrote:
> > Hi all,
>
> Hi,
>
> first of all, please do not top-post when replying to a public mailing list.
>
> > Furthermore the one in versalnet_edac.c looks like beyond fixing a
> > buffer overflow risk and code smell, it also introduces a behavior
> > change (bugfix?), because the old code I believe cuts off the last
> > character from the copied string. (Because it was using just strlen()
> > and not using strlen() + 1)
>
> The versalnet_edac is innocuous because, AFAICT, that silly code is copying
> "error_ipc" into
>
> struct rpmsg_channel_info {
> char name[RPMSG_NAME_SIZE];
>
> with that buffer size being 32.
>
> So no overflow here.
So just strcpy(chinfo.name, "error_ipc") will be absolutely fine.
If you extend the string to 32+ characters you'll get a compile error.
It degenerates to memcpy() and is likely to further degenerate to writes
of constant values.
>
> However, just to make this safer, we should min the size.
>
> Also, Shubhrajyoti, why aren't you padding the rest of the name string with \0
> *especially* since you're allocating it on the stack?!
>
> IOW, this (totally untested ofc):
>
> strscpy_pad(chinfo.name,
> amd_rpmsg_id_table[0].name,
> min_t(size_t, strlen(amd_rpmsg_id_table[0].name), RPMSG_NAME_SIZE));
Why isn't that just:
strscpy_pad(chinfo.name, amd_rpmsg_id_table[0].name, sizeof (chinfo.name));
I'm trying to remove all the unbounded strlen(), getting fed up of code
that does strlen() and strcpy() of the same string.
If you want the length, get it first, use it for the size checks, then
copy with memcpy().
-- David
>
> I still don't like it because we're noodling with this string as if it is
> super special and copying it back'n'forth just because this rpmsg is
> MODULE_DEVICE_TABLE and apparently is used to autoload the driver or so...
>
> Why do we need it?
>
> Can we simplify that gunk there?
>
> Please have a look.
>
> Thx.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 10:38 ` Borislav Petkov
2026-05-11 11:58 ` David Laight
@ 2026-05-11 11:59 ` Andrei Purdea
2026-05-11 12:51 ` Borislav Petkov
1 sibling, 1 reply; 12+ messages in thread
From: Andrei Purdea @ 2026-05-11 11:59 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shubhrajyoti Datta, Geraldo Nascimento, david.laight.linux,
Alexander A. Klimov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
> first of all, please do not top-post when replying to a public mailing list.
Acknowledged, apologies
> > Furthermore the one in versalnet_edac.c looks like beyond fixing a
> > buffer overflow risk and code smell, it also introduces a behavior
> > change (bugfix?), because the old code I believe cuts off the last
> > character from the copied string. (Because it was using just strlen()
> > and not using strlen() + 1)
>
> The versalnet_edac is innocuous because, AFAICT, that silly code is copying
> "error_ipc" into
No, the current code copies "error_ip" with null termination, and it
drops the "c" suffix.
And that seems buggy. And that's what I requested to explain the effects of.
>
> struct rpmsg_channel_info {
> char name[RPMSG_NAME_SIZE];
>
> with that buffer size being 32.
>
> So no overflow here.
Agreed. Also, even IF the source buffer gets overwritten with a different string
(not saying it necessarily does get overritten, but it's not const),
as long as it would
contain a null-terminated string there still wouldn't be an overflow,
cause the source
buffer is the same size. So the only way I see this being exploited
would be if it was
possible to get the source buffer to contain no null-terminators, but
that would imply
fault in other code too, so there's likely no security issue here,
(unless the dropped
"c" causes some problems).
>
> However, just to make this safer, we should min the size.
>
> Also, Shubhrajyoti, why aren't you padding the rest of the name string with \0
> *especially* since you're allocating it on the stack?!
>
> IOW, this (totally untested ofc):
>
> strscpy_pad(chinfo.name,
> amd_rpmsg_id_table[0].name,
> min_t(size_t, strlen(amd_rpmsg_id_table[0].name), RPMSG_NAME_SIZE));
I have two issues with the min_t() call here:
1) it still only copies "error_ip", with null termination, without the "c"
2) it negates the purpose of using strscpy_pad. If you use strlen(),
then the padding will
not happen, and it will behave exactly like strscpy.
3) it is unnecessary. Both strscpy and strscpy_pad stop reading from the source
buffer once they encounter a null terminator.
So if you want to pad the string, to avoid leaking old stack data,
it should just be:
strscpy_pad(chinfo.name, amd_rpmsg_id_table[0].name);
Andrei
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 11:59 ` Andrei Purdea
@ 2026-05-11 12:51 ` Borislav Petkov
2026-05-11 13:13 ` Andrei Purdea
2026-05-11 19:15 ` David Laight
0 siblings, 2 replies; 12+ messages in thread
From: Borislav Petkov @ 2026-05-11 12:51 UTC (permalink / raw)
To: Andrei Purdea
Cc: Shubhrajyoti Datta, Geraldo Nascimento, david.laight.linux,
Alexander A. Klimov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
On Mon, May 11, 2026 at 11:59:34AM +0000, Andrei Purdea wrote:
> No, the current code copies "error_ip" with null termination, and it
> drops the "c" suffix.
Pfff, that wasn't really clear to me from the explanation of strscpy...
> And that seems buggy. And that's what I requested to explain the effects of.
Shubhrajyoti, does that have any visible effects when using the driver?
> strscpy_pad(chinfo.name, amd_rpmsg_id_table[0].name);
No, as said "[h]owever, just to make this safer, we should min the size".
IOW:
strscpy_pad(chinfo.name,
amd_rpmsg_id_table[0].name,
min_t(size_t, strlen(amd_rpmsg_id_table[0].name) + 1, RPMSG_NAME_SIZE));
In case someone goes and changes that amd_rpmsg_id_table[0].name in the
future.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 12:51 ` Borislav Petkov
@ 2026-05-11 13:13 ` Andrei Purdea
2026-05-11 13:39 ` Borislav Petkov
2026-05-11 19:15 ` David Laight
1 sibling, 1 reply; 12+ messages in thread
From: Andrei Purdea @ 2026-05-11 13:13 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shubhrajyoti Datta, Geraldo Nascimento, david.laight.linux,
Alexander A. Klimov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
> Pfff, that wasn't really clear to me from the explanation of strscpy...
The third (optional) argument is count, which is specified as "Size of
destination buffer".
strscpy() does "Copy the string, or as much of it as fits".
The doc also says "The destination buffer is always NUL terminated,
unless it's zero-sized."
strlen() return value doesn't include the nul-terminator, but the
destination buffer must have space for it.
So all of these together imply that if you give strlen() of the source
as the third argument, the last character is gonna be cut off.
But this wasn't spelled out explicitly in the doc, because you're not
really supposed to use strlen() to specify the destination buffer
size.
The destination buffer size is how much space is allocated in memory,
and that's it. If you don't specify the third argument, it
automatically takes a sizeof() of the destination buffer, which only
works if it's an array, and not if it's a pointer.
The size of the source string is handled internally, (i.e. there is a
call to strlen() of the source buffer internally in strscpy)
> > strscpy_pad(chinfo.name, amd_rpmsg_id_table[0].name);
>
> No, as said "[h]owever, just to make this safer, we should min the size".
>
> IOW:
>
> strscpy_pad(chinfo.name,
> amd_rpmsg_id_table[0].name,
> min_t(size_t, strlen(amd_rpmsg_id_table[0].name) + 1, RPMSG_NAME_SIZE));
Why would that be safer? There's already a strlen() call inside
strscpy. (and even if you don't want to read the implementation, the
documentation implies this, by saying "Copy the string", because bytes
past the nul-terminator are not considered part of the string. It's
how all string copy functions always work, it's what defines C-style
string copying, otherwise it would be called memory copy.)
Plus strscpy_pad() needs to know the actual destination buffer size to
know how many bytes to pad if you want to pad.
If you give it a 3rd argument, then most of the time it's gonna be
less then the size of the buffer, so it's not gonna do any padding at
all, so it's just gonna be equivalent to strscpy() without _pad()
Andrei
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 13:13 ` Andrei Purdea
@ 2026-05-11 13:39 ` Borislav Petkov
2026-05-11 15:06 ` Andrei Purdea
0 siblings, 1 reply; 12+ messages in thread
From: Borislav Petkov @ 2026-05-11 13:39 UTC (permalink / raw)
To: Andrei Purdea
Cc: Shubhrajyoti Datta, Geraldo Nascimento, david.laight.linux,
Alexander A. Klimov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
On Mon, May 11, 2026 at 01:13:05PM +0000, Andrei Purdea wrote:
> But this wasn't spelled out explicitly in the doc, because you're not
> really supposed to use strlen() to specify the destination buffer
> size.
Why am I not supposed to? Says who?
> Why would that be safer? There's already a strlen() call inside
> strscpy.
Do you mean this thing:
sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst) + \
^^^^^^^^^^
__must_be_cstr(dst) + __must_be_cstr(src))
?
> (and even if you don't want to read the implementation, the
> documentation implies this, by saying "Copy the string", because bytes
How about documents it properly?
Obviously this new interface can confuse people...
> Plus strscpy_pad() needs to know the actual destination buffer size to
Oh boy.
IOW, before you use this function, ignore all the comments and read the source
completely and fully. Why am I even hoping for something better...
> know how many bytes to pad if you want to pad. If you give it a 3rd
> argument, then most of the time it's gonna be less then the size of the
> buffer, so it's not gonna do any padding at all, so it's just gonna be
> equivalent to strscpy() without _pad()
Lemme explain:
It should be written there explicitly so that whoever touches this code, can
go and *think* hard first what the source and destination are. Do they make
sense, what happens if she or he writes a string in there which is bigger than
RPMSG_NAME_SIZE - 1 and then wonders why this fancy strscpy truncates it?
In this particular case, it would be better to avoid this silly string copy
altogether.
Because obviously using that interface is not trivial.
So let's see what Shubhrajyoti finds out.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 13:39 ` Borislav Petkov
@ 2026-05-11 15:06 ` Andrei Purdea
0 siblings, 0 replies; 12+ messages in thread
From: Andrei Purdea @ 2026-05-11 15:06 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shubhrajyoti Datta, Geraldo Nascimento, david.laight.linux,
Alexander A. Klimov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
On Mon, May 11, 2026 at 1:39 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, May 11, 2026 at 01:13:05PM +0000, Andrei Purdea wrote:
> > But this wasn't spelled out explicitly in the doc, because you're not
> > really supposed to use strlen() to specify the destination buffer
> > size.
>
> Why am I not supposed to? Says who?
Maybe I was a little too vague in my statement, obviously there are
situations in which strlen() could be useful, for example:
If you want to append something to an existing string you could do
something like x = strlen(dst); strscpy(dst + x, src, sizeof(dst) -
x); (untested code)
Or if you want to explicitly truncate strings, say you want to copy
everything except the last 3 letters from the end of a string, you
could do something like strscpy(dst, src, min_t(size_t, sizeof(dst),
max_t(size_t, strlen(src) + 1 - 3, 1))); (untested code)
But in this case using plain strlen(src)+1 in strscpy() is redundant.
And using it in strscpy_pad() will make the padding functionality of
strscpy_pad() not function. Since you're the one who bought up padding
the rest of the buffer, I assumed you'd care that the buffer actually
gets padded.
> > Why would that be safer? There's already a strlen() call inside
> > strscpy.
>
> Do you mean this thing:
>
> sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst) + \
> ^^^^^^^^^^
> __must_be_cstr(dst) + __must_be_cstr(src))
>
> ?
No, I meant inside sized_strscpy (which is called by both strscpy and
strscpy_pad):
ssize_t sized_strscpy(char *dst, const char *src, size_t count)
{
size_t len;
if (count == 0)
return -E2BIG;
len = strnlen(src, count - 1);
^^^^^^^^^
In fact this is somewhat safer then plain strlen(), because it will be
somewhat bounded even if the source doesn't have a null terminator.
(although with the 2-argument strscpy() call the bound would be set by
the destination only)
If the source can be missing a null terminator, and you want to be
robust against a missing null-terminator, then you might want to do
something like strscpy(dst, src, min_t(size_t, sizeof(dst),
sizeof(src) + 1)), which would actually make things safer, and no need
to use strlen() explicitly in the call, because strscpy already takes
care of it.
> In this particular case, it would be better to avoid this silly string copy
> altogether.
Perhaps yes, IF the "error_ipc" string can not change at runtime, then
one could initialize it directly on the stack like:
struct rpmsg_channel_info chinfo = { .name = "error_ipc" };
And avoid all explicit calls to string copy functions. (Which I'm not
sure, but could compile to the same machine code as David Laight's
strcpy() suggestion, I would expect gcc to be smart enough to optimize
that)
But there's always gonna be some form of implicit string copy behind
the scenes, as long as the structure you're building has a string
array and lives on the stack.
Andrei
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix possible strscpy() buffer overflows
2026-05-11 12:51 ` Borislav Petkov
2026-05-11 13:13 ` Andrei Purdea
@ 2026-05-11 19:15 ` David Laight
1 sibling, 0 replies; 12+ messages in thread
From: David Laight @ 2026-05-11 19:15 UTC (permalink / raw)
To: Borislav Petkov
Cc: Andrei Purdea, Shubhrajyoti Datta, Geraldo Nascimento,
Alexander A. Klimov, Tony Luck, Kees Cook, Arnd Bergmann,
Greg Kroah-Hartman, Nick Li, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, linux-edac,
Linux Kernel Mailing List, linux-sound
On Mon, 11 May 2026 14:51:37 +0200
Borislav Petkov <bp@alien8.de> wrote:
> On Mon, May 11, 2026 at 11:59:34AM +0000, Andrei Purdea wrote:
> > No, the current code copies "error_ip" with null termination, and it
> > drops the "c" suffix.
>
> Pfff, that wasn't really clear to me from the explanation of strscpy...
>
> > And that seems buggy. And that's what I requested to explain the effects of.
It is a new(ish) driver.
It looks like just a bug.
Although the name itself looks strange.
>
> Shubhrajyoti, does that have any visible effects when using the driver?
>
> > strscpy_pad(chinfo.name, amd_rpmsg_id_table[0].name);
>
> No, as said "[h]owever, just to make this safer, we should min the size".
>
> IOW:
>
> strscpy_pad(chinfo.name,
> amd_rpmsg_id_table[0].name,
> min_t(size_t, strlen(amd_rpmsg_id_table[0].name) + 1, RPMSG_NAME_SIZE));
That is just pure crap.
strscpy(chinfo.name, amd_rpmsg_id_table[0].name);
will DTRT pretty much regardless of any obscurities - including the case where the
source array isn't '\0' terminated (although you might get a nasty run-time
error message if it is shorter than the destination).
Note that the inlined 'mess' generated for strscpy() in fortify_string.h isn't
ideal for all sorts of reasons.
(Not least because the 'hard' cases should be out of line.)
But that is an unrelated issue.
It would be better if amd_rpmsg_id_table[] were 'const' - in that case the strscpy()
call should 'degenerate' into a memcpy().
-- David
>
> In case someone goes and changes that amd_rpmsg_id_table[0].name in the
> future.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2026-05-11 19:16 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-10 18:24 [PATCH] Fix possible strscpy() buffer overflows Alexander A. Klimov
2026-05-10 22:08 ` David Laight
2026-05-11 1:30 ` Geraldo Nascimento
2026-05-11 6:46 ` Andrei Purdea
2026-05-11 10:38 ` Borislav Petkov
2026-05-11 11:58 ` David Laight
2026-05-11 11:59 ` Andrei Purdea
2026-05-11 12:51 ` Borislav Petkov
2026-05-11 13:13 ` Andrei Purdea
2026-05-11 13:39 ` Borislav Petkov
2026-05-11 15:06 ` Andrei Purdea
2026-05-11 19:15 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox