| Message ID | 20230809081241.32765-1-quic_wgong@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] wifi: ath12k: Fix buffer overflow when scanning with extraie | expand |
On 8/9/2023 1:12 AM, 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 problem is 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 <quic_wgong@quicinc.com> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> > --- > v2: seperate to another patch per johannes. > > drivers/net/wireless/ath/ath12k/wmi.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c > index 9ed33e2d6da0..cc9a377c06fd 100644 > --- a/drivers/net/wireless/ath/ath12k/wmi.c > +++ b/drivers/net/wireless/ath/ath12k/wmi.c > @@ -2221,8 +2221,7 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar, > struct wmi_tlv *tlv; > void *ptr; > int i, ret, len; > - u32 *tmp_ptr; > - u8 extraie_len_with_pad = 0; > + u32 *tmp_ptr, extraie_len_with_pad = 0; > struct ath12k_wmi_hint_short_ssid_arg *s_ssid = NULL; > struct ath12k_wmi_hint_bssid_arg *hint_bssid = NULL; > > > base-commit: 3f257461ab0ab19806bae2bfde4c3cd88dbf050e
On 8/9/2023 10:31 AM, Jeff Johnson wrote: > On 8/9/2023 1:12 AM, 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 problem is 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 <quic_wgong@quicinc.com> > > Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> Wen, can you please add a Fixes: tag since based upon the discussion you actually observed a crash
On 8/10/2023 2:16 AM, Jeff Johnson wrote: > On 8/9/2023 10:31 AM, Jeff Johnson wrote: >> On 8/9/2023 1:12 AM, Wen Gong wrote: >>> [...] >> >> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> > > Wen, can you please add a Fixes: tag since based upon the discussion > you actually observed a crash > Jeff, do you mean I should add the crash call stack or other thing in this patch? The crash is observed by Sven Eckelmann <sven@narfation.org> on 07 Dec 2021 here: Subject: Re: [PATCH] ath11k: enable IEEE80211_HW_SINGLE_SCAN_ON_ALL_BANDS for WCN6855 https://lore.kernel.org/linux-wireless/3267805.el9kkjlfUZ@ripper/
On Thursday, 10 August 2023 06:31:02 CEST Wen Gong wrote: > On 8/10/2023 2:16 AM, Jeff Johnson wrote: > > On 8/9/2023 10:31 AM, Jeff Johnson wrote: > >> On 8/9/2023 1:12 AM, Wen Gong wrote: > >>> > [...] > >> > >> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> > > > > Wen, can you please add a Fixes: tag since based upon the discussion > > you actually observed a crash > > > Jeff, do you mean I should add the crash call stack or other thing in > this patch? I think a reference to the commit which is fixed should be added. > The crash is observed by Sven Eckelmann <sven@narfation.org> on 07 Dec > 2021 here: > Subject: Re: [PATCH] ath11k: enable > IEEE80211_HW_SINGLE_SCAN_ON_ALL_BANDS for WCN6855 > https://lore.kernel.org/linux-wireless/3267805.el9kkjlfUZ@ripper/ This was for ath11k. See my patch for it in https://lore.kernel.org/r/20211207142913.1734635-1-sven@narfation.org So I doubt that it is ok to add the same backtrace for an ath12k commit. And if I compare both patches, it looks to me that you don't handle the params->extraie.len > 16 bit (see WMI_TLV_LEN) in ath12k. Kind regards, Sven
On 8/10/2023 4:09 PM, Sven Eckelmann wrote: > On Thursday, 10 August 2023 06:31:02 CEST Wen Gong wrote: >> On 8/10/2023 2:16 AM, Jeff Johnson wrote: >>> On 8/9/2023 10:31 AM, Jeff Johnson wrote: >>>> On 8/9/2023 1:12 AM, Wen Gong wrote: >> [...] >>>> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> >>> Wen, can you please add a Fixes: tag since based upon the discussion >>> you actually observed a crash >>> >> Jeff, do you mean I should add the crash call stack or other thing in >> this patch? > I think a reference to the commit which is fixed should be added. > >> The crash is observed by Sven Eckelmann <sven@narfation.org> on 07 Dec >> 2021 here: >> Subject: Re: [PATCH] ath11k: enable >> IEEE80211_HW_SINGLE_SCAN_ON_ALL_BANDS for WCN6855 >> https://lore.kernel.org/linux-wireless/3267805.el9kkjlfUZ@ripper/ > This was for ath11k. See my patch for it in > https://lore.kernel.org/r/20211207142913.1734635-1-sven@narfation.org > So I doubt that it is ok to add the same backtrace for an ath12k commit. > > And if I compare both patches, it looks to me that you don't handle the > params->extraie.len > 16 bit (see WMI_TLV_LEN) in ath12k. > > Kind regards, > Sven I added similar check here: [v2] wifi: ath12k: add check max message length while scanning with extraie https://patchwork.kernel.org/project/linux-wireless/patch/20230809081657.13858-1-quic_wgong@quicinc.com/
On Thursday, 10 August 2023 10:09:25 CEST Sven Eckelmann wrote: [...] > This was for ath11k. See my patch for it in > https://lore.kernel.org/r/20211207142913.1734635-1-sven@narfation.org > So I doubt that it is ok to add the same backtrace for an ath12k commit. > > And if I compare both patches, it looks to me that you don't handle the > params->extraie.len > 16 bit (see WMI_TLV_LEN) in ath12k. Ok, just saw that the v1 had handling for that but it was not split into two patches. https://lore.kernel.org/r/20230809081657.13858-1-quic_wgong@quicinc.com So just ignore this remark. Kind regards, Sven
On 8/9/2023 9:31 PM, Wen Gong wrote: > On 8/10/2023 2:16 AM, Jeff Johnson wrote: >> On 8/9/2023 10:31 AM, Jeff Johnson wrote: >>> On 8/9/2023 1:12 AM, Wen Gong wrote: >>>> > [...] >>> >>> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> >> >> Wen, can you please add a Fixes: tag since based upon the discussion >> you actually observed a crash >> > Jeff, do you mean I should add the crash call stack or other thing in > this patch? > > The crash is observed by Sven Eckelmann <sven@narfation.org> on 07 Dec > 2021 here: > Subject: Re: [PATCH] ath11k: enable > IEEE80211_HW_SINGLE_SCAN_ON_ALL_BANDS for WCN6855 > https://lore.kernel.org/linux-wireless/3267805.el9kkjlfUZ@ripper/ > > > It isn't necessary to add a call stack. Based upon the above you should add both a Fixes: tag and a Link: tag. These go in the tags section of the commit text before the Signed-off-by: <https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes> A Fixes: tag indicates that the patch fixes an issue in a previous commit. It is used to make it easy to determine where a bug originated, which can help review a bug fix. This tag also assists the stable kernel team in determining which stable kernel versions should receive your fix. This is the preferred method for indicating a bug fixed by the patch. See Describe your changes for more details. <https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes> If related discussions or any other background information behind the change can be found on the web, add 'Link:' tags pointing to it. If the patch is a result of some earlier mailing list discussions or something documented on the web, point to it.
diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c index 9ed33e2d6da0..cc9a377c06fd 100644 --- a/drivers/net/wireless/ath/ath12k/wmi.c +++ b/drivers/net/wireless/ath/ath12k/wmi.c @@ -2221,8 +2221,7 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar, struct wmi_tlv *tlv; void *ptr; int i, ret, len; - u32 *tmp_ptr; - u8 extraie_len_with_pad = 0; + u32 *tmp_ptr, extraie_len_with_pad = 0; struct ath12k_wmi_hint_short_ssid_arg *s_ssid = NULL; struct ath12k_wmi_hint_bssid_arg *hint_bssid = NULL;
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 problem is 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 <quic_wgong@quicinc.com> --- v2: seperate to another patch per johannes. drivers/net/wireless/ath/ath12k/wmi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) base-commit: 3f257461ab0ab19806bae2bfde4c3cd88dbf050e