| Message ID | 20230811-arch-s390-kernel-v1-1-7edbeeab3809@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | s390/ipl: refactor deprecated strncpy | expand |
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
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.
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!
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; \ } \
`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(-) --- base-commit: 52a93d39b17dc7eb98b6aa3edb93943248e03b2f change-id: 20230811-arch-s390-kernel-968d0545c45a Best regards, -- Justin Stitt <justinstitt@google.com>