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

[Andrei Purdea]

> 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