linux-kernel - Re: [PATCH] wifi: mac80211: Fix ieee80211_convert_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a4167f39-b2e5-4b86-b85f-a1dad4669fec@quicinc.com>
Date: Fri, 16 Aug 2024 13:04:57 -0700
From: Jeff Johnson <quic_jjohnson@...cinc.com>
To: Felix Fietkau <nbd@....name>,
        Toke Høiland-Jørgensen
	<toke@...hat.com>,
        Johannes Berg <johannes@...solutions.net>,
        "David S.
 Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        Michael Braun
	<michael-dev@...i-braun.de>
CC: Harsh Kumar Bijlani <hbijlani@....qualcomm.com>,
        Kalyan Tallapragada
	<ktallapr@....qualcomm.com>,
        Jyothi Chukkapalli <jchukkap@....qualcomm.com>,
        Anirban Sirkhell <anirban@....qualcomm.com>,
        Johannes Berg
	<johannes.berg@...el.com>,
        <linux-wireless@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <ath12k@...ts.infradead.org>
Subject: Re: [PATCH] wifi: mac80211: Fix ieee80211_convert_to_unicast() logic

On 8/16/2024 10:54 AM, Felix Fietkau wrote:
> On 16.08.24 16:30, Jeff Johnson wrote:
>> On 8/16/2024 4:31 AM, Toke Høiland-Jørgensen wrote:
>>> Jeff Johnson <quic_jjohnson@...cinc.com> writes:
>>>
>>>> The current logic in ieee80211_convert_to_unicast() uses skb_clone()
>>>> to obtain an skb for each individual destination of a multicast
>>>> frame, and then updates the destination address in the cloned skb's
>>>> data buffer before placing that skb on the provided queue.
>>>>
>>>> This logic is flawed since skb_clone() shares the same data buffer
>>>> with the original and the cloned skb, and hence each time the
>>>> destination address is updated, it overwrites the previous destination
>>>> address in this shared buffer. As a result, due to the special handing
>>>> of the first valid destination, all of the skbs will eventually be
>>>> sent to that first destination.
>>>
>>> Did you actually observe this happen in practice? ieee80211_change_da()
>>> does an skb_ensure_writable() check on the Ethernet header before
>>> writing it, so AFAICT it does not, in fact, overwrite the data of the
>>> original frame.
>>
>> I'm proxying this change for our internal team, and they have observed that
>> unicast frames are not being sent to the separate STAs.
>>
>> In response to your reply I went through the code again and it seems the
>> manner in which this functionality fails isn't as it was described to me.
>>
>> Instead this functionality fails because we'd fail on the first
>> ieee80211_change_da() call and hence goto multicast and where only the
>> original skb would be queued and transmitted as a multicast frame
>>
>> So the original logic is still faulty, only the actual faulty behavior is not
>> being described correctly: instead of sending multiple unicast frames to the
>> same STA we'd send a single multicast frame.
> 
> While I agree with switching over to skb_copy (or maybe pskb_copy to 
> reduce realloc on fragmented skbs), it's still not clear to me why 
> ieee80211_change_da fails. It should detect that the packet was cloned, 
> letting pskb_expand_head reallocate the head of the skb.
> 
> Please run some more tests to figure out the exact reason for the 
> failure. There might be another hidden bug lurking there, which would 
> get papered over by this change.
> 
> - Felix

Thanks, I've thrown this back over to the development team to clarify.

/jeff