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

[Hans de Goede]

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 ?

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