| Message ID | 20231207-strncpy-drivers-net-mdio-mdio-gpio-c-v2-1-c28d52dd3dfe@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: mdio-gpio: replace deprecated strncpy with strscpy | expand |
On Thu, Dec 07, 2023 at 09:54:31PM +0000, Justin Stitt wrote: > We expect new_bus->id to be NUL-terminated but not NUL-padded based on > its prior assignment through snprintf: > | snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); > > We can also use sizeof() instead of a length macro as this more closely > ties the maximum buffer size to the destination buffer. Honestly, this looks machine generated and unreviewed by the submitter, because... > if (bus_id != -1) > snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); > else > - strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE); > + strscpy(new_bus->id, "gpio", sizeof(new_bus->id)); If there is an argument for not using MII_BUS_ID_SIZE in one place, then the very same argument applies to snprintf(). If one place changes the other also needs to be changed.
On Thu, Dec 7, 2023 at 2:57 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Dec 07, 2023 at 09:54:31PM +0000, Justin Stitt wrote: > > We expect new_bus->id to be NUL-terminated but not NUL-padded based on > > its prior assignment through snprintf: > > | snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); > > > > We can also use sizeof() instead of a length macro as this more closely > > ties the maximum buffer size to the destination buffer. > > Honestly, this looks machine generated and unreviewed by the submitter, > because... > Not machine generated. Was just trying to keep my change as small as possible towards the goal of replacing strncpy. However, you're right. It's literally the line right above it and now it looks inconsistent . > > if (bus_id != -1) > > snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); > > else > > - strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE); > > + strscpy(new_bus->id, "gpio", sizeof(new_bus->id)); > > If there is an argument for not using MII_BUS_ID_SIZE in one place, > then the very same argument applies to snprintf(). If one place > changes the other also needs to be changed. > Gotcha, I've sent a [v3]. > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! [v3]: https://lore.kernel.org/all/20231211-strncpy-drivers-net-mdio-mdio-gpio-c-v3-1-76dea53a1a52@google.com/ Thanks Justin
diff --git a/drivers/net/mdio/mdio-gpio.c b/drivers/net/mdio/mdio-gpio.c index 0fb3c2de0845..a1718d646504 100644 --- a/drivers/net/mdio/mdio-gpio.c +++ b/drivers/net/mdio/mdio-gpio.c @@ -125,7 +125,7 @@ static struct mii_bus *mdio_gpio_bus_init(struct device *dev, if (bus_id != -1) snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); else - strncpy(new_bus->id, "gpio", MII_BUS_ID_SIZE); + strscpy(new_bus->id, "gpio", sizeof(new_bus->id)); if (pdata) { new_bus->phy_mask = pdata->phy_mask;
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 new_bus->id to be NUL-terminated but not NUL-padded based on its prior assignment through snprintf: | snprintf(new_bus->id, MII_BUS_ID_SIZE, "gpio-%x", bus_id); We can also use sizeof() instead of a length macro as this more closely ties the maximum buffer size to the destination buffer. Due to this, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] 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: - change subject line as it was causing problems in patchwork with "superseded" label being improperly applied. - update commit msg with rationale around sizeof() (thanks Kees) - Link to v1 (lore): https://lore.kernel.org/r/20231012-strncpy-drivers-net-mdio-mdio-gpio-c-v1-1-ab9b06cfcdab@google.com - Link to v1 (patchwork): https://patchwork.kernel.org/project/netdevbpf/patch/20231012-strncpy-drivers-net-mdio-mdio-gpio-c-v1-1-ab9b06cfcdab@google.com/ - Link to other patch with same subject message: https://patchwork.kernel.org/project/netdevbpf/patch/20231012-strncpy-drivers-net-phy-mdio_bus-c-v1-1-15242e6f9ec4@google.com/ --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/net/mdio/mdio-gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231012-strncpy-drivers-net-mdio-mdio-gpio-c-bddd9ed0c630 Best regards, -- Justin Stitt <justinstitt@google.com>