wifi: p54: fix incorrect frame length check in p54_find_ie()

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

Posted by Wang Yan 3 weeks, 1 day ago

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")
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;
 
 	pos = (u8 *)mgmt->u.beacon.variable;
-- 
2.25.1

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

Posted by Christian Lamparter 2 weeks, 5 days ago

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.

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

Posted by Wang Yan 2 weeks, 5 days ago

Hi Christian,

Thank you for the thorough review and for tracing the issue back to the original commit.

I agree with your analysis. The while loop together with the bounds check inside the loop body already ensures that no IE pointer is returned unless it lies within skb->len, making the initial length check redundant. As you also pointed out, the check itself is incorrect because sizeof(mgmt) evaluates to the pointer size rather than the size of struct ieee80211_mgmt.

I have posted a new patch that removes the superfluous check and updates the Fixes tag to e5ea92a7528d ("p54: AP & Ad‑hoc testing") as you suggested.

Thanks again for the review.

Regards,
Wang Yan

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

Posted by Johannes Berg 2 weeks, 5 days ago

On Sun, 2026-09-06 at 13:06 +0200, Christian Lamparter wrote:
> 
> >        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.

FWIW, I dropped it because it really shouldn't have been there this way
since 'mgmt' can be far bigger than needed since it contains the union
for all kinds of action frames etc.

I'm not even sure it's needed regardless of the next check since the
beacon is built by mac80211.

Just blindly patching one mistake for another doesn't help anyone.

johannes

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

Posted by Christian Lamparter 2 weeks, 5 days ago

On 9/6/26 1:49 PM, Johannes Berg wrote:
> On Sun, 2026-09-06 at 13:06 +0200, Christian Lamparter wrote:
>>
>>>         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.
> 
> FWIW, I dropped it because it really shouldn't have been there this way
> since 'mgmt' can be far bigger than needed since it contains the union
> for all kinds of action frames etc.
> 
> I'm not even sure it's needed regardless of the next check since the
> beacon is built by mac80211.
> 
> Just blindly patching one mistake for another doesn't help anyone.

Ok, alright? I just looked in both:
https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/
https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless.git/

Was there a patch already? Or did I missread the first sentence that hinted this was "dropped"?

Or do you want that Mr/Ms Yan (Sorry, but from what I was told, the name can be used for
both male and female) just post a new patch that removes the superfluous check.

About *mgmt vs mgmt: Yes, you are right. That said, that check came from sometime between
~2006-2008 ;). I don't think the struct back then already contained action frames with
sounding/beamforming/timing feedback. Still, I'm totally fine with it being "dropped" too.
If there was such a patch posted, please feel free to add a
"Acked-by: Christian Lamparter <chunkeey@gmail.com>" if you merge it.

Cheers,
Christian

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

Posted by Johannes Berg 2 weeks, 5 days ago

On Sun, 2026-09-06 at 16:24 +0200, Christian Lamparter wrote:
> > 
> > FWIW, I dropped it because it really shouldn't have been there this way
> > since 'mgmt' can be far bigger than needed since it contains the union
> > for all kinds of action frames etc.
> > 
> > I'm not even sure it's needed regardless of the next check since the
> > beacon is built by mac80211.
> > 
> > Just blindly patching one mistake for another doesn't help anyone.
> 
> Ok, alright? I just looked in both:
> https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless.git/
> 
> Was there a patch already? Or did I missread the first sentence that hinted this was "dropped"?

Oh, I didn't phrase that well - I dropped the patch from my queue.

johannes

[PATCH v2] wifi: p54: remove redundant length check in p54_find_ie()

Posted by Wang Yan 2 weeks, 5 days ago

The check in the while loop condition and the bounds check within the
loop body guarantee that "pos" is never returned unless the IE is still
within skb->len.

Additionally, sizeof(mgmt) evaluates to the pointer size rather than
the actual management frame header size, making the check incorrect as
well.

Therefore, removing the redundant check is the right fix.

Fixes: e5ea92a7528d ("p54: AP & Ad-hoc testing")
Signed-off-by: Wang Yan <wangyan01@kylinos.cn>
---
 drivers/net/wireless/intersil/p54/main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
index 57a62108cbc3..8f921dc4ecd3 100644
--- a/drivers/net/wireless/intersil/p54/main.c
+++ b/drivers/net/wireless/intersil/p54/main.c
@@ -76,9 +76,6 @@ 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))
-		return NULL;
-
 	pos = (u8 *)mgmt->u.beacon.variable;
 	end = skb->data + skb->len;
 	while (pos < end) {
-- 
2.25.1

Re: [PATCH v2] wifi: p54: remove redundant length check in p54_find_ie()

Posted by Christian Lamparter 1 week, 1 day ago

On 9/7/26 4:49 AM, Wang Yan wrote:
> The check in the while loop condition and the bounds check within the
> loop body guarantee that "pos" is never returned unless the IE is still
> within skb->len.
> 
> Additionally, sizeof(mgmt) evaluates to the pointer size rather than
> the actual management frame header size, making the check incorrect as
> well.
> 
> Therefore, removing the redundant check is the right fix.
> 
> Fixes: e5ea92a7528d ("p54: AP & Ad-hoc testing")
> Signed-off-by: Wang Yan <wangyan01@kylinos.cn>
Acked-by: Christian Lamparter <chunkeey@gmail.com>

> ---
>   drivers/net/wireless/intersil/p54/main.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index 57a62108cbc3..8f921dc4ecd3 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
> @@ -76,9 +76,6 @@ 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))
> -		return NULL;
> -
>   	pos = (u8 *)mgmt->u.beacon.variable;
>   	end = skb->data + skb->len;
>   	while (pos < end) {