sh: Replace all non-returning strlcpy with strscpy

PATCH Applied Azeem Shaikh 3 participants 9 messages May 30, 2023 – Jun 14, 2023 SuperH

Share & tools

Azeem Shaikh

author

2023-05-30 16:31 UTC #0 lore raw

PATCH 1/1 sh: Replace all non-returning strlcpy with strscpy

strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by Azeem Shaikh <[email protected]>

---
 arch/sh/drivers/dma/dma-api.c |    2 +-
 arch/sh/kernel/setup.c        |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

arch/sh/drivers/dma/dma-api.c +1 -1

index ab9170494dcc..89cd4a3b4cca 100644
@@ -198,7 +198,7 @@ int request_dma(unsigned int chan, const char *dev_id)
 	if (atomic_xchg(&channel->busy, 1))
 		return -EBUSY;
 
-	strlcpy(channel->dev_id, dev_id, sizeof(channel->dev_id));
+	strscpy(channel->dev_id, dev_id, sizeof(channel->dev_id));
 
 	if (info->ops->request) {
 		result = info->ops->request(channel);

arch/sh/kernel/setup.c +2 -2

index af977ec4ca5e..e4f0f9a1d355 100644
@@ -304,9 +304,9 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.end = virt_to_phys(__bss_stop)-1;
 
 #ifdef CONFIG_CMDLINE_OVERWRITE
-	strlcpy(command_line, CONFIG_CMDLINE, sizeof(command_line));
+	strscpy(command_line, CONFIG_CMDLINE, sizeof(command_line));
 #else
-	strlcpy(command_line, COMMAND_LINE, sizeof(command_line));
+	strscpy(command_line, COMMAND_LINE, sizeof(command_line));
 #ifdef CONFIG_CMDLINE_EXTEND
 	strlcat(command_line, " ", sizeof(command_line));
 	strlcat(command_line, CONFIG_CMDLINE, sizeof(command_line));

Kees Cook

Reviewed-by on v1 1/1

2023-05-30 23:21 UTC #1 lore raw

Quoted 8 lines · Azeem Shaikh

strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.

Quoted 4 lines

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89
Signed-off-by Azeem Shaikh <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

Kees Cook

Applied

2023-06-14 18:45 UTC #2 lore raw

In reply to Azeem Shaikh

Quoted 9 lines · Azeem Shaikh

strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[...]

Build tested with sh4 GCC 13.1 from:
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/13.1.0/

with defconfig and:

	CONFIG_CPU_SUBTYPE_SH7343=y
	CONFIG_SH_DMA=y
	CONFIG_SH_DMA_API=y

Applied to for-next/hardening, thanks!

[1/1] sh: Replace all non-returning strlcpy with strscpy

      https://git.kernel.org/kees/c/ca64da3052be

Signature · 1 line

Kees Cook

John Paul Adrian Glaubitz

maintainer

2023-06-14 18:49 UTC #3 lore raw

Hi Kees!

Quoted 15 lines · Kees Cook

Quoted 9 lines · Azeem Shaikh
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[...]
Build tested with sh4 GCC 13.1 from:
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/13.1.0/
with defconfig and:

Quoted 9 lines

	CONFIG_CPU_SUBTYPE_SH7343=y
	CONFIG_SH_DMA=y
	CONFIG_SH_DMA_API=y
Applied to for-next/hardening, thanks!
[1/1] sh: Replace all non-returning strlcpy with strscpy
      https://git.kernel.org/kees/c/ca64da3052be

Apologies, this fell off my table. I should have acked and tested this being the SuperH maintainer. If you can still update the patch in your tree, I can both test and ack this patch.

Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Kees Cook

2023-06-14 19:03 UTC #4 lore raw

Quoted 24 lines · John Paul Adrian Glaubitz

Hi Kees!
Quoted 21 lines · Kees Cook
Quoted 9 lines · Azeem Shaikh
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[...]
Build tested with sh4 GCC 13.1 from:
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/13.1.0/
with defconfig and:
	CONFIG_CPU_SUBTYPE_SH7343=y
	CONFIG_SH_DMA=y
	CONFIG_SH_DMA_API=y
Applied to for-next/hardening, thanks!

Quoted 7 lines

Quoted 3 lines
[1/1] sh: Replace all non-returning strlcpy with strscpy
      https://git.kernel.org/kees/c/ca64da3052be
Apologies, this fell off my table. I should have acked and tested this being the SuperH maintainer. If you can still update the patch in your tree, I can both test and ack this patch.

Absolutely! Thanks for double-checking. :)

-Kees

Signature · 1 line

Kees Cook

John Paul Adrian Glaubitz

maintainer

2023-06-14 19:24 UTC #5 lore raw

Quoted 29 lines · Kees Cook

Quoted 28 lines · John Paul Adrian Glaubitz
Hi Kees!
Quoted 24 lines · Kees Cook
Quoted 9 lines · Azeem Shaikh
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[...]
Build tested with sh4 GCC 13.1 from:
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/13.1.0/
with defconfig and:
	CONFIG_CPU_SUBTYPE_SH7343=y
	CONFIG_SH_DMA=y
	CONFIG_SH_DMA_API=y
Applied to for-next/hardening, thanks!
[1/1] sh: Replace all non-returning strlcpy with strscpy
      https://git.kernel.org/kees/c/ca64da3052be

Quoted 5 lines

