| Message ID | 20230530163041.985456-1-azeemshaikh38@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh: Replace all non-returning strlcpy with strscpy | expand |
On Tue, May 30, 2023 at 04:30:41PM +0000, Azeem Shaikh wrote: > 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 <azeemshaikh38@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org>
On Tue, 30 May 2023 16:30:41 +0000, Azeem Shaikh wrote: > 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
Hi Kees! On Wed, 2023-06-14 at 11:44 -0700, Kees Cook wrote: > On Tue, 30 May 2023 16:30:41 +0000, Azeem Shaikh wrote: > > 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 > 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
On Wed, Jun 14, 2023 at 08:49:13PM +0200, John Paul Adrian Glaubitz wrote: > Hi Kees! > > On Wed, 2023-06-14 at 11:44 -0700, Kees Cook wrote: > > On Tue, 30 May 2023 16:30:41 +0000, Azeem Shaikh wrote: > > > 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 > > > > 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
On Tue, 2023-05-30 at 16:30 +0000, Azeem Shaikh wrote: > 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 <azeemshaikh38@gmail.com> > --- > arch/sh/drivers/dma/dma-api.c | 2 +- > arch/sh/kernel/setup.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/sh/drivers/dma/dma-api.c b/arch/sh/drivers/dma/dma-api.c > index ab9170494dcc..89cd4a3b4cca 100644 > --- a/arch/sh/drivers/dma/dma-api.c > +++ b/arch/sh/drivers/dma/dma-api.c > @@ -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); > diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c > index af977ec4ca5e..e4f0f9a1d355 100644 > --- a/arch/sh/kernel/setup.c > +++ b/arch/sh/kernel/setup.c > @@ -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)); Acked-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
On Tue, 2023-05-30 at 16:30 +0000, Azeem Shaikh wrote: > 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 <azeemshaikh38@gmail.com> > --- > arch/sh/drivers/dma/dma-api.c | 2 +- > arch/sh/kernel/setup.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/sh/drivers/dma/dma-api.c b/arch/sh/drivers/dma/dma-api.c > index ab9170494dcc..89cd4a3b4cca 100644 > --- a/arch/sh/drivers/dma/dma-api.c > +++ b/arch/sh/drivers/dma/dma-api.c > @@ -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); > diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c > index af977ec4ca5e..e4f0f9a1d355 100644 > --- a/arch/sh/kernel/setup.c > +++ b/arch/sh/kernel/setup.c > @@ -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)); Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
On Wed, 2023-06-14 at 12:03 -0700, Kees Cook wrote: > On Wed, Jun 14, 2023 at 08:49:13PM +0200, John Paul Adrian Glaubitz wrote: > > Hi Kees! > > > > On Wed, 2023-06-14 at 11:44 -0700, Kees Cook wrote: > > > On Tue, 30 May 2023 16:30:41 +0000, Azeem Shaikh wrote: > > > > 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 > > > > > > > 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
On Wed, Jun 14, 2023 at 09:23:49PM +0200, John Paul Adrian Glaubitz wrote: > On Wed, 2023-06-14 at 12:03 -0700, Kees Cook wrote: > > On Wed, Jun 14, 2023 at 08:49:13PM +0200, John Paul Adrian Glaubitz wrote: > > > Hi Kees! > > > > > > On Wed, 2023-06-14 at 11:44 -0700, Kees Cook wrote: > > > > On Tue, 30 May 2023 16:30:41 +0000, Azeem Shaikh wrote: > > > > > 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 > > > > > > > > > > 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
diff --git a/arch/sh/drivers/dma/dma-api.c b/arch/sh/drivers/dma/dma-api.c index ab9170494dcc..89cd4a3b4cca 100644 --- a/arch/sh/drivers/dma/dma-api.c +++ b/arch/sh/drivers/dma/dma-api.c @@ -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); diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c index af977ec4ca5e..e4f0f9a1d355 100644 --- a/arch/sh/kernel/setup.c +++ b/arch/sh/kernel/setup.c @@ -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));
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 <azeemshaikh38@gmail.com> --- arch/sh/drivers/dma/dma-api.c | 2 +- arch/sh/kernel/setup.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)