If cfg80211 is providing extraie's for a scanning process then ath12k will
copy that over to the firmware. The extraie.len is a 32 bit value in struct
element_info and describes the amount of bytes for the vendor information
elements.
The WMI_TLV packet is having a special WMI_TAG_ARRAY_BYTE section. This
section can have a (payload) length up to 65535 bytes because the
WMI_TLV_LEN can store up to 16 bits. The code was missing such a check and
could have created a scan request which cannot be parsed correctly by the
firmware.
But the bigger problem was the allocation of the buffer. It has to align
the TLV sections by 4 bytes. But the code was using an u8 to store the
newly calculated length of this section (with alignment). And the new
calculated length was then used to allocate the skbuff. But the actual code
to copy in the data is using the extraie.len and not the calculated
"aligned" length.
The length of extraie with IEEE80211_HW_SINGLE_SCAN_ON_ALL_BANDS enabled
was 264 bytes during tests with a wifi card. But it only allocated 8
bytes (264 bytes % 256) for it. As consequence, the code to memcpy the
extraie into the skb was then just overwriting data after skb->end. Things
like shinfo were therefore corrupted. This could usually be seen by a crash
in skb_zcopy_clear which tried to call a ubuf_info callback (using a bogus
address).
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
Signed-off-by: Wen Gong <[email protected]>
---
drivers/net/wireless/ath/ath12k/wmi.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
index 6512267ae4ca..0acd58aed79d 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.c
+++ b/drivers/net/wireless/ath/ath12k/wmi.c
@@ -2145,7 +2145,7 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
void *ptr;
int i, ret, len;
u32 *tmp_ptr;
- u8 extraie_len_with_pad = 0;
+ u32 extraie_len_with_pad = 0;
struct ath12k_wmi_hint_short_ssid_arg *s_ssid = NULL;
struct ath12k_wmi_hint_bssid_arg *hint_bssid = NULL;
@@ -2163,12 +2163,6 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
if (arg->num_bssid)
len += sizeof(*bssid) * arg->num_bssid;
- len += TLV_HDR_SIZE;
- if (arg->extraie.len)
- extraie_len_with_pad =
- roundup(arg->extraie.len, sizeof(u32));
- len += extraie_len_with_pad;
-
if (arg->num_hint_bssid)
len += TLV_HDR_SIZE +
arg->num_hint_bssid * sizeof(*hint_bssid);
@@ -2177,6 +2171,18 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
len += TLV_HDR_SIZE +
arg->num_hint_s_ssid * sizeof(*s_ssid);
+ len += TLV_HDR_SIZE;
+ if (arg->extraie.len)
+ extraie_len_with_pad =
+ roundup(arg->extraie.len, sizeof(u32));
+ if (extraie_len_with_pad <= (wmi->wmi_ab->max_msg_len[ar->pdev_idx] - len)) {
+ len += extraie_len_with_pad;
+ } else {
+ ath12k_warn(ar->ab, "discard large size %d bytes extraie for scan start\n",
+ arg->extraie.len);
+ extraie_len_with_pad = 0;
+ }
+
skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
if (!skb)
return -ENOMEM;
@@ -2266,7 +2272,7 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_BYTE, len);
ptr += TLV_HDR_SIZE;
- if (arg->extraie.len)
+ if (extraie_len_with_pad)
memcpy(ptr, arg->extraie.ptr,
arg->extraie.len);
base-commit: b21fe5be53eb873c02e7479372726c8aeed171e3
--
2.40.1
Hi,
Since I'm covering for Kalle right now ...
On Sun, 2023-08-06 at 23:08 -0400, Wen Gong wrote:
> If cfg80211 is providing extraie's for a scanning process then ath12k will
> copy that over to the firmware. The extraie.len is a 32 bit value in struct
> element_info and describes the amount of bytes for the vendor information
> elements.
>
> The WMI_TLV packet is having a special WMI_TAG_ARRAY_BYTE section. This
> section can have a (payload) length up to 65535 bytes because the
> WMI_TLV_LEN can store up to 16 bits. The code was missing such a check and
> could have created a scan request which cannot be parsed correctly by the
> firmware.
>
> But the bigger problem was the allocation of the buffer. It has to align
> the TLV sections by 4 bytes. But the code was using an u8 to store the
> newly calculated length of this section (with alignment). And the new
> calculated length was then used to allocate the skbuff. But the actual code
> to copy in the data is using the extraie.len and not the calculated
> "aligned" length.
>
> The length of extraie with IEEE80211_HW_SINGLE_SCAN_ON_ALL_BANDS enabled
> was 264 bytes during tests with a wifi card. But it only allocated 8
> bytes (264 bytes % 256) for it. As consequence, the code to memcpy the
> extraie into the skb was then just overwriting data after skb->end. Things
> like shinfo were therefore corrupted. This could usually be seen by a crash
> in skb_zcopy_clear which tried to call a ubuf_info callback (using a bogus
> address).
I feel these are two separate issues. Having a large enough TLV that the
firmware cannot parse it is highly unlikely to happen, and not really an
issue here.
Please split this into two patches, and fix *just* the buffer overflow
in a patch titled "Fix buffer overflow". I believe simply changing the
variable type is sufficient for this, as the code is otherwise
equivalent. That's a patch I'd take to wireless at this stage (rc5), but
probably not the entire bigger change.
johannes
On 8/8/2023 6:55 PM, Johannes Berg wrote:
> Hi,
>
> Since I'm covering for Kalle right now ...
>
> On Sun, 2023-08-06 at 23:08 -0400, Wen Gong wrote:
[...]
> I feel these are two separate issues. Having a large enough TLV that the
> firmware cannot parse it is highly unlikely to happen, and not really an
> issue here.
>
> Please split this into two patches, and fix *just* the buffer overflow
> in a patch titled "Fix buffer overflow". I believe simply changing the
> variable type is sufficient for this, as the code is otherwise
> equivalent. That's a patch I'd take to wireless at this stage (rc5), but
> probably not the entire bigger change.
>
> johannes
Yes, let me send the 2 patch separately .