ieee80211_tx_status() on injected packets

ieee80211_tx_status() on injected packets

[Pavel Roskin]

Hello!

I've been testing mac80211 with kmemcheck.  By injecting specially
crafted packets, I could trigger a warning in ieee80211_tx_status() on
this line:

        frag = le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_FRAG;

It turns out hdr->seq_ctrl is beyond the end of the skb.  Adding printk
confirms it:

hdr=0xffff88012aa04868, &hdr->seq_ctrl=0xffff88012aa0487e,
skb->data=0xffff88012aa04868, skb->data + skb->len=0xffff88012aa0487c

The packets that produce the warning have the radiotap header length
increased by 10.

Here's the annotated dump of the packet:

/* Original radiotap header, but the length should be 0e, not 18 */
00 00 18 00 03 00 00 00 00 02 6c 09 a0 00
/* mac80211 treats this as part of the radiotap header */
08 03 00 00 01 0c cc 00 00 00
/* frame control */
00 11
/* duration */
6b 39
/* addr1 */
40 19 11 04 28 00
/* addr2 */
00 00 10 00 00 00
/* addr3 - incomplete */
00 00 00 00
/* sequence control - beyond the skb end */

I'm using rt73usb to inject.  ieee80211_tx_status() is scheduled by
ieee80211_tx_status_irqsafe(), which is called in rt2x00dev.c.

If we allow to inject malformed packets, we shouldn't assume them to be
valid 802.11 packets unless we can verify it.  And even then, maybe it's
better to bypass ieee80211_tx_status() for injected packets, as it can
influence statistics and rate control algorithms in unpredictable ways.

-- 
Regards,
Pavel Roskin

Re: ieee80211_tx_status() on injected packets

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

On Fri, 2009-07-10 at 18:32 -0400, Pavel Roskin wrote:

> /* Original radiotap header, but the length should be 0e, not 18 */
> 00 00 18 00 03 00 00 00 00 02 6c 09 a0 00

Heh.

> /* mac80211 treats this as part of the radiotap header */
> 08 03 00 00 01 0c cc 00 00 00
> /* frame control */
> 00 11
> /* duration */
> 6b 39
> /* addr1 */
> 40 19 11 04 28 00
> /* addr2 */
> 00 00 10 00 00 00
> /* addr3 - incomplete */
> 00 00 00 00
> /* sequence control - beyond the skb end */
> 
> I'm using rt73usb to inject.  ieee80211_tx_status() is scheduled by
> ieee80211_tx_status_irqsafe(), which is called in rt2x00dev.c.
> 
> If we allow to inject malformed packets, we shouldn't assume them to be
> valid 802.11 packets unless we can verify it.  And even then, maybe it's
> better to bypass ieee80211_tx_status() for injected packets, as it can
> influence statistics and rate control algorithms in unpredictable ways.

Yeah, we should verify the length. I don't think we can skip the
processing since these packets might actually be sent by hostapd which
wants the processing -- if you fuck up your connection by injecting
random junk that seems to be your own fault, but I agree we should fix
the bug.

It would probably be useful to take the code in ieee80211_tx_status up
to (but not including) the skb_orphan() call, stick it into a separate
function and call it only when at least the frame the header is valid
(i.e. skb->len >= hdrlen).

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]