Re: [PATCH wireless 3/3] wifi: p54: fix IE parsing loop bounds check

[Christian Lamparter <chunkeey@gmail.com>]

On 10/26/26 12:45 PM, Johannes Berg wrote:
> On Mon, 2026-10-26 at 11:21 +0000, Tristan Madani wrote:
>> From: Tristan Madani <tristan@talencesecurity.com>
>>
>> p54_find_ie() iterates over information elements using a loop condition
>> of while (pos < end). When only one byte remains (pos == end - 1),
>> the loop body accesses pos[1] which reads one byte beyond the validated
>> buffer boundary. The read happens in the bounds check expression
>> pos + 2 + pos[1] > end before the result of that check can prevent
>> the access.
>>
>> Fix by tightening the loop condition to while (pos + 2 <= end),
>> which ensures that at least the IE id (pos[0]) and length (pos[1])
>> bytes are within bounds before any access.
>>
>> Fixes: 0ac0d6cedf611 ("p54: Move mac80211 glue code")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
>> ---
>>   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 57a62108cbc35..16614f65dfa5f 100644
>> --- a/drivers/net/wireless/intersil/p54/main.c
>> +++ b/drivers/net/wireless/intersil/p54/main.c
>> @@ -81,7 +81,7 @@ u8 *p54_find_ie(struct sk_buff *skb, u8 ie)
> 
> We'd probably be better off just replacing that whole function with one
> of the real search functions or iteration macros?
> yeah, I think carl9170 probably can just straight up use cfg80211_find_ie.

But p54 needs to edit the beacon and the memmove the (found) TIM element
to the end of the beacon and modify it a bit of tim in p54_beacon_format_ie_tim.
The problem here is that the helpers are all casting to const. So this will
look "funny" via to "uncast" const. Unless you meant something else?

As for what to expect if something read past the end? Well, p54 works on skbs. So past the
"end" in p54_beacon_format_ie_tim() would either be extra tailroom then padding or the start
of skb_shared_info (u8 flags) I think? In case of p54_pspoll_workaround(), after the "end",
there will be the FCS (32-bit checksum) - at least on real hardware) and then padding or
the start of skb_shared_info.

For rt2xx: not sure. But the existing use cfg80211_find_ie in rtw89_core_cancel_6ghz_probe_tx()
and rtw89_core_cancel_6ghz_probe_tx() looks suspecious.
Because from what I gleamed, both just pass "skb->len" as then len parameter. But shouldn't
that be subtracted by the offsetof mgmt->u.beacon.variable? Because cfg80211_find_ie thinks
len starts from the "ies"? I can't tell right now as I'm sick and need to go back to bed.
This all could either be a fever dream an be true/half-true or completely wrong. So I have to
ask in advance: Please don't take this all too seriously.

Cheers,
Christian

PS: Is there also a patch for rtl_find_ie (isn't that pretty much the same as all the others?)