* [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() @ 2026-06-04 13:17 Thorsten Blum 2026-06-05 4:41 ` Andy Shevchenko 0 siblings, 1 reply; 7+ messages in thread From: Thorsten Blum @ 2026-06-04 13:17 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Andrew Morton, Ard Biesheuvel, Mike Rapoport (Microsoft), Thomas Zimmermann, Arnd Bergmann, Jiri Bohac, Harshit Mogalapalli Cc: Andy Shevchenko, linux-hardening, Thorsten Blum, Ingo Molnar, linux-kernel In preparation to remove strlcat() from the kernel [1], replace two strlcat() calls with one snprintf() call in setup_arch(). Also drop the explicit size argument of strscpy() to further simplify the code since strscpy() can determine the size automatically when the destination buffer has a fixed length. [1] https://github.com/KSPP/linux/issues/370 Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev> --- arch/x86/kernel/setup.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 46882ce79c3a..b11b0ce31a27 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -21,6 +21,7 @@ #include <linux/pci.h> #include <linux/random.h> #include <linux/root_dev.h> +#include <linux/sprintf.h> #include <linux/static_call.h> #include <linux/sysfb.h> #include <linux/swiotlb.h> @@ -915,16 +916,18 @@ void __init setup_arch(char **cmdline_p) strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); #else if (builtin_cmdline[0]) { + size_t len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); + /* append boot loader cmdline to builtin */ - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); + snprintf(builtin_cmdline + len, COMMAND_LINE_SIZE - len, " %s", + boot_command_line); strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); } #endif builtin_cmdline_added = true; #endif - strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE); + strscpy(command_line, boot_command_line); *cmdline_p = command_line; /* ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() 2026-06-04 13:17 [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() Thorsten Blum @ 2026-06-05 4:41 ` Andy Shevchenko 2026-06-05 15:42 ` Thorsten Blum 0 siblings, 1 reply; 7+ messages in thread From: Andy Shevchenko @ 2026-06-05 4:41 UTC (permalink / raw) To: Thorsten Blum Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Andrew Morton, Ard Biesheuvel, Mike Rapoport (Microsoft), Thomas Zimmermann, Arnd Bergmann, Jiri Bohac, Harshit Mogalapalli, linux-hardening, Ingo Molnar, linux-kernel On Thu, Jun 04, 2026 at 03:17:53PM +0200, Thorsten Blum wrote: > In preparation to remove strlcat() from the kernel [1], replace two > strlcat() calls with one snprintf() call in setup_arch(). > > Also drop the explicit size argument of strscpy() to further simplify > the code since strscpy() can determine the size automatically when the > destination buffer has a fixed length. > [1] https://github.com/KSPP/linux/issues/370 Make it Link tag? ... > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); This also has third argument fixed. Don't you want to change that? > #else > if (builtin_cmdline[0]) { > + size_t len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); > + > /* append boot loader cmdline to builtin */ > - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > + snprintf(builtin_cmdline + len, COMMAND_LINE_SIZE - len, " %s", > + boot_command_line); Hmm... Wouldn't GCC complain on this? (Build with `make W=1`.) > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); And this has also third argument. > } > #endif > builtin_cmdline_added = true; > #endif > > - strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE); > + strscpy(command_line, boot_command_line); -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() 2026-06-05 4:41 ` Andy Shevchenko @ 2026-06-05 15:42 ` Thorsten Blum 2026-06-05 15:55 ` Andy Shevchenko 0 siblings, 1 reply; 7+ messages in thread From: Thorsten Blum @ 2026-06-05 15:42 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Andrew Morton, Ard Biesheuvel, Mike Rapoport (Microsoft), Thomas Zimmermann, Arnd Bergmann, Jiri Bohac, Harshit Mogalapalli, linux-hardening, Ingo Molnar, linux-kernel On Fri, Jun 05, 2026 at 07:41:11AM +0300, Andy Shevchenko wrote: > On Thu, Jun 04, 2026 at 03:17:53PM +0200, Thorsten Blum wrote: > > In preparation to remove strlcat() from the kernel [1], replace two > > strlcat() calls with one snprintf() call in setup_arch(). > > > > Also drop the explicit size argument of strscpy() to further simplify > > the code since strscpy() can determine the size automatically when the > > destination buffer has a fixed length. > > > [1] https://github.com/KSPP/linux/issues/370 > > Make it Link tag? > > ... > > > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > > This also has third argument fixed. Don't you want to change that? That doesn't work because boot_command_line, at least the declaration in linux/init.h, doesn't have a fixed size. > > #else > > if (builtin_cmdline[0]) { > > + size_t len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); > > + > > /* append boot loader cmdline to builtin */ > > - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > > - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > > + snprintf(builtin_cmdline + len, COMMAND_LINE_SIZE - len, " %s", > > + boot_command_line); > > Hmm... Wouldn't GCC complain on this? (Build with `make W=1`.) No warnings with W=1. Why would GCC warn here? > > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > > And this has also third argument. Same reason as above. Thanks, Thorsten ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() 2026-06-05 15:42 ` Thorsten Blum @ 2026-06-05 15:55 ` Andy Shevchenko 2026-06-05 18:05 ` Thorsten Blum 0 siblings, 1 reply; 7+ messages in thread From: Andy Shevchenko @ 2026-06-05 15:55 UTC (permalink / raw) To: Thorsten Blum Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Andrew Morton, Ard Biesheuvel, Mike Rapoport (Microsoft), Thomas Zimmermann, Arnd Bergmann, Jiri Bohac, Harshit Mogalapalli, linux-hardening, Ingo Molnar, linux-kernel On Fri, Jun 05, 2026 at 05:42:48PM +0200, Thorsten Blum wrote: > On Fri, Jun 05, 2026 at 07:41:11AM +0300, Andy Shevchenko wrote: > > On Thu, Jun 04, 2026 at 03:17:53PM +0200, Thorsten Blum wrote: ... > > > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > > > > This also has third argument fixed. Don't you want to change that? > > That doesn't work because boot_command_line, at least the declaration in > linux/init.h, doesn't have a fixed size. Ah, okay. > > > #else > > > if (builtin_cmdline[0]) { > > > + size_t len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); > > > + > > > /* append boot loader cmdline to builtin */ > > > - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > > > - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > > > + snprintf(builtin_cmdline + len, COMMAND_LINE_SIZE - len, " %s", > > > + boot_command_line); > > > > Hmm... Wouldn't GCC complain on this? (Build with `make W=1`.) > > No warnings with W=1. Why would GCC warn here? Sometimes it complains if it can't prove the size of the string to fit the destination. You said that there is no size for boot_command_line, I'm not sure I understand how GCC proves that the above snprintf() won't ever truncate the input. > > > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > > > > And this has also third argument. > > Same reason as above. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() 2026-06-05 15:55 ` Andy Shevchenko @ 2026-06-05 18:05 ` Thorsten Blum 2026-06-05 18:28 ` Andy Shevchenko 0 siblings, 1 reply; 7+ messages in thread From: Thorsten Blum @ 2026-06-05 18:05 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Andrew Morton, Ard Biesheuvel, Mike Rapoport (Microsoft), Thomas Zimmermann, Arnd Bergmann, Jiri Bohac, Harshit Mogalapalli, linux-hardening, Ingo Molnar, linux-kernel On Fri, Jun 05, 2026 at 06:55:31PM +0300, Andy Shevchenko wrote: > On Fri, Jun 05, 2026 at 05:42:48PM +0200, Thorsten Blum wrote: > > On Fri, Jun 05, 2026 at 07:41:11AM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 04, 2026 at 03:17:53PM +0200, Thorsten Blum wrote: > > ... > > > > > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > > > > > > This also has third argument fixed. Don't you want to change that? > > > > That doesn't work because boot_command_line, at least the declaration in > > linux/init.h, doesn't have a fixed size. > > Ah, okay. > > > > > #else > > > > if (builtin_cmdline[0]) { > > > > + size_t len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); > > > > + > > > > /* append boot loader cmdline to builtin */ > > > > - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > > > > - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > > > > + snprintf(builtin_cmdline + len, COMMAND_LINE_SIZE - len, " %s", > > > > + boot_command_line); > > > > > > Hmm... Wouldn't GCC complain on this? (Build with `make W=1`.) > > > > No warnings with W=1. Why would GCC warn here? > > Sometimes it complains if it can't prove the size of the string to fit the > destination. You said that there is no size for boot_command_line, I'm not > sure I understand how GCC proves that the above snprintf() won't ever truncate > the input. The compiler doesn't prove that this cannot truncate. It only knows the buffer sizes, but not the runtime string lengths. snprintf() can truncate, and its return value could be used to detect that. However, the previous version also ignored possible truncation by strlcat(), so I didn't add new truncation handling. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() 2026-06-05 18:05 ` Thorsten Blum @ 2026-06-05 18:28 ` Andy Shevchenko 2026-06-06 10:12 ` David Laight 0 siblings, 1 reply; 7+ messages in thread From: Andy Shevchenko @ 2026-06-05 18:28 UTC (permalink / raw) To: Thorsten Blum Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Andrew Morton, Ard Biesheuvel, Mike Rapoport (Microsoft), Thomas Zimmermann, Arnd Bergmann, Jiri Bohac, Harshit Mogalapalli, linux-hardening, Ingo Molnar, linux-kernel On Fri, Jun 05, 2026 at 08:05:21PM +0200, Thorsten Blum wrote: > On Fri, Jun 05, 2026 at 06:55:31PM +0300, Andy Shevchenko wrote: > > On Fri, Jun 05, 2026 at 05:42:48PM +0200, Thorsten Blum wrote: > > > On Fri, Jun 05, 2026 at 07:41:11AM +0300, Andy Shevchenko wrote: > > > > On Thu, Jun 04, 2026 at 03:17:53PM +0200, Thorsten Blum wrote: ... > > > > > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > > > > > > > > This also has third argument fixed. Don't you want to change that? > > > > > > That doesn't work because boot_command_line, at least the declaration in > > > linux/init.h, doesn't have a fixed size. > > > > Ah, okay. > > > > > > > #else > > > > > if (builtin_cmdline[0]) { > > > > > + size_t len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); > > > > > + > > > > > /* append boot loader cmdline to builtin */ > > > > > - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > > > > > - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > > > > > + snprintf(builtin_cmdline + len, COMMAND_LINE_SIZE - len, " %s", > > > > > + boot_command_line); > > > > > > > > Hmm... Wouldn't GCC complain on this? (Build with `make W=1`.) > > > > > > No warnings with W=1. Why would GCC warn here? > > > > Sometimes it complains if it can't prove the size of the string to fit the > > destination. You said that there is no size for boot_command_line, I'm not > > sure I understand how GCC proves that the above snprintf() won't ever truncate > > the input. > > The compiler doesn't prove that this cannot truncate. It only knows the > buffer sizes, but not the runtime string lengths. > > snprintf() can truncate, and its return value could be used to detect > that. However, the previous version also ignored possible truncation by > strlcat(), so I didn't add new truncation handling. I understand that, but AFAIK strlcat() doesn't induce a warning in such a case, while GCC does (or at least should). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() 2026-06-05 18:28 ` Andy Shevchenko @ 2026-06-06 10:12 ` David Laight 0 siblings, 0 replies; 7+ messages in thread From: David Laight @ 2026-06-06 10:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Thorsten Blum, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Andrew Morton, Ard Biesheuvel, Mike Rapoport (Microsoft), Thomas Zimmermann, Arnd Bergmann, Jiri Bohac, Harshit Mogalapalli, linux-hardening, Ingo Molnar, linux-kernel On Fri, 5 Jun 2026 21:28:50 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Jun 05, 2026 at 08:05:21PM +0200, Thorsten Blum wrote: > > On Fri, Jun 05, 2026 at 06:55:31PM +0300, Andy Shevchenko wrote: > > > On Fri, Jun 05, 2026 at 05:42:48PM +0200, Thorsten Blum wrote: > > > > On Fri, Jun 05, 2026 at 07:41:11AM +0300, Andy Shevchenko wrote: > > > > > On Thu, Jun 04, 2026 at 03:17:53PM +0200, Thorsten Blum wrote: > > ... > > > > > > > strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > > > > > > > > > > This also has third argument fixed. Don't you want to change that? > > > > > > > > That doesn't work because boot_command_line, at least the declaration in > > > > linux/init.h, doesn't have a fixed size. > > > > > > Ah, okay. > > > > > > > > > #else > > > > > > if (builtin_cmdline[0]) { > > > > > > + size_t len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); > > > > > > + > > > > > > /* append boot loader cmdline to builtin */ > > > > > > - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > > > > > > - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > > > > > > + snprintf(builtin_cmdline + len, COMMAND_LINE_SIZE - len, " %s", > > > > > > + boot_command_line); > > > > > > > > > > Hmm... Wouldn't GCC complain on this? (Build with `make W=1`.) > > > > > > > > No warnings with W=1. Why would GCC warn here? > > > > > > Sometimes it complains if it can't prove the size of the string to fit the > > > destination. You said that there is no size for boot_command_line, I'm not > > > sure I understand how GCC proves that the above snprintf() won't ever truncate > > > the input. > > > > The compiler doesn't prove that this cannot truncate. It only knows the > > buffer sizes, but not the runtime string lengths. > > > > snprintf() can truncate, and its return value could be used to detect > > that. However, the previous version also ignored possible truncation by > > strlcat(), so I didn't add new truncation handling. > > I understand that, but AFAIK strlcat() doesn't induce a warning in such a case, > while GCC does (or at least should). > gcc only complains about snprintf() when it knows the the sizes (including taking strings from arrays). So I suspect the warnings are mostly false-positives. But I'm not really sure using snprintf() to avoid strlcat() is a gain. This could be: len = strnlen(builtin_cmdline, COMMAND_LINE_SIZE); if (strscpy(builtin_cmdline + len + 1, boot_command_line, COMMAND_LINE_SIZE - len - 1) >= 0) builtin_cmdline[len] = ' '; but I suspect that doesn't return a useful string on overflow. I've been trying to remove strcpy(), a lot of code has already done strlen() for a bound check - so memcpy() can be used instead. -- David ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-06-06 10:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-06-04 13:17 [PATCH] x86/setup: replace strlcat() with snprintf() in setup_arch() Thorsten Blum 2026-06-05 4:41 ` Andy Shevchenko 2026-06-05 15:42 ` Thorsten Blum 2026-06-05 15:55 ` Andy Shevchenko 2026-06-05 18:05 ` Thorsten Blum 2026-06-05 18:28 ` Andy Shevchenko 2026-06-06 10:12 ` David Laight
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.