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

[Andrei Purdea]

On Mon, May 11, 2026 at 1:39 PM Borislav Petkov <bp@xxxxxxxxx> 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