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 mdiodev->modalias to be NUL-terminated based on its usage with
strcmp():
| return strcmp(mdiodev->modalias, drv->name) == 0;
Moreover, mdiodev->modalias is already zero-allocated:
| mdiodev = kzalloc(sizeof(*mdiodev), GFP_KERNEL);
... which means the NUL-padding strncpy provides is not necessary.
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.
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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/net/phy/mdio_bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 25dcaa49ab8b..6cf73c15635b 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -506,7 +506,7 @@ static int mdiobus_create_device(struct mii_bus *bus,
if (IS_ERR(mdiodev))
return -ENODEV;
- strncpy(mdiodev->modalias, bi->modalias,
+ strscpy(mdiodev->modalias, bi->modalias,
sizeof(mdiodev->modalias));
mdiodev->bus_match = mdio_device_bus_match;
mdiodev->dev.platform_data = (void *)bi->platform_data;
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231012-strncpy-drivers-net-phy-mdio_bus-c-0a0d5e875712
Best regards,
--
Justin Stitt <[email protected]>
On Thu, Oct 12, 2023 at 09:53:03PM +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.
Hi Justin
You just sent two patches with the same Subject. That got me confused
for a while, is it a resend? A new version?
> 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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Thu, Oct 12, 2023 at 2:59 PM Andrew Lunn <[email protected]> wrote:
>
> On Thu, Oct 12, 2023 at 09:53:03PM +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.
>
> Hi Justin
>
> You just sent two patches with the same Subject. That got me confused
> for a while, is it a resend? A new version?
Yep, just saw this.
I'm working (top to bottom) on a list of strncpy hits. I have an automated tool
fetch the prefix and update the subject line accordingly. They are two separate
patches but ended up with the same exact subject line due to oversight and
over-automation.
Looking for guidance:
Should I combine them into one patch?
>
> > 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: [email protected]
> > Signed-off-by: Justin Stitt <[email protected]>
>
> Reviewed-by: Andrew Lunn <[email protected]>
>
> Andrew
>
Thanks
Justin
On Thu, Oct 12, 2023 at 03:01:06PM -0700, Justin Stitt wrote:
> On Thu, Oct 12, 2023 at 2:59 PM Andrew Lunn <[email protected]> wrote:
> >
> > On Thu, Oct 12, 2023 at 09:53:03PM +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.
> >
> > Hi Justin
> >
> > You just sent two patches with the same Subject. That got me confused
> > for a while, is it a resend? A new version?
>
> Yep, just saw this.
>
> I'm working (top to bottom) on a list of strncpy hits. I have an automated tool
> fetch the prefix and update the subject line accordingly. They are two separate
> patches but ended up with the same exact subject line due to oversight and
> over-automation.
>
> Looking for guidance:
> Should I combine them into one patch?
No, it is fine. Just try to avoid it in the future.
Andrew
On Thu, Oct 12, 2023 at 09:53:03PM +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 mdiodev->modalias to be NUL-terminated based on its usage with
> strcmp():
> | return strcmp(mdiodev->modalias, drv->name) == 0;
>
> Moreover, mdiodev->modalias is already zero-allocated:
> | mdiodev = kzalloc(sizeof(*mdiodev), GFP_KERNEL);
> ... which means the NUL-padding strncpy provides is not necessary.
>
> 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.
>
> 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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
Looks good!
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook