| Message ID | 20230807-arch-um-drivers-v1-1-10d602c5577a@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | um: vector: refactor deprecated strncpy | expand |
On 07/08/2023 19:22, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on its destination buffer argument which is > _not_ the case for `strncpy`! > > In this case, we are able to drop the now superfluous `... - 1` > instances because `strscpy` will automatically truncate the last byte by > setting it to a NUL byte if the source size exceeds the destination size > or if the source string is not NUL-terminated. > > I've also opted to remove the seemingly useless char* casts. I'm not > sure why they're present at all since (after expanding the `ifr_name` > macro) `ifr.ifr_ifrn.ifrn_name` is a char* already. > > All in all, `strscpy` is a more robust and less ambiguous interface > while also letting us remove some `... -1`'s which cleans things up a > bit. > > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [2]: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > arch/um/drivers/vector_user.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c > index c650e428432b..c719e1ec4645 100644 > --- a/arch/um/drivers/vector_user.c > +++ b/arch/um/drivers/vector_user.c > @@ -141,7 +141,7 @@ static int create_tap_fd(char *iface) > } > memset(&ifr, 0, sizeof(ifr)); > ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR; > - strncpy((char *)&ifr.ifr_name, iface, sizeof(ifr.ifr_name) - 1); > + strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name)); > > err = ioctl(fd, TUNSETIFF, (void *) &ifr); > if (err != 0) { > @@ -171,7 +171,7 @@ static int create_raw_fd(char *iface, int flags, int proto) > goto raw_fd_cleanup; > } > memset(&ifr, 0, sizeof(ifr)); > - strncpy((char *)&ifr.ifr_name, iface, sizeof(ifr.ifr_name) - 1); > + strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name)); > if (ioctl(fd, SIOCGIFINDEX, (void *) &ifr) < 0) { > err = -errno; > goto raw_fd_cleanup; > > --- > base-commit: c1a515d3c0270628df8ae5f5118ba859b85464a2 > change-id: 20230807-arch-um-drivers-ad44050885d0 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um > Acked-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c index c650e428432b..c719e1ec4645 100644 --- a/arch/um/drivers/vector_user.c +++ b/arch/um/drivers/vector_user.c @@ -141,7 +141,7 @@ static int create_tap_fd(char *iface) } memset(&ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR; - strncpy((char *)&ifr.ifr_name, iface, sizeof(ifr.ifr_name) - 1); + strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name)); err = ioctl(fd, TUNSETIFF, (void *) &ifr); if (err != 0) { @@ -171,7 +171,7 @@ static int create_raw_fd(char *iface, int flags, int proto) goto raw_fd_cleanup; } memset(&ifr, 0, sizeof(ifr)); - strncpy((char *)&ifr.ifr_name, iface, sizeof(ifr.ifr_name) - 1); + strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name)); if (ioctl(fd, SIOCGIFINDEX, (void *) &ifr) < 0) { err = -errno; goto raw_fd_cleanup;
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ the case for `strncpy`! In this case, we are able to drop the now superfluous `... - 1` instances because `strscpy` will automatically truncate the last byte by setting it to a NUL byte if the source size exceeds the destination size or if the source string is not NUL-terminated. I've also opted to remove the seemingly useless char* casts. I'm not sure why they're present at all since (after expanding the `ifr_name` macro) `ifr.ifr_ifrn.ifrn_name` is a char* already. All in all, `strscpy` is a more robust and less ambiguous interface while also letting us remove some `... -1`'s which cleans things up a bit. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- arch/um/drivers/vector_user.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: c1a515d3c0270628df8ae5f5118ba859b85464a2 change-id: 20230807-arch-um-drivers-ad44050885d0 Best regards, -- Justin Stitt <justinstitt@google.com>