In p54_beacon_update(), beacon is allocated via ieee80211_beacon_get().
If p54_beacon_format_ie_tim() fails, the function returns immediately
without freeing the allocated beacon skb, leading to a memory leak.

Since no other references to this memory exist, it must be freed locally
before returning the error. Fix this by freeing the buffer using
dev_kfree_skb_any() in the error path.

Compile tested only. Issue found using a prototype static analysis tool
and code review.

Fixes: e5ea92a7528d ("p54: AP & Ad-hoc testing")
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
Changes in v2:
- Correct the Fixes tag to point to the commit that introduced this issue.

 drivers/net/wireless/intersil/p54/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
index 2ec3655f1a9c..57a62108cbc3 100644
--- a/drivers/net/wireless/intersil/p54/main.c
+++ b/drivers/net/wireless/intersil/p54/main.c
@@ -143,8 +143,10 @@ static int p54_beacon_update(struct p54_common *priv,
 	if (!beacon)
 		return -ENOMEM;
 	ret = p54_beacon_format_ie_tim(beacon);
-	if (ret)
+	if (ret) {
+		dev_kfree_skb_any(beacon);
 		return ret;
+	}
 
 	/*
 	 * During operation, the firmware takes care of beaconing.
-- 
2.34.1

Hi,

I'm sorry for not seeing this sooner. Yes, 24hrs are passed.

On 1/20/26 2:01 PM, Zilin Guan wrote:
> In p54_beacon_update(), beacon is allocated via ieee80211_beacon_get().
> If p54_beacon_format_ie_tim() fails, the function returns immediately
> without freeing the allocated beacon skb, leading to a memory leak.
> 
> Since no other references to this memory exist, it must be freed locally
> before returning the error. Fix this by freeing the buffer using
> dev_kfree_skb_any() in the error path.
> 
> Compile tested only. Issue found using a prototype static analysis tool
> and code review.

Ok, from what I remember, this return basically is/was and likely will be a dead-code path.
So adding something there is only there to "look" good for the static analysis tools.
But many commits like these have been merged before. As long as it is mentioned that
static analysis was the reason for this. Yeah sure why not.


Reason being why this is dead-code is that in order for the path to trigger, mac80211's
ieee80211_beacon_get must have prepared an invalid beacon (with an invalid TIM Element)
to start with... And looking at ieee80211_beacon_add_tim_pvb, it still looks to me like
the IE length can't be less than 3 ever. But, I've been wrong before, if you do see please
correct me. (If not, you don't neet to really bother with the Fixes-Tag)


Cheers,
Christian

> 
> Fixes: e5ea92a7528d ("p54: AP & Ad-hoc testing")
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
> Changes in v2:
> - Correct the Fixes tag to point to the commit that introduced this issue.
> 
>   drivers/net/wireless/intersil/p54/main.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index 2ec3655f1a9c..57a62108cbc3 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
> @@ -143,8 +143,10 @@ static int p54_beacon_update(struct p54_common *priv,
>   	if (!beacon)
>   		return -ENOMEM;
>   	ret = p54_beacon_format_ie_tim(beacon);
> -	if (ret)
> +	if (ret) {
> +		dev_kfree_skb_any(beacon);
>   		return ret;
> +	}

Hmm

>   
>   	/*
>   	 * During operation, the firmware takes care of beaconing.

On Tue, Jan 20, 2026 at 09:25:15PM +0100, Christian Lamparter wrote:
> Ok, from what I remember, this return basically is/was and likely will be a dead-code path.
> So adding something there is only there to "look" good for the static analysis tools.
> But many commits like these have been merged before. As long as it is mentioned that
> static analysis was the reason for this. Yeah sure why not.
> 
> 
> Reason being why this is dead-code is that in order for the path to trigger, mac80211's
> ieee80211_beacon_get must have prepared an invalid beacon (with an invalid TIM Element)
> to start with... And looking at ieee80211_beacon_add_tim_pvb, it still looks to me like
> the IE length can't be less than 3 ever. But, I've been wrong before, if you do see please
> correct me. (If not, you don't neet to really bother with the Fixes-Tag)
> 
> 
> Cheers,
> Christian

Hi Christian,

Thanks for the detailed review.

I agree with your analysis. I checked the code and confirmed that mac80211 
guarantees a minimum TIM length of 4 bytes for non-S1G devices.

I appreciate you accepting this patch to silence the static analysis warning.

Best regards,
Zilin Guan

Hi Zilin,

On 1/21/26 10:05 AM, Zilin Guan wrote:
> On Tue, Jan 20, 2026 at 09:25:15PM +0100, Christian Lamparter wrote:
>> Ok, from what I remember, this return basically is/was and likely will be a dead-code path.
>> So adding something there is only there to "look" good for the static analysis tools.
>> But many commits like these have been merged before. As long as it is mentioned that
>> static analysis was the reason for this. Yeah sure why not.
>>
>>
>> Reason being why this is dead-code is that in order for the path to trigger, mac80211's
>> ieee80211_beacon_get must have prepared an invalid beacon (with an invalid TIM Element)
>> to start with... And looking at ieee80211_beacon_add_tim_pvb, it still looks to me like
>> the IE length can't be less than 3 ever. But, I've been wrong before, if you do see please
>> correct me. (If not, you don't neet to really bother with the Fixes-Tag)
> 
> I agree with your analysis. I checked the code and confirmed that mac80211
> guarantees a minimum TIM length of 4 bytes for non-S1G devices.
> 
> I appreciate you accepting this patch to silence the static analysis warning.

Phew, and so far no buildbot replied with comments.

I have one last request: Can you please add a sentence about that analysis into the commit log as well?
Our future selves could maybe appreciate that one day, if this comes up again. Because then we won't have
to remember all or search/look for it again, if it's already neatly written down directly there.

Thank you,
Christian

On Thu, Jan 22, 2026 at 09:14:50AM +0100, Christian Lamparter wrote:
> Hi Zilin,
> 
> On 1/21/26 10:05 AM, Zilin Guan wrote:
> > On Tue, Jan 20, 2026 at 09:25:15PM +0100, Christian Lamparter wrote:
> >> Ok, from what I remember, this return basically is/was and likely will be a dead-code path.
> >> So adding something there is only there to "look" good for the static analysis tools.
> >> But many commits like these have been merged before. As long as it is mentioned that
> >> static analysis was the reason for this. Yeah sure why not.
> >>
> >>
> >> Reason being why this is dead-code is that in order for the path to trigger, mac80211's
> >> ieee80211_beacon_get must have prepared an invalid beacon (with an invalid TIM Element)
> >> to start with... And looking at ieee80211_beacon_add_tim_pvb, it still looks to me like
> >> the IE length can't be less than 3 ever. But, I've been wrong before, if you do see please
> >> correct me. (If not, you don't neet to really bother with the Fixes-Tag)
> > 
> > I agree with your analysis. I checked the code and confirmed that mac80211
> > guarantees a minimum TIM length of 4 bytes for non-S1G devices.
> > 
> > I appreciate you accepting this patch to silence the static analysis warning.
> 
> Phew, and so far no buildbot replied with comments.
> 
> I have one last request: Can you please add a sentence about that analysis into the commit log as well?
> Our future selves could maybe appreciate that one day, if this comes up again. Because then we won't have
> to remember all or search/look for it again, if it's already neatly written down directly there.
> 
> Thank you,
> Christian

Hi Christian,

Sure, I will add a note about the analysis to the commit message as 
requested and send a v3 patch shortly.

Thanks,
Zilin