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

[Kalle Valo]

Justin Stitt <justinstitt@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
> ---
> 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