[PATCH] wifi: ath10k: perform buffer size check in ath10k_wow_convert_8023_to_80211()

[PATCH] wifi: ath10k: perform buffer size check in ath10k_wow_convert_8023_to_80211()

[Dmitry Antipov]

Looking through the following:

-> ath10k_vif_wow_set_wakeups()
   -> ath10k_wow_convert_8023_to_80211()
      ...
      memcpy(..., ..., pattern_len);                    [1]
      ...
   <- ...
   if (WARN_ON(...packet_len > WOW_MAX_PATTERN_SIZE))   [2]
      ...

I've found that [2] makes no sense after [1]. I.e. check for possible
buffer overflow should be performed prior to touching both 'pattern' and
'mask' buffers with 'memcpy()' in 'ath10k_wow_convert_8023_to_80211()'.
Compile tested only.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/ath/ath10k/wow.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wow.c b/drivers/net/wireless/ath/ath10k/wow.c
index aa7b2e703f3d..40adb51211d0 100644
--- a/drivers/net/wireless/ath/ath10k/wow.c
+++ b/drivers/net/wireless/ath/ath10k/wow.c
@@ -78,8 +78,8 @@ static int ath10k_wow_cleanup(struct ath10k *ar)
  * 802.11: |4B|dest mac(6B)| 6B |src mac(6B)|  8B  |type(2B)|  body...  |
  *         +--+------------+----+-----------+---------------+-----------+
  */
-static void ath10k_wow_convert_8023_to_80211(struct cfg80211_pkt_pattern *new,
-					     const struct cfg80211_pkt_pattern *old)
+static int ath10k_wow_convert_8023_to_80211(struct cfg80211_pkt_pattern *new,
+					    const struct cfg80211_pkt_pattern *old)
 {
 	u8 hdr_8023_pattern[ETH_HLEN] = {};
 	u8 hdr_8023_bit_mask[ETH_HLEN] = {};
@@ -149,6 +149,9 @@ static void ath10k_wow_convert_8023_to_80211(struct cfg80211_pkt_pattern *new,
 
 	new->pattern_len = hdr_80211_end_offset - new->pkt_offset;
 
+	if (WARN_ON(new->pattern_len > WOW_MAX_PATTERN_SIZE))
+		return -EINVAL;
+
 	memcpy((u8 *)new->pattern,
 	       hdr_80211_pattern + new->pkt_offset,
 	       new->pattern_len);
@@ -167,6 +170,8 @@ static void ath10k_wow_convert_8023_to_80211(struct cfg80211_pkt_pattern *new,
 
 		new->pattern_len += total_len - ETH_HLEN;
 	}
+
+	return 0;
 }
 
 static int ath10k_wmi_pno_check(struct ath10k *ar, u32 vdev_id,
@@ -341,16 +346,16 @@ static int ath10k_vif_wow_set_wakeups(struct ath10k_vif *arvif,
 
 		if (ar->wmi.rx_decap_mode == ATH10K_HW_TXRX_NATIVE_WIFI) {
 			if (patterns[i].pkt_offset < ETH_HLEN) {
-				ath10k_wow_convert_8023_to_80211(&new_pattern,
-								 &old_pattern);
+				ret = ath10k_wow_convert_8023_to_80211(&new_pattern,
+								       &old_pattern);
 			} else {
 				new_pattern = old_pattern;
 				new_pattern.pkt_offset += WOW_HDR_LEN - ETH_HLEN;
 			}
 		}
 
-		if (WARN_ON(new_pattern.pattern_len > WOW_MAX_PATTERN_SIZE))
-			return -EINVAL;
+		if (ret)
+			return ret;
 
 		ret = ath10k_wmi_wow_add_pattern(ar, arvif->vdev_id,
 						 pattern_id,
-- 
2.47.1

Re: [PATCH] wifi: ath10k: perform buffer size check in ath10k_wow_convert_8023_to_80211()

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> Looking through the following:
>
> -> ath10k_vif_wow_set_wakeups()
>    -> ath10k_wow_convert_8023_to_80211()
>       ...
>       memcpy(..., ..., pattern_len);                    [1]
>       ...
>    <- ...
>    if (WARN_ON(...packet_len > WOW_MAX_PATTERN_SIZE))   [2]
>       ...
>
> I've found that [2] makes no sense after [1]. I.e. check for possible
> buffer overflow should be performed prior to touching both 'pattern' and
> 'mask' buffers with 'memcpy()' in 'ath10k_wow_convert_8023_to_80211()'.
> Compile tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

This code path should be tested on a real device, can anyone help with
that?

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

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