On Fri, Nov 22, 2019 at 1:00 PM Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
>
> On Tue, 2019-11-19 at 14:34 +0100, Fredrik Olofsson wrote:
> > Fixes overwriting of qos_ctrl.tid field for encrypted frames injected on
> > monitor interface. qos_ctrl.tid is protected by the encryption, and
> > cannot be modified after encryption. For injected frames, the encryption
> > key may not be available.
> >
> > Before passing the frame to the driver, the qos_ctrl.tid field is
> > updated from skb->priority. Prior to dbd50a851c50 skb->priority was
> > updated in ieee80211_select_queue_80211(), but this function is no longer
> > always called. This patch tries to mimmic the previous behaviour by
> > updating skb->priority in ieee80211_monitor_start_xmit().
>
> 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?
BR
/Fredrik
