Re: [PATCH] printk: cleanup deprecated uses of strncpy/strcpy

[Kees Cook <keescook@chromium.org>]

On Mon, Apr 29, 2024 at 11:06:54PM +0000, Justin Stitt wrote:
> Cleanup some deprecated uses of strncpy() and strcpy() [1].
> 
> There doesn't seem to be any bugs with the current code but the
> readability of this code could benefit from a quick makeover while
> removing some deprecated stuff as a benefit.
> 
> The most interesting replacement made in this patch involves
> concatenating "ttyS" with a digit-led user-supplied string. Instead of
> doing two distinct string copies with carefully managed offsets and
> lengths, let's use the more robust and self-explanatory scnprintf().
> scnprintf will 1) respect the bounds of @buf, 2) null-terminate @buf, 3)
> do the concatenation. This allows us to drop the manual NUL-byte assignment.
> 
> Also, since isdigit() is used about a dozen lines after the open-coded
> version we'll replace it for uniformity's sake.
> 
> All the strcpy() --> strscpy() replacements are trivial as the source
> strings are literals and much smaller than the destination size. No
> behavioral change here.
> 
> Use the new 2-argument version of strscpy() introduced in Commit
> e6584c3964f2f ("string: Allow 2-argument strscpy()"). However, to make
> this work fully (since the size must be known at compile time), also
> update the extern-qualified declaration to have the proper size
> information.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90 [2]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> ---
>  include/linux/printk.h |  2 +-
>  kernel/printk/printk.c | 20 +++++++++-----------
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 955e31860095..b3a29c27abe9 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -71,7 +71,7 @@ extern void console_verbose(void);
>  
>  /* strlen("ratelimit") + 1 */
>  #define DEVKMSG_STR_MAX_SIZE 10
> -extern char devkmsg_log_str[];
> +extern char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE];
>  struct ctl_table;
>  
>  extern int suppress_printk;
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index adf99c05adca..64617bcda070 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -178,9 +178,9 @@ static int __init control_devkmsg(char *str)
>  	 * Set sysctl string accordingly:
>  	 */
>  	if (devkmsg_log == DEVKMSG_LOG_MASK_ON)
> -		strcpy(devkmsg_log_str, "on");
> +		strscpy(devkmsg_log_str, "on");
>  	else if (devkmsg_log == DEVKMSG_LOG_MASK_OFF)
> -		strcpy(devkmsg_log_str, "off");
> +		strscpy(devkmsg_log_str, "off");
>  	/* else "ratelimit" which is set by default. */
>  
>  	/*
> @@ -209,7 +209,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  			return -EINVAL;
>  
>  		old = devkmsg_log;
> -		strncpy(old_str, devkmsg_log_str, DEVKMSG_STR_MAX_SIZE);
> +		strscpy(old_str, devkmsg_log_str);
>  	}
>  
>  	err = proc_dostring(table, write, buffer, lenp, ppos);
> @@ -227,7 +227,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  
>  			/* ... and restore old setting. */
>  			devkmsg_log = old;
> -			strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
> +			strscpy(devkmsg_log_str, old_str);
>  
>  			return -EINVAL;
>  		}
> @@ -2506,21 +2506,19 @@ static int __init console_setup(char *str)
>  	/*
>  	 * Decode str into name, index, options.
>  	 */
> -	if (str[0] >= '0' && str[0] <= '9') {
> -		strcpy(buf, "ttyS");
> -		strncpy(buf + 4, str, sizeof(buf) - 5);
> +	if (isdigit(str[0])) {
> +		scnprintf(buf, sizeof(buf), "ttyS%s", str);
>  	} else {
> -		strncpy(buf, str, sizeof(buf) - 1);
> +		strscpy(buf, str);
>  	}
> -	buf[sizeof(buf) - 1] = 0;
>  	options = strchr(str, ',');
>  	if (options)
>  		*(options++) = 0;
>  #ifdef __sparc__
>  	if (!strcmp(str, "ttya"))
> -		strcpy(buf, "ttyS0");
> +		strscpy(buf, "ttyS0");
>  	if (!strcmp(str, "ttyb"))
> -		strcpy(buf, "ttyS1");
> +		strscpy(buf, "ttyS1");
>  #endif
>  	for (s = buf; *s; s++)
>  		if (isdigit(*s) || *s == ',')
> 
> ---
> base-commit: 9e4bc4bcae012c98964c3c2010debfbd9e5b229f
> change-id: 20240429-strncpy-kernel-printk-printk-c-6a72fe6d0715
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>

Yeah, everything here checks out. I had to read through the sysctl
handler pretty carefully, but I think this is a nice readability
improvement. Thanks!

-Kees

-- 
Kees Cook