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

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

[Fredrik Olofsson]

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().

Fixes: dbd50a851c50 ("mac80211: only allocate one queue when using iTXQs")
Signed-off-by: Fredrik Olofsson <fredrik.olofsson@anyfinetworks.com>
---
 net/mac80211/tx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1fa422782905..cbd273c0b275 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2263,6 +2263,15 @@ netdev_tx_t ieee80211_monitor_start_xmit(struct sk_buff *skb,
 						    payload[7]);
 	}
 
+	/*
+	 * Initialize skb->priority for QoS frames. This is put in the TID field
+	 * of the frame before passing it to the driver.
+	 */
+	if (ieee80211_is_data_qos(hdr->frame_control)) {
+		u8 *p = ieee80211_get_qos_ctl(hdr);
+		skb->priority = *p & IEEE80211_QOS_CTL_TAG1D_MASK;
+	}
+
 	memset(info, 0, sizeof(*info));
 
 	info->flags = IEEE80211_TX_CTL_REQ_TX_STATUS |
-- 
2.20.1

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

[Johannes Berg]

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?

johannes

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

[Fredrik Olofsson]

On Fri, Nov 22, 2019 at 1:00 PM Johannes Berg <johannes@sipsolutions.net> 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

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

[Johannes Berg]

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