diff mbox series

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

Message ID 20230824-strncpy-arch-x86-platform-uv-uv_nmi-v2-1-e16d9a3ec570@google.com (mailing list archive)
State Superseded
Headers show
Series [v2] x86/platform/uv: refactor deprecated strcpy and strncpy | expand

Commit Message

Justin Stitt Aug. 24, 2023, 6:52 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.

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@zte.com.cn/ [3]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Co-developed-by: Yang Yang <yang.yang29@zte.com.cn>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
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@google.com
---
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(-)


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

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

Comments

Hans de Goede Sept. 5, 2023, 10:52 a.m. UTC | #1 
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@zte.com.cn/ [3]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Co-developed-by: Yang Yang <yang.yang29@zte.com.cn>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> 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@google.com
> ---
> 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 ?

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@google.com>
>
Justin Stitt Sept. 5, 2023, 9:40 p.m. UTC | #2 
On Tue, Sep 5, 2023 at 3:52 AM Hans de Goede <hdegoede@redhat.com> 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@zte.com.cn/ [3]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Co-developed-by: Yang Yang <yang.yang29@zte.com.cn>
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> > 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@google.com
> > ---
> > 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@google.com>
> >
>
diff mbox series

Patch

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);
 
 	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 */