linux-kernel - Re: [PATCH v2] wifi: ath11k: fix memory leaks in beacon template setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <97b132e8-d3b2-4e52-aa74-c8aeb0f5d516@oss.qualcomm.com>
Date: Thu, 29 Jan 2026 07:36:43 -0800
From: Jeff Johnson <jeff.johnson@....qualcomm.com>
To: Zilin Guan <zilin@....edu.cn>
Cc: ath11k@...ts.infradead.org, baochen.qiang@....qualcomm.com,
        jianhao.xu@....edu.cn, jjohnson@...nel.org,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org
Subject: Re: [PATCH v2] wifi: ath11k: fix memory leaks in beacon template
 setup

On 1/28/2026 10:13 PM, Zilin Guan wrote:
> On Wed, Jan 28, 2026 at 08:30:22AM -0800, Jeff Johnson wrote:
>> On 1/19/2026 10:37 PM, Zilin Guan wrote:
>>> The functions ath11k_mac_setup_bcn_tmpl_ema() and
>>> ath11k_mac_setup_bcn_tmpl_mbssid() allocate memory for beacon templates
>>> but fail to free it when parameter setup returns an error.
>>>
>>> Since beacon templates must be released during normal execution, they
>>> must also be released in the error handling paths to prevent memory
>>> leaks.
>>>
>>> Fix this by adding the missing deallocation calls in the respective
>>> error paths.
>>>
>>> Compile tested only. Issue found using a prototype static analysis tool
>>> and code review.
>>>
>>> Fixes: 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
>>> Fixes: 335a92765d30 ("wifi: ath11k: MBSSID beacon support")
>>> Suggested-by: Baochen Qiang <baochen.qiang@....qualcomm.com>
>>> Signed-off-by: Zilin Guan <zilin@....edu.cn>
>>> ---
>>> Changes in v2:
>>> - Use unified exit paths for cleanup.
>>>
>>>  drivers/net/wireless/ath/ath11k/mac.c | 25 +++++++++++++++----------
>>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
>>> index 4dfd08b58416..42edcc5e9e49 100644
>>> --- a/drivers/net/wireless/ath/ath11k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath11k/mac.c
>>> @@ -1561,8 +1561,10 @@ static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif,
>>
>> while looking to apply this patch I noticed the following logic earlier in the
>> function:
>>
>>  	beacons = ieee80211_beacon_get_template_ema_list(tx_arvif->ar->hw,
>>  							 tx_arvif->vif, 0);
>>  	if (!beacons || !beacons->cnt) {
>>  		ath11k_warn(arvif->ar->ab,
>>  			    "failed to get ema beacon templates from mac80211\n");
>>  		return -EPERM;
>>  	}
>>
>> I did not look at ieee80211_beacon_get_template_ema_list()
>> But if it is possible that this can return a valid beacons pointer with
>> beacons->cnt == 0, then won't this also leak the beacons allocation?
>>
>> Given that ieee80211_beacon_free_ema_list(beacons) can handle a NULL
>> beacons pointer, perhaps this should also goto free?
> 
> Hi Jeff,
> 
> Thanks for pointing that out.
> 
> I looked into the allocation chain for 
> ieee80211_beacon_get_template_ema_list():
> 
> ieee80211_beacon_get_template_ema_list()
> 	|__ __ieee80211_beacon_get()
> 		|__ ieee80211_beacon_get_ap_ema_list()
> 
> It seems that ieee80211_beacon_get_ap_ema_list() only returns a valid 
> pointer when ema->cnt is non-zero. Therefore, a valid beacons pointer with 
> beacons->cnt == 0 is likely unreachable under the current mac80211 
> implementation, making the existing check more of a defensive programming
> measure.
> 
> However, for the sake of strict logical consistency, it would make sense 
> to use the goto path there as well.
> 
> Do you think it's worth updating this in a v3, or is the current v2 
> sufficient given the current call logic?

I prefer strict logical consistency so I prefer either adding the goto or
removing the beacons->cnt check.

Or a completely different approach would be to use cleanup.h functionality and
annotate beacons with __free(ieee80211_beacon_free_ema_list) so that no
explicit calls to that function are required. If you try this approach then
beacons must be defined at the point of allocation:

	struct ieee80211_ema_beacons *beacons __free(ieee80211_beacon_free_ema_list) =
		ieee80211_beacon_get_template_ema_list(tx_arvif->ar->hw,
						       tx_arvif->vif, 0);

Note that I have not tried this approach with allocations other than from the
kmalloc() family with __free(kfree), but in theory this should work.

/jeff