net: mdio: replace deprecated strncpy with strscpy

Commit Message

Justin Stitt Oct. 12, 2023, 9:53 p.m. UTC

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: [e-mail]
Signed-off-by: Justin Stitt <[e-mail]>
---
Note: build-tested only.

Found with: $ rg "strncpy\("
---
 drivers/net/phy/mdio_bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231012-strncpy-drivers-net-phy-mdio_bus-c-0a0d5e875712

Best regards,
--
Justin Stitt <[e-mail]>

Comments

Andrew Lunn Oct. 12, 2023, 9:59 p.m. UTC | #1

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: [e-mail]
> Signed-off-by: Justin Stitt <[e-mail]>

Reviewed-by: Andrew Lunn <[e-mail]>

    Andrew

Justin Stitt Oct. 12, 2023, 10:01 p.m. UTC | #2

On Thu, Oct 12, 2023 at 2:59 PM Andrew Lunn <[e-mail]> 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: [e-mail]
> > Signed-off-by: Justin Stitt <[e-mail]>
>
> Reviewed-by: Andrew Lunn <[e-mail]>
>
>     Andrew
>
Thanks
Justin

Andrew Lunn Oct. 13, 2023, 12:05 p.m. UTC | #3

On Thu, Oct 12, 2023 at 03:01:06PM -0700, Justin Stitt wrote:
> On Thu, Oct 12, 2023 at 2:59 PM Andrew Lunn <[e-mail]> 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

Kees Cook Oct. 18, 2023, 11:23 p.m. UTC | #4

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: [e-mail]
> Signed-off-by: Justin Stitt <[e-mail]>

Looks good!

Reviewed-by: Kees Cook <[e-mail]>

Patch

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;