Re: [PATCH] wifi: p54: fix incorrect frame length check in p54_find_ie()

[Christian Lamparter <chunkeey@gmail.com>]

On 9/3/26 10:12 AM, Wang Yan wrote:
> In p54_find_ie(), mgmt is a pointer to struct ieee80211_mgmt, so
> sizeof(mgmt) evaluates to the size of the pointer rather than the size
> of the management frame header.
> 
> Use sizeof(*mgmt) instead so that the skb length is compared against
> the actual size of the management frame header.
> 
> Fixes: 0ac0d6cedf61 ("p54: Move mac80211 glue code")

It's older than that. I traced it back to:
Fixes: e5ea92a7528d ("p54: AP & Ad-hoc testing")

> Signed-off-by: Wang Yan <wangyan01@kylinos.cn>
> ---
>   drivers/net/wireless/intersil/p54/main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index 57a62108cbc3..d3e1776174f9 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
> @@ -76,7 +76,7 @@ u8 *p54_find_ie(struct sk_buff *skb, u8 ie)
>   	struct ieee80211_mgmt *mgmt = (void *)skb->data;
>   	u8 *pos, *end;
>   
> -	if (skb->len <= sizeof(mgmt))
> +	if (skb->len <= sizeof(*mgmt))
>   		return NULL;

In theory this check is actually superfluous. Reason being the rest of the code of this function:

|        pos = (u8 *)mgmt->u.beacon.variable;
|        end = skb->data + skb->len;
|        while (pos < end) {
|                if (pos + 2 + pos[1] > end)
|                        return NULL;
|
|                if (pos[0] == ie)
|                        return pos;
|
|                pos += 2 + pos[1];
|        }
|        return NULL;

The check in the while loop and the checks within the while loop make sure that
no "pos" is returned unless the IE is still within skb->len.

But true, it should have been *mgmt and not mgmt.
So:
Acked-by: Christian Lamparter <chunkeey@gmail.com>

That said, if you want to respin and remove this check, I would also ack it.