diff mbox series

[v2] airo: replace deprecated strncpy with strscpy_pad

Message ID 20231026-strncpy-drivers-net-wireless-cisco-airo-c-v2-1-413427249e47@google.com (mailing list archive)
State Mainlined
Commit 9beac4ee49286101e30a1de2fe58625f998bf77c
Headers show
Series [v2] airo: replace deprecated strncpy with strscpy_pad | expand

Commit Message

Justin Stitt Oct. 26, 2023, 11:19 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.

`extra` is clearly supposed to be NUL-terminated which is evident by the
manual NUL-byte assignment as well as its immediate usage with strlen().

Moreover, let's NUL-pad since there is deliberate effort (48 instances)
made elsewhere to zero-out buffers in these getters and setters:
6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
6130 | memset(local->config.rates, 0, 8);
6139 | memset(local->config.rates, 0, 8);
6414 | memset(key.key, 0, MAX_KEY_SIZE);
6497 | memset(extra, 0, 16);
(to be clear, strncpy also NUL-padded -- we are matching that behavior)

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.

We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
because this function is a wext handler.

In wext-core.c we have:
static const struct iw_ioctl_description standard_ioctl[] = {
...
        [IW_IOCTL_IDX(SIOCGIWNICKN)] = {
                .header_type    = IW_HEADER_TYPE_POINT,
                .token_size     = 1,
                .max_tokens     = IW_ESSID_MAX_SIZE,
        },

So the buffer size is (strangely) IW_ESSID_MAX_SIZE

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:
- use IW_ESSID_MAX_SIZE (thanks Jeff, Kees)
- Link to v1: https://lore.kernel.org/r/20231017-strncpy-drivers-net-wireless-cisco-airo-c-v1-1-e34d5b3b7e37@google.com
---
Note: build-tested only.

Found with: $ rg "strncpy\("
---
 drivers/net/wireless/cisco/airo.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231017-strncpy-drivers-net-wireless-cisco-airo-c-d09cd0500a6e

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Jeff Johnson Oct. 27, 2023, 12:32 a.m. UTC | #1 
On 10/26/2023 4:19 PM, 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 clearly supposed to be NUL-terminated which is evident by the
> manual NUL-byte assignment as well as its immediate usage with strlen().
> 
> Moreover, let's NUL-pad since there is deliberate effort (48 instances)
> made elsewhere to zero-out buffers in these getters and setters:
> 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
> 6130 | memset(local->config.rates, 0, 8);
> 6139 | memset(local->config.rates, 0, 8);
> 6414 | memset(key.key, 0, MAX_KEY_SIZE);
> 6497 | memset(extra, 0, 16);
> (to be clear, strncpy also NUL-padded -- we are matching that behavior)
> 
> 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.
> 
> We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
> because this function is a wext handler.
> 
> In wext-core.c we have:
> static const struct iw_ioctl_description standard_ioctl[] = {
> ...
>          [IW_IOCTL_IDX(SIOCGIWNICKN)] = {
>                  .header_type    = IW_HEADER_TYPE_POINT,
>                  .token_size     = 1,
>                  .max_tokens     = IW_ESSID_MAX_SIZE,
>          },
> 
> So the buffer size is (strangely) IW_ESSID_MAX_SIZE
> 
> 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: Jeff Johnson <quic_jjohnson@quicinc.com>
Kees Cook Oct. 27, 2023, 3:59 p.m. UTC | #2 
On Thu, Oct 26, 2023 at 11:19:18PM +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 clearly supposed to be NUL-terminated which is evident by the
> manual NUL-byte assignment as well as its immediate usage with strlen().
> 
> Moreover, let's NUL-pad since there is deliberate effort (48 instances)
> made elsewhere to zero-out buffers in these getters and setters:
> 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
> 6130 | memset(local->config.rates, 0, 8);
> 6139 | memset(local->config.rates, 0, 8);
> 6414 | memset(key.key, 0, MAX_KEY_SIZE);
> 6497 | memset(extra, 0, 16);
> (to be clear, strncpy also NUL-padded -- we are matching that behavior)
> 
> 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.
> 
> We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
> because this function is a wext handler.
> 
> In wext-core.c we have:
> static const struct iw_ioctl_description standard_ioctl[] = {
> ...
>         [IW_IOCTL_IDX(SIOCGIWNICKN)] = {
>                 .header_type    = IW_HEADER_TYPE_POINT,
>                 .token_size     = 1,
>                 .max_tokens     = IW_ESSID_MAX_SIZE,
>         },
> 
> So the buffer size is (strangely) IW_ESSID_MAX_SIZE
> 
> 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>

Looks good; thanks!

Reviewed-by: Kees Cook <keescook@chromium.org>
Kalle Valo Oct. 27, 2023, 5:13 p.m. UTC | #3 
Kees Cook <keescook@chromium.org> writes:

> On Thu, Oct 26, 2023 at 11:19:18PM +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 clearly supposed to be NUL-terminated which is evident by the
>> manual NUL-byte assignment as well as its immediate usage with strlen().
>> 
>> Moreover, let's NUL-pad since there is deliberate effort (48 instances)
>> made elsewhere to zero-out buffers in these getters and setters:
>> 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
>> 6130 | memset(local->config.rates, 0, 8);
>> 6139 | memset(local->config.rates, 0, 8);
>> 6414 | memset(key.key, 0, MAX_KEY_SIZE);
>> 6497 | memset(extra, 0, 16);
>> (to be clear, strncpy also NUL-padded -- we are matching that behavior)
>> 
>> 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.
>> 
>> We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
>> because this function is a wext handler.
>> 
>> In wext-core.c we have:
>> static const struct iw_ioctl_description standard_ioctl[] = {
>> ...
>>         [IW_IOCTL_IDX(SIOCGIWNICKN)] = {
>>                 .header_type    = IW_HEADER_TYPE_POINT,
>>                 .token_size     = 1,
>>                 .max_tokens     = IW_ESSID_MAX_SIZE,
>>         },
>> 
>> So the buffer size is (strangely) IW_ESSID_MAX_SIZE
>> 
>> 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>
>
> Looks good; thanks!
>
> Reviewed-by: Kees Cook <keescook@chromium.org>

BTW most likely next week this driver and a bunch of other ancient
drivers will removed:

https://patchwork.kernel.org/project/linux-wireless/list/?series=795639&state=*&order=date

So to avoid unnecessary work on already removed drivers I recommend
using wireless-next as the baseline for wireless patches. Though I'm
still planning to apply this patch in case we ever add the driver back
(I hope not).
Kalle Valo Oct. 30, 2023, 5:24 p.m. UTC | #4 
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 clearly supposed to be NUL-terminated which is evident by the
> manual NUL-byte assignment as well as its immediate usage with strlen().
> 
> Moreover, let's NUL-pad since there is deliberate effort (48 instances)
> made elsewhere to zero-out buffers in these getters and setters:
> 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
> 6130 | memset(local->config.rates, 0, 8);
> 6139 | memset(local->config.rates, 0, 8);
> 6414 | memset(key.key, 0, MAX_KEY_SIZE);
> 6497 | memset(extra, 0, 16);
> (to be clear, strncpy also NUL-padded -- we are matching that behavior)
> 
> 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.
> 
> We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
> because this function is a wext handler.
> 
> In wext-core.c we have:
> static const struct iw_ioctl_description standard_ioctl[] = {
> ...
>         [IW_IOCTL_IDX(SIOCGIWNICKN)] = {
>                 .header_type    = IW_HEADER_TYPE_POINT,
>                 .token_size     = 1,
>                 .max_tokens     = IW_ESSID_MAX_SIZE,
>         },
> 
> So the buffer size is (strangely) IW_ESSID_MAX_SIZE
> 
> 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: Jeff Johnson <quic_jjohnson@quicinc.com>

Patch applied to wireless-next.git, thanks.

9beac4ee4928 wifi: airo: replace deprecated strncpy with strscpy_pad
diff mbox series

Patch

diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
index dbd13f7aa3e6..6a099642e854 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -6067,8 +6067,7 @@  static int airo_get_nick(struct net_device *dev,
 	struct airo_info *local = dev->ml_priv;
 
 	readConfigRid(local, 1);
-	strncpy(extra, local->config.nodeName, 16);
-	extra[16] = '\0';
+	strscpy_pad(extra, local->config.nodeName, IW_ESSID_MAX_SIZE);
 	dwrq->length = strlen(extra);
 
 	return 0;