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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
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 <[email protected]>
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
Either with or without the __nonstring...
Acked-by: Jeff Johnson <[email protected]>
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: [email protected]
> > Signed-off-by: Justin Stitt <[email protected]>
>
> Either with or without the __nonstring...
> Acked-by: Jeff Johnson <[email protected]>
Yup, it looks like the ssid member is passed around with memcpy()
everywhere else.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
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.
Justin Stitt <[email protected]> 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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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
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
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