diff mbox series

x86/platform/uv: refactor deprecated strcpy and strncpy

Message ID 20230822-strncpy-arch-x86-platform-uv-uv_nmi-v1-1-931f2943de0d@google.com (mailing list archive)
State Superseded
Headers show
Series x86/platform/uv: refactor deprecated strcpy and strncpy | expand

Commit Message

Justin Stitt Aug. 22, 2023, 10:32 p.m. UTC
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.

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@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only
---
 arch/x86/platform/uv/uv_nmi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)


---
base-commit: 706a741595047797872e669b3101429ab8d378ef
change-id: 20230822-strncpy-arch-x86-platform-uv-uv_nmi-474e5295c2c1

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Andy Shevchenko Aug. 23, 2023, 11:07 a.m. UTC | #1
On Wed, Aug 23, 2023 at 1:32 AM Justin Stitt <justinstitt@google.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.
Justin Stitt Aug. 23, 2023, 10:49 p.m. UTC | #2
On Wed, Aug 23, 2023 at 4:07 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Aug 23, 2023 at 1:32 AM Justin Stitt <justinstitt@google.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.

>
> --
> With Best Regards,
> Andy Shevchenko
Kees Cook Aug. 23, 2023, 11 p.m. UTC | #3
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@gmail.com> wrote:
> >
> > On Wed, Aug 23, 2023 at 1:32 AM Justin Stitt <justinstitt@google.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.
Dimitri Sivanich Aug. 24, 2023, 3:57 p.m. UTC | #4
On Wed, Aug 23, 2023 at 04:00:13PM -0700, Kees Cook wrote:
> 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@gmail.com> wrote:
> > >
> > > On Wed, Aug 23, 2023 at 1:32 AM Justin Stitt <justinstitt@google.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?!

The original code for the above (strcpy()), copies strlen(arg) assuming null
termination, so strlen(uv_nmi_action) is not correct for this case.  You
probably want to use sizeof of 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.
>

I agree.
diff mbox series

Patch

diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
index a60af0230e27..45d784143a13 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, ACTION_LEN);
 	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, strlen(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", strlen(uv_nmi_action));
 	}
 
 	/* Pause as all CPU's enter the NMI handler */