| Message ID | 20231013-strncpy-drivers-net-wireless-ath-ath10k-mac-c-v1-1-24e40201afa3@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: replace deprecated strncpy with strtomem_pad | expand |
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>
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>
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 <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);
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 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. :)
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
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* :)
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; } }
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(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231013-strncpy-drivers-net-wireless-ath-ath10k-mac-c-c73a55666e6a Best regards, -- Justin Stitt <justinstitt@google.com>