[PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Justin Stitt]

strncpy() is deprecated [1] and we should prefer less ambiguous
interfaces.

In this case, arvif->u.ap.ssid has its length maintained by
arvif->u.ap.ssid_len which indicates it may not need to be
NUL-terminated, although by virtue of using strtomem_pad (with NUL-byte
pad character) and having a destination size larger than the source,
ssid will, incidentally, be NUL-terminated here.

As strtomem_pad() docs say:
 * @dest: Pointer of destination character array (marked as __nonstring)
 * @src: Pointer to NUL-terminated string
 * @pad: Padding character to fill any remaining bytes of @dest after copy
 *
 * This is a replacement for strncpy() uses where the destination is not
 * a NUL-terminated string, but with bounds checking on the source size, and
 * an explicit padding character. If padding is not required, use strtomem().

Let's also mark ath10k_vif.u.ap.ssid as __nonstring.

It is unclear to me whether padding is strictly necessary. Perhaps we
should opt for just strtomem() -- padding certainly doesn't hurt,
though.

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>
---
Note: build-tested only.

Found with: $ rg "strncpy\("
---
 drivers/net/wireless/ath/ath10k/core.h | 2 +-
 drivers/net/wireless/ath/ath10k/mac.c  | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4b5239de4018..ba9795a8378a 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -607,7 +607,7 @@ struct ath10k_vif {
 			u8 tim_bitmap[64];
 			u8 tim_len;
 			u32 ssid_len;
-			u8 ssid[IEEE80211_MAX_SSID_LEN];
+			u8 ssid[IEEE80211_MAX_SSID_LEN] __nonstring;
 			bool hidden_ssid;
 			/* P2P_IE with NoA attribute for P2P_GO case */
 			u32 noa_len;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 03e7bc5b6c0b..7daa007bd8b3 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -6125,8 +6125,7 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
 
 		if (ieee80211_vif_is_mesh(vif)) {
 			/* mesh doesn't use SSID but firmware needs it */
-			strncpy(arvif->u.ap.ssid, "mesh",
-				sizeof(arvif->u.ap.ssid));
+			strtomem_pad(arvif->u.ap.ssid, "mesh", '\0');
 			arvif->u.ap.ssid_len = 4;
 		}
 	}

---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231013-strncpy-drivers-net-wireless-ath-ath10k-mac-c-c73a55666e6a

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

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Jeff Johnson]

On 10/13/2023 1:33 PM, Justin Stitt wrote:
> strncpy() is deprecated [1] and we should prefer less ambiguous
> interfaces.
> 
> In this case, arvif->u.ap.ssid has its length maintained by
> arvif->u.ap.ssid_len which indicates it may not need to be
> NUL-terminated, although by virtue of using strtomem_pad (with NUL-byte
> pad character) and having a destination size larger than the source,
> ssid will, incidentally, be NUL-terminated here.
> 
> As strtomem_pad() docs say:
>   * @dest: Pointer of destination character array (marked as __nonstring)
>   * @src: Pointer to NUL-terminated string
>   * @pad: Padding character to fill any remaining bytes of @dest after copy
>   *
>   * This is a replacement for strncpy() uses where the destination is not
>   * a NUL-terminated string, but with bounds checking on the source size, and
>   * an explicit padding character. If padding is not required, use strtomem().
> 
> Let's also mark ath10k_vif.u.ap.ssid as __nonstring.

what criteria is used to determine whether or not to use __nonstring?
doesn't the use of u8 vs char already communicate that distinction?
just want to know what other u8 arrays might require this.
FWIW the documentation referenced by the __nonstring macro explicitly 
refers to "type array of char, signed char, or unsigned char"

> 
> It is unclear to me whether padding is strictly necessary. Perhaps we
> should opt for just strtomem() -- padding certainly doesn't hurt,
> though.

concur that padding probably isn't necessary but doesn't hurt, and will 
prevent confusion if looking at this member in a crashdump

> 
> 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>

Either with or without the __nonstring...
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Kees Cook]

