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 both hi.data.modename and hi.data.drivername to be
NUL-terminated based on its usage with sprintf:
| sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s",
| bc->cfg.intclk ? "int" : "ext",
| bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps,
| bc->cfg.loopback ? ",loopback" : "");
Note that this data is copied out to userspace with:
| if (copy_to_user(data, &hi, sizeof(hi)))
... however, the data was also copied FROM the user here:
| if (copy_from_user(&hi, data, sizeof(hi)))
Considering the above, a suitable replacement is strscpy_pad() as it
guarantees NUL-termination on the destination buffer while also
NUL-padding (which is good+wanted behavior when copying data to
userspace).
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Changes in v2:
- use strscpy_pad (thanks Kees)
- bytes arrays are now considered "buffets" :) (thanks Kees)
- Link to v1: https://lore.kernel.org/r/20231012-strncpy-drivers-net-hamradio-baycom_epp-c-v1-1-8f4097538ee4@google.com
---
Note: build-tested only.
Also, there are 33 instances of trailing whitespace in this file alone.
I've opted to not remove them in this patch.
---
drivers/net/hamradio/baycom_epp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hamradio/baycom_epp.c b/drivers/net/hamradio/baycom_epp.c
index 83ff882f5d97..ccfc83857c26 100644
--- a/drivers/net/hamradio/baycom_epp.c
+++ b/drivers/net/hamradio/baycom_epp.c
@@ -1074,7 +1074,7 @@ static int baycom_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
return 0;
case HDLCDRVCTL_DRIVERNAME:
- strncpy(hi.data.drivername, "baycom_epp", sizeof(hi.data.drivername));
+ strscpy_pad(hi.data.drivername, "baycom_epp", sizeof(hi.data.drivername));
break;
case HDLCDRVCTL_GETMODE:
@@ -1091,7 +1091,7 @@ static int baycom_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
return baycom_setmode(bc, hi.data.modename);
case HDLCDRVCTL_MODELIST:
- strncpy(hi.data.modename, "intclk,extclk,intmodem,extmodem,divider=x",
+ strscpy_pad(hi.data.modename, "intclk,extclk,intmodem,extmodem,divider=x",
sizeof(hi.data.modename));
break;
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231012-strncpy-drivers-net-hamradio-baycom_epp-c-6e11c9483b9f
Best regards,
--
Justin Stitt <[email protected]>
On Mon, Oct 16, 2023 at 06:42:42PM +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 both hi.data.modename and hi.data.drivername to be
> NUL-terminated based on its usage with sprintf:
> | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s",
> | bc->cfg.intclk ? "int" : "ext",
> | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps,
> | bc->cfg.loopback ? ",loopback" : "");
>
> Note that this data is copied out to userspace with:
> | if (copy_to_user(data, &hi, sizeof(hi)))
> ... however, the data was also copied FROM the user here:
> | if (copy_from_user(&hi, data, sizeof(hi)))
>
> Considering the above, a suitable replacement is strscpy_pad() as it
> guarantees NUL-termination on the destination buffer while also
> NUL-padding (which is good+wanted behavior when copying data to
> userspace).
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
Thanks!
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Mon, Oct 16, 2023 at 12:42:22PM -0700, Kees Cook wrote:
> On Mon, Oct 16, 2023 at 06:42:42PM +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 both hi.data.modename and hi.data.drivername to be
> > NUL-terminated based on its usage with sprintf:
> > | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s",
> > | bc->cfg.intclk ? "int" : "ext",
> > | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps,
> > | bc->cfg.loopback ? ",loopback" : "");
> >
> > Note that this data is copied out to userspace with:
> > | if (copy_to_user(data, &hi, sizeof(hi)))
> > ... however, the data was also copied FROM the user here:
> > | if (copy_from_user(&hi, data, sizeof(hi)))
> >
> > Considering the above, a suitable replacement is strscpy_pad() as it
> > guarantees NUL-termination on the destination buffer while also
> > NUL-padding (which is good+wanted behavior when copying data to
> > userspace).
> >
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: [email protected]
> > Signed-off-by: Justin Stitt <[email protected]>
>
> Thanks!
>
> Reviewed-by: Kees Cook <[email protected]>
Likewise, thanks. I prefer this over v1.
Reviewed-by: Simon Horman <[email protected]>
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Mon, 16 Oct 2023 18:42:42 +0000 you 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 both hi.data.modename and hi.data.drivername to be
> NUL-terminated based on its usage with sprintf:
> | sprintf(hi.data.modename, "%sclk,%smodem,fclk=%d,bps=%d%s",
> | bc->cfg.intclk ? "int" : "ext",
> | bc->cfg.extmodem ? "ext" : "int", bc->cfg.fclk, bc->cfg.bps,
> | bc->cfg.loopback ? ",loopback" : "");
>
> [...]
Here is the summary with links:
- [v2] hamradio: replace deprecated strncpy with strscpy_pad
https://git.kernel.org/netdev/net-next/c/d4b14c1da5bf
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html