Hi, On 9/5/23 23:54, Justin Stitt wrote: > Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated > destination strings [1]. > > We can see that `arg` and `uv_nmi_action` are expected to be > NUL-terminated strings due to their use within `strcmp()` and format > strings respectively. > > With this in mind, a suitable replacement is `strscpy` [2] due to the > fact that it guarantees NUL-termination on its destination buffer > argument which is _not_ the case for `strncpy` or `strcpy`! > > In this case, we can drop both the forced NUL-termination and the `... -1` from: > | strncpy(arg, val, ACTION_LEN - 1); > as `strscpy` implicitly has this behavior. > > Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@xxxxxxxxxxxxxxx > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > --- > Changes in v3: > - Use sizeof instead of strlen (thanks Andy and Dimitri) > - Drop unrelated changes regarding strnchrnul (thanks Hans) > - Link to v2: https://lore.kernel.org/r/20230824-strncpy-arch-x86-platform-uv-uv_nmi-v2-1-e16d9a3ec570@xxxxxxxxxx Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx> Regards, Hans > > Changes in v2: > - use `sizeof` on destination string instead of `strlen` (thanks Andy, Kees and Dimitri) > - refactor code to remove potential new-line chars (thanks Yang Yang and Andy) > - Link to v1: https://lore.kernel.org/r/20230822-strncpy-arch-x86-platform-uv-uv_nmi-v1-1-931f2943de0d@xxxxxxxxxx > --- > Note: build-tested only > --- > arch/x86/platform/uv/uv_nmi.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c > index a60af0230e27..dd30fb2baf6c 100644 > --- a/arch/x86/platform/uv/uv_nmi.c > +++ b/arch/x86/platform/uv/uv_nmi.c > @@ -205,8 +205,7 @@ static int param_set_action(const char *val, const struct kernel_param *kp) > char arg[ACTION_LEN], *p; > > /* (remove possible '\n') */ > - strncpy(arg, val, ACTION_LEN - 1); > - arg[ACTION_LEN - 1] = '\0'; > + strscpy(arg, val, sizeof(arg)); > p = strchr(arg, '\n'); > if (p) > *p = '\0'; > @@ -216,7 +215,7 @@ static int param_set_action(const char *val, const struct kernel_param *kp) > break; > > if (i < n) { > - strcpy(uv_nmi_action, arg); > + strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action)); > pr_info("UV: New NMI action:%s\n", uv_nmi_action); > return 0; > } > @@ -959,7 +958,7 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs) > > /* Unexpected return, revert action to "dump" */ > if (master) > - strncpy(uv_nmi_action, "dump", strlen(uv_nmi_action)); > + strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action)); > } > > /* Pause as all CPU's enter the NMI handler */ > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230822-strncpy-arch-x86-platform-uv-uv_nmi-474e5295c2c1 > > Best regards, > -- > Justin Stitt <justinstitt@xxxxxxxxxx> >
