On October 15, 2023 10:47:53 PM PDT, Dan Carpenter [off-list ref] wrote:

On Sun, Oct 15, 2023 at 05:06:19PM +0200, Simon Horman wrote:

quoted

On Thu, Oct 12, 2023 at 09:33:32PM +0000, Justin Stitt wrote:

quoted

strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.

We expect both hi.data.modename and hi.data.drivername to be
NUL-terminated but not necessarily NUL-padded which is evident by its
usage with sprintf:
|       sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s",
|               bc->cfg.intclk ? "int" : "ext",
|               bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps,
|               bc->cfg.loopback ? ",loopback" : "");

Note that this data is copied out to userspace with:
|       if (copy_to_user(data, &hi, sizeof(hi)))
... however, the data was also copied FROM the user here:
|       if (copy_from_user(&hi, data, sizeof(hi)))

Thanks Justin,

I see that too.

Perhaps I am off the mark here, and perhaps it's out of scope for this
patch, but I do think it would be nicer if the kernel only sent
intended data to user-space, even if any unintended payload came
from user-space.

It's kind of normal to pass user space data back to itself.  We
generally only worry about info leaks.

True but since this used to zero the rest of the buffet, let's just keep that behavior and use strscpy_pad().

-Kees

-- 
Kees Cook