linux-kernel - Re: [PATCH] x86/platform/uv: refactor deprecated strcpy and strncpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202308231554.4873EE731@keescook>
Date:   Wed, 23 Aug 2023 16:00:13 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Justin Stitt <justinstitt@...gle.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Steve Wahl <steve.wahl@....com>,
        Mike Travis <mike.travis@....com>,
        Dimitri Sivanich <dimitri.sivanich@....com>,
        Russ Anderson <russ.anderson@....com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] x86/platform/uv: refactor deprecated strcpy and strncpy

On Wed, Aug 23, 2023 at 03:49:34PM -0700, Justin Stitt wrote:
> On Wed, Aug 23, 2023 at 4:07 AM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> >
> > On Wed, Aug 23, 2023 at 1:32 AM Justin Stitt <justinstitt@...gle.com> 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.
> >
> > ...
> >
> > >         char arg[ACTION_LEN], *p;
> > >
> > >         /* (remove possible '\n') */
> > > -       strncpy(arg, val, ACTION_LEN - 1);
> > > -       arg[ACTION_LEN - 1] = '\0';
> > > +       strscpy(arg, val, ACTION_LEN);
> > >         p = strchr(arg, '\n');
> > >         if (p)
> > >                 *p = '\0';
> >
> > https://lore.kernel.org/all/202212091545310085328@zte.com.cn/
> >
> > ...
> >
> > > +               strscpy(uv_nmi_action, arg, strlen(uv_nmi_action));
> >
> > strlen() on the destination?!
> >
> > ...
> >
> > > -                       strncpy(uv_nmi_action, "dump", strlen(uv_nmi_action));
> > > +                       strscpy(uv_nmi_action, "dump", strlen(uv_nmi_action));
> >
> > Again, this is weird.
> 
> This is a common pattern with `strxcpy` and `sizeof` if you `$ rg
> "strncpy\(.*sizeof"`. Do you recommend I switch the strlen(dest) to
> strlen(src)? I only kept as-is because that's what was there
> originally and I assumed some greater purpose of it.

It's best to avoid any assumptions. If it can't be answered through code
inspection, the next best thing would be to ask for clarification. In
looking I see uv_nmi_action is a string:

arch/x86/platform/uv/uv_nmi.c:193:typedef char action_t[ACTION_LEN];

arch/x86/platform/uv/uv_nmi.c:          strcpy(uv_nmi_action, arg);
arch/x86/platform/uv/uv_nmi.c:module_param_named(action, uv_nmi_action, action, 0644);
arch/x86/platform/uv/uv_nmi.c:  return (strncmp(uv_nmi_action, action, strlen(action)) == 0);
arch/x86/platform/uv/uv_nmi.c:                  strncpy(uv_nmi_action, "dump", strlen(uv_nmi_action));

using strlen() here seems "accidentally safe", as it's overwriting
"kdump":

        if (uv_nmi_action_is("kdump")) {
                uv_nmi_kdump(cpu, master, regs);

                /* Unexpected return, revert action to "dump" */
                if (master)
                        strncpy(uv_nmi_action, "dump", strlen(uv_nmi_action));

anyway, a simple "sizeof" should be used AFAICT.

-- 
Kees Cook