On Fri, Oct 13, 2023 at 05:58:03PM -0700, Jeff Johnson wrote:
> On 10/13/2023 1:33 PM, Justin Stitt wrote:
> > strncpy() is deprecated [1] and we should prefer less ambiguous
> > interfaces.
> > 
> > In this case, arvif->u.ap.ssid has its length maintained by
> > arvif->u.ap.ssid_len which indicates it may not need to be
> > NUL-terminated, although by virtue of using strtomem_pad (with NUL-byte
> > pad character) and having a destination size larger than the source,
> > ssid will, incidentally, be NUL-terminated here.
> > 
> > As strtomem_pad() docs say:
> >   * @dest: Pointer of destination character array (marked as __nonstring)
> >   * @src: Pointer to NUL-terminated string
> >   * @pad: Padding character to fill any remaining bytes of @dest after copy
> >   *
> >   * This is a replacement for strncpy() uses where the destination is not
> >   * a NUL-terminated string, but with bounds checking on the source size, and
> >   * an explicit padding character. If padding is not required, use strtomem().
> > 
> > Let's also mark ath10k_vif.u.ap.ssid as __nonstring.
> 
> what criteria is used to determine whether or not to use __nonstring?
> doesn't the use of u8 vs char already communicate that distinction?
> just want to know what other u8 arrays might require this.
> FWIW the documentation referenced by the __nonstring macro explicitly refers
> to "type array of char, signed char, or unsigned char"

The use of __nonstring is for byte arrays that are _not_ expected to be
%NUL terminated. Unfortunately "char" vs "u8" isn't distinguished by the
compiler. All byte arrays are treated as C strings unless __nonstring is
used.

> > It is unclear to me whether padding is strictly necessary. Perhaps we
> > should opt for just strtomem() -- padding certainly doesn't hurt,
> > though.
> 
> concur that padding probably isn't necessary but doesn't hurt, and will
> prevent confusion if looking at this member in a crashdump
> 
> > 
> > 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>
> 
> Either with or without the __nonstring...
> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Yup, it looks like the ssid member is passed around with memcpy()
everywhere else.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Jeff Johnson]

On 10/18/2023 4:35 PM, Kees Cook wrote:
> On Fri, Oct 13, 2023 at 05:58:03PM -0700, Jeff Johnson wrote:
>>> Let's also mark ath10k_vif.u.ap.ssid as __nonstring.
>>
>> what criteria is used to determine whether or not to use __nonstring?
>> doesn't the use of u8 vs char already communicate that distinction?
>> just want to know what other u8 arrays might require this.
>> FWIW the documentation referenced by the __nonstring macro explicitly refers
>> to "type array of char, signed char, or unsigned char"
> 
> The use of __nonstring is for byte arrays that are _not_ expected to be
> %NUL terminated. Unfortunately "char" vs "u8" isn't distinguished by the
> compiler. All byte arrays are treated as C strings unless __nonstring is
> used.

So is the plan to annotate every single binary blob array in the kernel 
as __nonstring? I suspect those outnumber string arrays.

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Kalle Valo]

Justin Stitt <justinstitt@google.com> writes:

> strncpy() is deprecated [1] and we should prefer less ambiguous
> interfaces.
>
> In this case, arvif->u.ap.ssid has its length maintained by
> arvif->u.ap.ssid_len which indicates it may not need to be
> NUL-terminated, although by virtue of using strtomem_pad (with NUL-byte
> pad character) and having a destination size larger than the source,
> ssid will, incidentally, be NUL-terminated here.
>
> As strtomem_pad() docs say:
>  * @dest: Pointer of destination character array (marked as __nonstring)
>  * @src: Pointer to NUL-terminated string
>  * @pad: Padding character to fill any remaining bytes of @dest after copy
>  *
>  * This is a replacement for strncpy() uses where the destination is not
>  * a NUL-terminated string, but with bounds checking on the source size, and
>  * an explicit padding character. If padding is not required, use strtomem().
>
> Let's also mark ath10k_vif.u.ap.ssid as __nonstring.
>
> It is unclear to me whether padding is strictly necessary. Perhaps we
> should opt for just strtomem() -- padding certainly doesn't hurt,
> though.
>
> 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>
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
>  drivers/net/wireless/ath/ath10k/core.h | 2 +-
>  drivers/net/wireless/ath/ath10k/mac.c  | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 4b5239de4018..ba9795a8378a 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -607,7 +607,7 @@ struct ath10k_vif {
>  			u8 tim_bitmap[64];
>  			u8 tim_len;
>  			u32 ssid_len;
> -			u8 ssid[IEEE80211_MAX_SSID_LEN];
> +			u8 ssid[IEEE80211_MAX_SSID_LEN] __nonstring;
>  			bool hidden_ssid;
>  			/* P2P_IE with NoA attribute for P2P_GO case */
>  			u32 noa_len;
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 03e7bc5b6c0b..7daa007bd8b3 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -6125,8 +6125,7 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
>  
>  		if (ieee80211_vif_is_mesh(vif)) {
>  			/* mesh doesn't use SSID but firmware needs it */
> -			strncpy(arvif->u.ap.ssid, "mesh",
> -				sizeof(arvif->u.ap.ssid));
> +			strtomem_pad(arvif->u.ap.ssid, "mesh", '\0');
>  			arvif->u.ap.ssid_len = 4;
>  		}
>  	}

