Re: divide by zero in ath9k_htc_choose_bslot()

["Toke Høiland-Jørgensen" <toke@toke.dk>]

rtm@csail.mit.edu writes:

> The attached demo uses usbip to pretend to be a USB device for 
>
>   drivers/net/wireless/ath/ath9k/
>
> It sets up the wifi interface, and then maliciously generates a frame
> that claims to be on the USB endpoint that ath9k_htc_rx_msg() passes to
> ath9k_wmi_ctrl_rx(). The cmd_id in the frame is 0x1002, or
> WMI_SWBA_EVENTID, which causes ath9k_wmi_ctrl_rx() to wake up the
> ath9k_wmi_event_tasklet. Which calls ath9k_htc_swba(), which calls
> ath9k_htc_choose_bslot(), which says
>
>         intval = priv->cur_beacon_conf.beacon_interval;
>         tsf = be64_to_cpu(swba->tsf);
>         tsftu = TSF_TO_TU(tsf >> 32, tsf);
>         slot = ((tsftu % intval) * ATH9K_HTC_MAX_BCN_VIF) / intval;
>         slot = ATH9K_HTC_MAX_BCN_VIF - slot - 1;
>
> At this point beacon_interval is zero, so this divides by zero. On an
> amd64, a fault. On a RISC-V, which produces -1 for divide by zero,
> slot ends up as 2, which is too large; later on, slot=2 causes
> ath9k_htc_send_buffered() to index off the end of the bslot array;
> ieee80211_get_buffered_bc() then dereferences the resulting bad vif
> pointer.
>
>         vif = priv->beacon.bslot[slot];
>         skb = ieee80211_get_buffered_bc(priv->hw, vif);
>
> Changing ath9k_htc_choose_bslot() to return zero if intval is zero makes
> the crash go away. I don't know if that would be correct with a real
> Atheros device, but it probably doesn't matter since I imagine this
> would only ever arise with a broken or malicious USB device.

Thank you for the report, and the thorough analysis!

So I guess this happens because your malicious device sends the SWBA
command before beacons have actually been enabled (and
ath9k_htc_beacon_config() called to set the inverval)? In which case I
think a patch like the below makes more sense than relying on
ath9k_htc_choose_bslot() returning 0. WDYT?

-Toke

diff --git i/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c w/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
index 547634f82183..81fa7cbad892 100644
--- i/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
+++ w/drivers/net/wireless/ath/ath9k/htc_drv_beacon.c
@@ -290,6 +290,9 @@ void ath9k_htc_swba(struct ath9k_htc_priv *priv,
        struct ath_common *common = ath9k_hw_common(priv->ah);
        int slot;
 
+       if (!priv->cur_beacon_conf.enable_beacon)
+               return;
+
        if (swba->beacon_pending != 0) {
                priv->beacon.bmisscnt++;
                if (priv->beacon.bmisscnt > BSTUCK_THRESHOLD) {