| Message ID | 20231019-strncpy-drivers-net-wireless-atmel-atmel-c-v2-1-52e46fbea083@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] atmel: replace deprecated strncpy with strscpy | expand |
On Thu, Oct 19, 2023 at 05:29:01PM +0000, Justin Stitt wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect priv->firmware_id to be NUL-terminated based on its usage > with seq_printf() and strlen() in atmel.c: > 1420 | seq_printf(m, "%s loaded by host\n", priv->firmware_id); > ... > 3884 | if (strlen(priv->firmware_id) == 0) { > > NUL-padding is not required, which is evident by the usage of a plain > strcpy(): > 3891 | strcpy(priv->firmware_id, "atmel_at76c502.bin"); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Let's also replace hard-coded lengths to be `sizeof(...)` for buffers > that the compiler can detect the size for as this is less error prone. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [2] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org>
Justin Stitt <justinstitt@google.com> wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect priv->firmware_id to be NUL-terminated based on its usage > with seq_printf() and strlen() in atmel.c: > 1420 | seq_printf(m, "%s loaded by host\n", priv->firmware_id); > ... > 3884 | if (strlen(priv->firmware_id) == 0) { > > NUL-padding is not required, which is evident by the usage of a plain > strcpy(): > 3891 | strcpy(priv->firmware_id, "atmel_at76c502.bin"); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Let's also replace hard-coded lengths to be `sizeof(...)` for buffers > that the compiler can detect the size for as this is less error prone. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [2] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > Reviewed-by: Kees Cook <keescook@chromium.org> Failed to apply: Recorded preimage for 'drivers/net/wireless/atmel/atmel.c' error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Applying: wifi: atmel: replace deprecated strncpy with strscpy Using index info to reconstruct a base tree... M drivers/net/wireless/atmel/atmel.c Falling back to patching base and 3-way merge... Auto-merging drivers/net/wireless/atmel/atmel.c CONFLICT (content): Merge conflict in drivers/net/wireless/atmel/atmel.c Patch failed at 0001 wifi: atmel: replace deprecated strncpy with strscpy Patch set to Changes Requested.
diff --git a/drivers/net/wireless/atmel/atmel.c b/drivers/net/wireless/atmel/atmel.c index 7c2d1c588156..99920430e8b4 100644 --- a/drivers/net/wireless/atmel/atmel.c +++ b/drivers/net/wireless/atmel/atmel.c @@ -2651,8 +2651,7 @@ static int atmel_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) priv->firmware = new_firmware; priv->firmware_length = com.len; - strncpy(priv->firmware_id, com.id, 31); - priv->firmware_id[31] = '\0'; + strscpy(priv->firmware_id, com.id, sizeof(priv->firmware_id)); break; case ATMELRD:
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect priv->firmware_id to be NUL-terminated based on its usage with seq_printf() and strlen() in atmel.c: 1420 | seq_printf(m, "%s loaded by host\n", priv->firmware_id); ... 3884 | if (strlen(priv->firmware_id) == 0) { NUL-padding is not required, which is evident by the usage of a plain strcpy(): 3891 | strcpy(priv->firmware_id, "atmel_at76c502.bin"); Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Let's also replace hard-coded lengths to be `sizeof(...)` for buffers that the compiler can detect the size for as this is less error prone. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [2] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Changes in v2: - leave strcpy() usage as is (thanks Kees) - Link to v1: https://lore.kernel.org/r/20231016-strncpy-drivers-net-wireless-atmel-atmel-c-v1-1-038d0e73c402@google.com --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/net/wireless/atmel/atmel.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231016-strncpy-drivers-net-wireless-atmel-atmel-c-7ca951cf7cfa Best regards, -- Justin Stitt <justinstitt@google.com>