Quoted 3 lines
Apologies, this fell off my table. I should have acked and tested this being the SuperH maintainer. If you can still update the patch in your tree, I can both test and ack this patch.
Absolutely! Thanks for double-checking. :)

I have tested the patch on my SH-7785LCR board on top of Linus' tree and also acked it.

Thanks,
Adrian

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Kees Cook

2023-06-14 19:28 UTC #6 lore raw

Quoted 30 lines · John Paul Adrian Glaubitz

Quoted 29 lines · Kees Cook
Quoted 28 lines · John Paul Adrian Glaubitz
Hi Kees!
Quoted 24 lines · Kees Cook
Quoted 9 lines · Azeem Shaikh
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[...]
Build tested with sh4 GCC 13.1 from:
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/13.1.0/
with defconfig and:
	CONFIG_CPU_SUBTYPE_SH7343=y
	CONFIG_SH_DMA=y
	CONFIG_SH_DMA_API=y
Applied to for-next/hardening, thanks!
[1/1] sh: Replace all non-returning strlcpy with strscpy
      https://git.kernel.org/kees/c/ca64da3052be

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Apologies, this fell off my table. I should have acked and tested this being the SuperH maintainer. If you can still update the patch in your tree, I can both test and ack this patch.
Absolutely! Thanks for double-checking. :)
I have tested the patch on my SH-7785LCR board on top of Linus' tree and also acked it.

Awesome. :) I have updated the tags and will push out my tree shortly.

Thanks!

-Kees

Signature · 1 line

Kees Cook

John Paul Adrian Glaubitz

maintainer Acked-by on v1 1/1

2023-06-14 19:22 UTC #7 lore raw

In reply to Azeem Shaikh

Quoted 36 lines · Azeem Shaikh

strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89
Signed-off-by Azeem Shaikh <[email protected]>
---
 arch/sh/drivers/dma/dma-api.c |    2 +-
 arch/sh/kernel/setup.c        |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
arch/sh/drivers/dma/dma-api.c +1 -1
index ab9170494dcc..89cd4a3b4cca 100644
@@ -198,7 +198,7 @@ int request_dma(unsigned int chan, const char *dev_id)
 	if (atomic_xchg(&channel->busy, 1))
 		return -EBUSY;
 
-	strlcpy(channel->dev_id, dev_id, sizeof(channel->dev_id));
+	strscpy(channel->dev_id, dev_id, sizeof(channel->dev_id));
 
 	if (info->ops->request) {
 		result = info->ops->request(channel);
arch/sh/kernel/setup.c +0 -0
index af977ec4ca5e..e4f0f9a1d355 100644
@@ -304,9 +304,9 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.end = virt_to_phys(__bss_stop)-1;

arch/sh/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 312

Quoted 10 lines

arch/sh/kernel/setup.c +2 -2

 
 #ifdef CONFIG_CMDLINE_OVERWRITE
-	strlcpy(command_line, CONFIG_CMDLINE, sizeof(command_line));
+	strscpy(command_line, CONFIG_CMDLINE, sizeof(command_line));
 #else
-	strlcpy(command_line, COMMAND_LINE, sizeof(command_line));
+	strscpy(command_line, COMMAND_LINE, sizeof(command_line));
 #ifdef CONFIG_CMDLINE_EXTEND
 	strlcat(command_line, " ", sizeof(command_line));
 	strlcat(command_line, CONFIG_CMDLINE, sizeof(command_line));

Acked-by John Paul Adrian Glaubitz <[email protected]>

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

John Paul Adrian Glaubitz

maintainer Tested-by on v1 1/1

2023-06-14 19:23 UTC #8 lore raw

In reply to Azeem Shaikh

Quoted 36 lines · Azeem Shaikh

strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89
Signed-off-by Azeem Shaikh <[email protected]>
---
 arch/sh/drivers/dma/dma-api.c |    2 +-
 arch/sh/kernel/setup.c        |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
arch/sh/drivers/dma/dma-api.c +1 -1
index ab9170494dcc..89cd4a3b4cca 100644
@@ -198,7 +198,7 @@ int request_dma(unsigned int chan, const char *dev_id)
 	if (atomic_xchg(&channel->busy, 1))
 		return -EBUSY;
 
-	strlcpy(channel->dev_id, dev_id, sizeof(channel->dev_id));
+	strscpy(channel->dev_id, dev_id, sizeof(channel->dev_id));
 
 	if (info->ops->request) {
 		result = info->ops->request(channel);
arch/sh/kernel/setup.c +0 -0
index af977ec4ca5e..e4f0f9a1d355 100644
@@ -304,9 +304,9 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.end = virt_to_phys(__bss_stop)-1;

arch/sh/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 312

Quoted 10 lines

arch/sh/kernel/setup.c +2 -2

 
 #ifdef CONFIG_CMDLINE_OVERWRITE
-	strlcpy(command_line, CONFIG_CMDLINE, sizeof(command_line));
+	strscpy(command_line, CONFIG_CMDLINE, sizeof(command_line));
 #else
-	strlcpy(command_line, COMMAND_LINE, sizeof(command_line));
+	strscpy(command_line, COMMAND_LINE, sizeof(command_line));
 #ifdef CONFIG_CMDLINE_EXTEND
 	strlcat(command_line, " ", sizeof(command_line));
 	strlcat(command_line, CONFIG_CMDLINE, sizeof(command_line));

Tested-by John Paul Adrian Glaubitz <[email protected]>

Signature · 4 lines

 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913