| Message ID | 20231016-strncpy-drivers-net-wireless-atmel-atmel-c-v1-1-038d0e73c402@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | atmel: replace deprecated strncpy/strcpy with strscpy | expand |
On Mon, Oct 16, 2023 at 08:22:45PM +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. strcpy() is also deprecated [2] and shouldn't be used. Since these are read-only const C strings being copied into a fixed sized buffer, I would actually prefer to just leave these as-is since they're already getting verified by FORTIFY_SOURCE. > 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> > --- > Note: build-tested only. > > Found with: $ rg "strncpy\(" > --- > drivers/net/wireless/atmel/atmel.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/atmel/atmel.c b/drivers/net/wireless/atmel/atmel.c > index 7c2d1c588156..199b9144ef63 100644 > --- a/drivers/net/wireless/atmel/atmel.c > +++ b/drivers/net/wireless/atmel/atmel.c > @@ -2011,7 +2011,7 @@ static int atmel_get_name(struct net_device *dev, > union iwreq_data *wrqu, > char *extra) > { > - strcpy(wrqu->name, "IEEE 802.11-DS"); > + strscpy(wrqu->name, "IEEE 802.11-DS", sizeof(wrqu->name)); > return 0; > } > > @@ -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)); I wonder why this ever used strncpy -- they're both 32 byte buffer arrays. Regardless, yup, this looks correct to me. Can you respin without the strcpy() changes? -Kees > break; > > case ATMELRD: > @@ -3889,7 +3888,8 @@ static int reset_atmel_card(struct net_device *dev) > printk(KERN_INFO > "%s: if not, use the firmware= module parameter.\n", > dev->name); > - strcpy(priv->firmware_id, "atmel_at76c502.bin"); > + strscpy(priv->firmware_id, "atmel_at76c502.bin", > + sizeof(priv->firmware_id)); > } > err = request_firmware(&fw_entry, priv->firmware_id, priv->sys_dev); > if (err != 0) { > > --- > base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 > change-id: 20231016-strncpy-drivers-net-wireless-atmel-atmel-c-7ca951cf7cfa > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
diff --git a/drivers/net/wireless/atmel/atmel.c b/drivers/net/wireless/atmel/atmel.c index 7c2d1c588156..199b9144ef63 100644 --- a/drivers/net/wireless/atmel/atmel.c +++ b/drivers/net/wireless/atmel/atmel.c @@ -2011,7 +2011,7 @@ static int atmel_get_name(struct net_device *dev, union iwreq_data *wrqu, char *extra) { - strcpy(wrqu->name, "IEEE 802.11-DS"); + strscpy(wrqu->name, "IEEE 802.11-DS", sizeof(wrqu->name)); return 0; } @@ -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: @@ -3889,7 +3888,8 @@ static int reset_atmel_card(struct net_device *dev) printk(KERN_INFO "%s: if not, use the firmware= module parameter.\n", dev->name); - strcpy(priv->firmware_id, "atmel_at76c502.bin"); + strscpy(priv->firmware_id, "atmel_at76c502.bin", + sizeof(priv->firmware_id)); } err = request_firmware(&fw_entry, priv->firmware_id, priv->sys_dev); if (err != 0) {
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. strcpy() is also deprecated [2] and shouldn't be used. 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> --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/net/wireless/atmel/atmel.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231016-strncpy-drivers-net-wireless-atmel-atmel-c-7ca951cf7cfa Best regards, -- Justin Stitt <justinstitt@google.com>