On Tue, Sep 5, 2023 at 3:52 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi Justin, > > On 8/24/23 20:52, Justin Stitt wrote: > > Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated > > destination strings [1]. > > > > 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. > > > > Also include slight refactor to code removing possible new-line chars as > > per Yang Yang's work at [3]. This reduces code size and complexity by > > using more robust and better understood interfaces. > > > > 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://lore.kernel.org/all/202212091545310085328@xxxxxxxxxx/ [3] > > Link: https://github.com/KSPP/linux/issues/90 > > Cc: linux-hardening@xxxxxxxxxxxxxxx > > Co-developed-by: Yang Yang <yang.yang29@xxxxxxxxxx> > > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > > --- > > 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 > > > > Another thing, Yang Yang's patch [3] had some review from Andy regarding > > the use of `-1` and `+1` in and around the strnchrnul invocation. I > > believe Yang Yang's original implementation is correct but let's also > > just use sizeof(arg) instead of ACTION_LEN. > > > > Here's a godbolt link detailing some findings around the new-line > > refactor in response to Andy's feedback: https://godbolt.org/z/K8drG3oq5 > > --- > > arch/x86/platform/uv/uv_nmi.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c > > index a60af0230e27..913347b2b9ab 100644 > > --- a/arch/x86/platform/uv/uv_nmi.c > > +++ b/arch/x86/platform/uv/uv_nmi.c > > @@ -202,21 +202,17 @@ static int param_set_action(const char *val, const struct kernel_param *kp) > > { > > int i; > > int n = ARRAY_SIZE(valid_acts); > > - char arg[ACTION_LEN], *p; > > + char arg[ACTION_LEN]; > > > > /* (remove possible '\n') */ > > - strncpy(arg, val, ACTION_LEN - 1); > > - arg[ACTION_LEN - 1] = '\0'; > > - p = strchr(arg, '\n'); > > - if (p) > > - *p = '\0'; > > + strscpy(arg, val, strnchrnul(val, sizeof(arg) - 1, '\n') - val + 1); > > I have 25 years of C-programming experience and even I > cannot read this. > > It seems to me that you are trying to use the length > argument to not copy the '\n' here. > > While at the same time using strnchr(..., sizeof(arg) ...) > instead of normal strchr() to make sure you don't pass\ > a value bigger then sizeof(arg) as length to strscpy(). > > Please do not do this it is needlessly complicated and > makes the code almost impossible to read / reason about. > > What the original code was doing, first copying at > most ACTION_LEN - 1 bytes into arg and then ensuring > 0 termination, followed by stripping '\n' from the > writable copy we have just made is much cleaner. > > IMHO this patch should simple replace the strncpy() > + 0 termination with a strscpy() and not make > any other changes, leading to: > > /* (remove possible '\n') */ > strscpy(arg, val, sizeof(arg)); > p = strchr(arg, '\n'); > if (p) > *p = '\0'; > > See how this is much much more readable / > much easier to wrap ones mind around ? Right, I agree. This was basically the v1 of my patch. I will send a v3 with feedback implemented. > > And then as a *separate* followup patch > you could simplify this further by using strchrnul(): > > /* (remove possible '\n') */ > strscpy(arg, val, sizeof(arg)); > p = strchrnul(arg, '\n'); > *p = '\0'; > > But again that belongs in a separate patch > since it is not: > > "refactor deprecated strcpy and strncpy" > > Regards, > > Hans > > > > > > > > > > for (i = 0; i < n; i++) > > if (!strcmp(arg, valid_acts[i].action)) > > 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 +955,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: 706a741595047797872e669b3101429ab8d378ef > > change-id: 20230822-strncpy-arch-x86-platform-uv-uv_nmi-474e5295c2c1 > > > > Best regards, > > -- > > Justin Stitt <justinstitt@xxxxxxxxxx> > > >