Using NUL-termination with SSID makes me always cringe as back in the
day we had so many bad implementations which didn't use SSID with
specific length parameter. The firmware should only check for ssid_len
(though I didn't check) so I find confusing that here we are suddenly
NUL-terminating it.

What about using just memcpy() to make it clear it's not really a proper
string:

arvif->u.ap.ssid_len = 4;
memcpy(arvif->u.ap.ssid, "mesh", arvif->u.ap.ssid_len);

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Jeff Johnson]

On 10/24/2023 6:03 AM, Kalle Valo wrote:
> What about using just memcpy() to make it clear it's not really a proper
> string:
> 
> arvif->u.ap.ssid_len = 4;
> memcpy(arvif->u.ap.ssid, "mesh", arvif->u.ap.ssid_len);
> 

In the "changed & BSS_CHANGED_SSID" case that comes soon after this we 
just set the length and use memcpy without clearing the rest of the 
buffer, so doing the same here, as you suggest, would be consistent.

/jeff

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Kees Cook]

On Tue, Oct 24, 2023 at 07:11:51AM -0700, Jeff Johnson wrote:
> On 10/24/2023 6:03 AM, Kalle Valo wrote:
> > What about using just memcpy() to make it clear it's not really a proper
> > string:
> > 
> > arvif->u.ap.ssid_len = 4;
> > memcpy(arvif->u.ap.ssid, "mesh", arvif->u.ap.ssid_len);
> > 
> 
> In the "changed & BSS_CHANGED_SSID" case that comes soon after this we just
> set the length and use memcpy without clearing the rest of the buffer, so
> doing the same here, as you suggest, would be consistent.

Ah, please ignore my other email asking about memcpy safety -- I'm
reading threads backwards.  :)

-- 
Kees Cook

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Jeff Johnson]

On 10/24/2023 2:43 PM, Kees Cook wrote:
> On Tue, Oct 24, 2023 at 07:11:51AM -0700, Jeff Johnson wrote:
>> On 10/24/2023 6:03 AM, Kalle Valo wrote:
>>> What about using just memcpy() to make it clear it's not really a proper
>>> string:
>>>
>>> arvif->u.ap.ssid_len = 4;
>>> memcpy(arvif->u.ap.ssid, "mesh", arvif->u.ap.ssid_len);
>>>
>>
>> In the "changed & BSS_CHANGED_SSID" case that comes soon after this we just
>> set the length and use memcpy without clearing the rest of the buffer, so
>> doing the same here, as you suggest, would be consistent.
> 
> Ah, please ignore my other email asking about memcpy safety -- I'm
> reading threads backwards.  :)
> 
And I'm replying without first reading through my mail queue

Re: [PATCH] ath10k: replace deprecated strncpy with strtomem_pad

[Kees Cook]

On Tue, Oct 24, 2023 at 04:25:35PM -0700, Jeff Johnson wrote:
> On 10/24/2023 2:43 PM, Kees Cook wrote:
> > On Tue, Oct 24, 2023 at 07:11:51AM -0700, Jeff Johnson wrote:
> > > On 10/24/2023 6:03 AM, Kalle Valo wrote:
> > > > What about using just memcpy() to make it clear it's not really a proper
> > > > string:
> > > > 
> > > > arvif->u.ap.ssid_len = 4;
> > > > memcpy(arvif->u.ap.ssid, "mesh", arvif->u.ap.ssid_len);
> > > > 
> > > 
> > > In the "changed & BSS_CHANGED_SSID" case that comes soon after this we just
> > > set the length and use memcpy without clearing the rest of the buffer, so
> > > doing the same here, as you suggest, would be consistent.
> > 
> > Ah, please ignore my other email asking about memcpy safety -- I'm
> > reading threads backwards.  :)
> > 
> And I'm replying without first reading through my mail queue

*high five* :)

-- 
Kees Cook