| Message ID | 20231017-strncpy-drivers-net-wireless-intel-ipw2x00-ipw2200-c-v2-1-465e10dc817c@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8890b9bca38f16274bd3f612986f78f81a886c8b |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] wifi: ipw2x00: replace deprecated strncpy with strscpy_pad | expand |
On Tue, Oct 17, 2023 at 09:48:15PM +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. > > `extra` is intended to be NUL-terminated which is evident by the manual > assignment of a NUL-byte as well as its immediate usage with strlen(). > > Moreover, many of these getters and setters are NUL-padding buffers with > memset(): > 2439 | memset(&tx_power, 0, sizeof(tx_power)); > 9998 | memset(sys_config, 0, sizeof(struct ipw_sys_config)); > 10084 | memset(tfd, 0, sizeof(*tfd)); > 10261 | memset(&dummystats, 0, sizeof(dummystats)); > ... let's maintain this behavior and NUL-pad our destination buffer. > > Considering the above, a suitable replacement is `strscpy_pad` due to > the fact that it guarantees both NUL-termination and NUL-padding on the > destination buffer. > > To be clear, there is no bug in the current implementation as > MAX_WX_STRING is much larger than the size of the string literals being > copied from. Also, strncpy() does NUL-pad the destination buffer and > using strscpy_pad() simply matches that behavior. All in all, there > should be no functional change but we are one step closer to eliminating > usage of strncpy(). > > Do note that we cannot use the more idiomatic strscpy invocation of > (dest, src, sizeof(dest)) as the destination buffer cannot have its size > determined at compile time. So, let's stick with (dest, src, LEN). Yeah, these interfaces have external buffer size declarations. In this case, MAX_WX_STRING. This is probably one of the most difficult set of callbacks to track down. sysfs might be worse... But, ultimately, this is a private ioctl handler, and it is all boiled down to calling ioctl_private_iw_point() (via ioctl_private_call()), which does the allocation of "extra". The size, which is not passed to the handler (*sob*), is determined by: get_priv_descr_and_size(), which is looking at drivers/net/wireless/intel/ipw2x00/ipw2200.c's ipw_priv_args: { .cmd = IPW_PRIV_GET_MODE, .get_args = IW_PRIV_TYPE_CHAR | IW_PRIV_SIZE_FIXED | MAX_WX_STRING, .name = "get_mode"}, So it looks good to me. Reviewed-by: Kees Cook <keescook@chromium.org>
Justin Stitt <justinstitt@google.com> 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. > > `extra` is intended to be NUL-terminated which is evident by the manual > assignment of a NUL-byte as well as its immediate usage with strlen(). > > Moreover, many of these getters and setters are NUL-padding buffers with > memset(): > 2439 | memset(&tx_power, 0, sizeof(tx_power)); > 9998 | memset(sys_config, 0, sizeof(struct ipw_sys_config)); > 10084 | memset(tfd, 0, sizeof(*tfd)); > 10261 | memset(&dummystats, 0, sizeof(dummystats)); > ... let's maintain this behavior and NUL-pad our destination buffer. > > Considering the above, a suitable replacement is `strscpy_pad` due to > the fact that it guarantees both NUL-termination and NUL-padding on the > destination buffer. > > To be clear, there is no bug in the current implementation as > MAX_WX_STRING is much larger than the size of the string literals being > copied from. Also, strncpy() does NUL-pad the destination buffer and > using strscpy_pad() simply matches that behavior. All in all, there > should be no functional change but we are one step closer to eliminating > usage of strncpy(). > > Do note that we cannot use the more idiomatic strscpy invocation of > (dest, src, sizeof(dest)) as the destination buffer cannot have its size > determined at compile time. So, let's stick with (dest, src, LEN). > > 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: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > Reviewed-by: Kees Cook <keescook@chromium.org> Patch applied to wireless-next.git, thanks. 8890b9bca38f wifi: ipw2x00: replace deprecated strncpy with strscpy_pad
diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2200.c b/drivers/net/wireless/intel/ipw2x00/ipw2200.c index 820100cac491..44f2d91ad30f 100644 --- a/drivers/net/wireless/intel/ipw2x00/ipw2200.c +++ b/drivers/net/wireless/intel/ipw2x00/ipw2200.c @@ -9656,31 +9656,30 @@ static int ipw_wx_get_wireless_mode(struct net_device *dev, mutex_lock(&priv->mutex); switch (priv->ieee->mode) { case IEEE_A: - strncpy(extra, "802.11a (1)", MAX_WX_STRING); + strscpy_pad(extra, "802.11a (1)", MAX_WX_STRING); break; case IEEE_B: - strncpy(extra, "802.11b (2)", MAX_WX_STRING); + strscpy_pad(extra, "802.11b (2)", MAX_WX_STRING); break; case IEEE_A | IEEE_B: - strncpy(extra, "802.11ab (3)", MAX_WX_STRING); + strscpy_pad(extra, "802.11ab (3)", MAX_WX_STRING); break; case IEEE_G: - strncpy(extra, "802.11g (4)", MAX_WX_STRING); + strscpy_pad(extra, "802.11g (4)", MAX_WX_STRING); break; case IEEE_A | IEEE_G: - strncpy(extra, "802.11ag (5)", MAX_WX_STRING); + strscpy_pad(extra, "802.11ag (5)", MAX_WX_STRING); break; case IEEE_B | IEEE_G: - strncpy(extra, "802.11bg (6)", MAX_WX_STRING); + strscpy_pad(extra, "802.11bg (6)", MAX_WX_STRING); break; case IEEE_A | IEEE_B | IEEE_G: - strncpy(extra, "802.11abg (7)", MAX_WX_STRING); + strscpy_pad(extra, "802.11abg (7)", MAX_WX_STRING); break; default: - strncpy(extra, "unknown", MAX_WX_STRING); + strscpy_pad(extra, "unknown", MAX_WX_STRING); break; } - extra[MAX_WX_STRING - 1] = '\0'; IPW_DEBUG_WX("PRIV GET MODE: %s\n", extra);
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. `extra` is intended to be NUL-terminated which is evident by the manual assignment of a NUL-byte as well as its immediate usage with strlen(). Moreover, many of these getters and setters are NUL-padding buffers with memset(): 2439 | memset(&tx_power, 0, sizeof(tx_power)); 9998 | memset(sys_config, 0, sizeof(struct ipw_sys_config)); 10084 | memset(tfd, 0, sizeof(*tfd)); 10261 | memset(&dummystats, 0, sizeof(dummystats)); ... let's maintain this behavior and NUL-pad our destination buffer. Considering the above, a suitable replacement is `strscpy_pad` due to the fact that it guarantees both NUL-termination and NUL-padding on the destination buffer. To be clear, there is no bug in the current implementation as MAX_WX_STRING is much larger than the size of the string literals being copied from. Also, strncpy() does NUL-pad the destination buffer and using strscpy_pad() simply matches that behavior. All in all, there should be no functional change but we are one step closer to eliminating usage of strncpy(). Do note that we cannot use the more idiomatic strscpy invocation of (dest, src, sizeof(dest)) as the destination buffer cannot have its size determined at compile time. So, let's stick with (dest, src, LEN). 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: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Changes in v2: - add wifi: to subject - Link to v1: https://lore.kernel.org/r/20231017-strncpy-drivers-net-wireless-intel-ipw2x00-ipw2200-c-v1-1-ee7d3e258d78@google.com --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/net/wireless/intel/ipw2x00/ipw2200.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) --- base-commit: 58720809f52779dc0f08e53e54b014209d13eebb change-id: 20231017-strncpy-drivers-net-wireless-intel-ipw2x00-ipw2200-c-6f8880232e06 Best regards, -- Justin Stitt <justinstitt@google.com>