[PATCH] s390/ipl: refactor deprecated strncpy

[PATCH] s390/ipl: refactor deprecated strncpy

[Justin Stitt]

`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(-)

diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
index 85a00d97a314..a6dcf6f28197 100644
--- a/arch/s390/kernel/ipl.c
+++ b/arch/s390/kernel/ipl.c
@@ -266,7 +266,7 @@ static ssize_t sys_##_prefix##_##_name##_store(struct kobject *kobj,	\
 		struct kobj_attribute *attr,				\
 		const char *buf, size_t len)				\
 {									\
-	strncpy(_value, buf, sizeof(_value) - 1);			\
+	strscpy_pad(_value, buf, sizeof(_value));			\
 	strim(_value);							\
 	return len;							\
 }									\

---
base-commit: 52a93d39b17dc7eb98b6aa3edb93943248e03b2f
change-id: 20230811-arch-s390-kernel-968d0545c45a

Best regards,
--
Justin Stitt <justinstitt@google.com>

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

[Kees Cook]

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(-)
> 
> diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
> index 85a00d97a314..a6dcf6f28197 100644
> --- a/arch/s390/kernel/ipl.c
> +++ b/arch/s390/kernel/ipl.c
> @@ -266,7 +266,7 @@ static ssize_t sys_##_prefix##_##_name##_store(struct kobject *kobj,	\
>  		struct kobj_attribute *attr,				\
>  		const char *buf, size_t len)				\
>  {									\
> -	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 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.)

-Kees

-- 
Kees Cook

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

[Heiko Carstens]

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.

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

[Heiko Carstens]

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(-)
> 
> diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
> index 85a00d97a314..a6dcf6f28197 100644
> --- a/arch/s390/kernel/ipl.c
> +++ b/arch/s390/kernel/ipl.c
> @@ -266,7 +266,7 @@ static ssize_t sys_##_prefix##_##_name##_store(struct kobject *kobj,	\
>  		struct kobj_attribute *attr,				\
>  		const char *buf, size_t len)				\
>  {									\
> -	strncpy(_value, buf, sizeof(_value) - 1);			\
> +	strscpy_pad(_value, buf, sizeof(_value));			\
>  	strim(_value);							\
>  	return len;							\
>  }									\

Patch applied, but replaced strscpy_pad() with strscpy() like it was
suggested by Kees.

Thanks!