Re: [PATCH v2] wifi: p54: Fix memory leak in p54_beacon_update()

[Christian Lamparter <chunkeey@gmail.com>]

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.