Re: [PATCH] s390/ipl: refactor deprecated strncpy

[Heiko Carstens <hca@linux.ibm.com>]

On Mon, Aug 14, 2023 at 04:32:11PM -0700, Kees Cook wrote:
> On Fri, Aug 11, 2023 at 09:56:15PM +0000, Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> > 
> > Use `strscpy_pad` which has the same behavior as `strncpy` here with the
> > extra safeguard of guaranteeing NUL-termination of destination strings.
> > In it's current form, this may result in silent truncation if the src
> > string has the same size as the destination string.
> > 
> > Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> >  arch/s390/kernel/ipl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
...
> > -	strncpy(_value, buf, sizeof(_value) - 1);			\
> > +	strscpy_pad(_value, buf, sizeof(_value));			\
> 
> Padding isn't needed here -- the string are consumed by __cpcmd(), which
> explicitly uses strlen() and a memcpy to pass them off.
> 
> >  	strim(_value);							\
> 
> This existing code line is buggy, though -- it will not trim leading
> whitespace in the buffer. (It _returns_ a string that has been
> forward-adjusted.)

I'm quite sure that was intentional, so only whitespace at the end of
the string, in particular '\n', will be stripped.
But then again this code is 15 years old...

> I think this is an API mistake -- strim() should either do in-place
> changes for both ends or be defined with __much_check so the return
> value doesn't get lost. (But this is a separate issue.)

For __must_check see strstrip() which was actively avoided with commit
1d802e24774c ("[S390] Use strim instead of strstrip to avoid false
warnings.") many years ago.

For all converted usages of strstrip() by that commit it was indeed
intended to remove only trailing whitespace, where the above code snipped
is the only piece of code which is questionable, since it is user space
provided input; however I don't think this should be changed now.