Re: [PATCH] mac80211: fix overwriting of qos_ctrl.tid field

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2019-11-22 at 16:06 +0100, Fredrik Olofsson wrote:

> > I'm not sure I understand.
> > 
> > If the QoS field is overwritten (where, btw?) then wouldn't that still
> > be done even after this change, and if the frame is pre-encrypted it is
> > corrupted?
> 
> Thanks for your response, and sorry for being too terse.
> 
> What we are doing is injecting pre-encrypted frames on the monitor interface.
> This used to work without issues (prior to the commit I mentioned). But now,
> the QoS field is overwritten, corrupting the frame. Actually the overwriting
> happened previously as well, but the QoS.TID field was not changed since it
> was always overwritten with the same value as it already had. This
> overwriting with the same value happens after my patch as well.
> 
> The simplified call flow for the frame is something like this:
> 
> netdev_core_pick_tx()
>   if (dev->real_num_tx_queues != 1) {
>     dev->ndo_select_queue()      <<< really: ieee80211_monitor_select_queue()
>   }
> 
> ...
> 
> ieee80211_monitor_start_xmit()
>   ieee80211_xmit()
>     ieee80211_set_qos_hdr()      <<< here the tid is overwritten
>     ieee80211_tx()
> 
> But after dbd50a851c50, real_num_tx_queues == 1 and
> ieee80211_monitor_select_queue() is never called. Then when the frame arrives
> in ieee80211_set_qos_hdr() the qos.tid field is overwritten with the wrong
> value. My patch makes sure ieee80211_set_qos_hdr() writes the same QoS.TID
> value as was originally in the frame. Even if ieee80211_monitor_select_queue()
> is not called.
> 
> My suggested patch tries to do the same as what happened before, but it may
> not be the correct way to do this. Maybe real_num_queues should not be 1?
> Maybe it would be better to detect the case that the frame is already
> encrypted, and make sure not to touch any protected fields at all?

No ... I get it now I think. Will need to think about it correctly
again.

What confused me is that you said the field was 'encrypted' but really
it's not, it's just part of what gets protected during the encryption. I
paid too much attention to your email vs. remembering how this all works
and then figured you couldn't write back the value to an encrypted
field. All good, I'll figure it out :)

johannes