Re: [PATCH] Fix possible strscpy() buffer overflows

[David Laight]

On Mon, 11 May 2026 12:38:54 +0200
Borislav Petkov <bp@xxxxxxxxx> 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.
>