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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
---
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 <[email protected]>
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> ---
> 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 <[email protected]>
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
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> ---
> 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 <[email protected]>
>
>
>
On Wed, May 1, 2024 at 2:39 PM Christophe JAILLET
<[email protected]> 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
>
Le 02/05/2024 à 01:18, Justin Stitt a écrit :
> On Wed, May 1, 2024 at 2:39 PM Christophe JAILLET
> <[email protected]> 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
>>
>
>
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
Nice improvements. Looks fine.
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
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
> > <[email protected]> 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
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
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