diff mbox series

printk: cleanup deprecated uses of strncpy/strcpy

Message ID 20240429-strncpy-kernel-printk-printk-c-v1-1-4da7926d7b69@google.com (mailing list archive)
State Mainlined
Commit e0550222e03bae3fd629641e246ef7f47803d795
Headers show
Series printk: cleanup deprecated uses of strncpy/strcpy | expand

Commit Message

Justin Stitt April 29, 2024, 11:06 p.m. UTC 
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(-)


---
base-commit: 9e4bc4bcae012c98964c3c2010debfbd9e5b229f
change-id: 20240429-strncpy-kernel-printk-printk-c-6a72fe6d0715

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

Comments

Kees Cook May 1, 2024, 9:08 p.m. UTC | #1 
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
Christophe JAILLET May 1, 2024, 9:39 p.m. UTC | #2 
Le 30/04/2024 à 01:06, Justin Stitt a écrit :
> 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);
>   	}

Hi,

Nit: The { } around each branch can now also be removed.

CJ

> -	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>
> 
> 
>
Justin Stitt May 1, 2024, 11:18 p.m. UTC | #3 
On Wed, May 1, 2024 at 2:39 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Hi,
>
> Nit: The { } around each branch can now also be removed.

There was one line before and there's one line now.

I'll remove the brackets but I will briefly wait to see if any other
concerns come in.

Thanks

>
> CJ
>
Christophe JAILLET May 2, 2024, 5:06 a.m. UTC | #4 
Le 02/05/2024 à 01:18, Justin Stitt a écrit :
> On Wed, May 1, 2024 at 2:39 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>> Hi,
>>
>> Nit: The { } around each branch can now also be removed.
> 
> There was one line before and there's one line now.

In the block after the "else", yes, but now the block after the "if" is 
only 1 line. (it was 2 before).

So, {} should now be omitted on both branches.

-    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);
       }

This is a really minor nitpick. Not sure you need to repost if there is 
no other comment.

CJ

> 
> I'll remove the brackets but I will briefly wait to see if any other
> concerns come in.
> 
> Thanks
> 
>>
>> CJ
>>
> 
>
Petr Mladek May 2, 2024, 3:07 p.m. UTC | #5 
On Mon 2024-04-29 23:06:54, 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>

Nice improvements. Looks fine.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr
Petr Mladek May 2, 2024, 3:14 p.m. UTC | #6 
On Thu 2024-05-02 07:06:21, Christophe JAILLET wrote:
> Le 02/05/2024 à 01:18, Justin Stitt a écrit :
> > On Wed, May 1, 2024 at 2:39 PM Christophe JAILLET
> > <christophe.jaillet@wanadoo.fr> wrote:
> > > Hi,
> > > 
> > > Nit: The { } around each branch can now also be removed.
> > 
> > There was one line before and there's one line now.
> 
> In the block after the "else", yes, but now the block after the "if" is only
> 1 line. (it was 2 before).
> 
> So, {} should now be omitted on both branches.
> 
> -    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);
>       }
> 
> This is a really minor nitpick. Not sure you need to repost if there is no
> other comment.

I could remove the brackets when pushing the patch. But feel free to
send v2.

I am going to push it the following week if nobody complains.

Best Regards,
Petr
Petr Mladek May 7, 2024, 9:55 a.m. UTC | #7 
On Mon 2024-04-29 23:06:54, 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>

JFYI, the patch has been comitted into printk/linux.git, branch for-6.10.

I have removed the obsoleted brackets and added some empty lines
to break the blob of code a bit, see
https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/commit/?h=for-6.10&id=e0550222e03bae3fd629641e246ef7f47803d795

Best Regards,
Petr
diff mbox series

Patch

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 == ',')