Re: [PATCH v2] x86/platform/uv: refactor deprecated strcpy and strncpy

[Justin Stitt]

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>
> >
>