On 7/30/24 6:10 PM, Toke Høiland-Jørgensen wrote:
Chien Wong <m@xxxxxxxx> writes:
Currently, the length of USB messages sent from host to Wi-Fi dongle is
not checked. Without the check, we could crash the firmware.
The length limits are determined by _HIFusb_get_max_msg_len_patch()
in the firmware code, located in k2_HIF_usb_patch.c and HIF_usb_patch.c
of the open-ath9k-htc-firmware project. The limits are 512 and 1600
bytes for regout and Wi-Fi TX messages respectively.
I'm not sure if the firmware crash is due to buffer overflow if RXing
too long USB messages but the length limit is clear and verified.
Somebody knowing hardware internals could help.
We should try our best not to crash the firmware. Setting the MTU limit
will help prevent some too long packets from being generated. However,
doing this alone is not enough: monitor interfaces will ignore the
limit so other measure should also be taken.
It's not easy to choose an optimal limit. The numbers that come to mind
include (1)1500, (2)1600 - sizeof overhead of shortest possible frame
and so on. For (1), it's too limiting: the device supports longer
frames. For (2), it won't filter frames with longer overhead.
Why bother considering higher layer protocols? We could just consider
the driver layer overhead.
How to reproduce a crash:
1. Insert a supported Wi-Fi card
2. Associate to an AP
3. Increase MTU of interface: # ip link set wlan0 mtu 2000
4. Generate some big packets: $ ping <gateway> -s 1600
5. The firmware should crash. If not, repeat step 4.
Tested-on: TP-LINK TL-WN722N v1(AR9271)
Tested-on: TP-LINK TL-WN821N v3(AR7010+AR9287).
Signed-off-by: Chien Wong <m@xxxxxxxx>
(Sorry for not getting around to looking at this earlier)
---
drivers/net/wireless/ath/ath9k/hif_usb.h | 3 +++
drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h
index b3e66b0485a5..f8fd78309829 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.h
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
@@ -50,6 +50,9 @@ extern int htc_use_dev_fw;
#define ATH_USB_RX_STREAM_MODE_TAG 0x4e00
#define ATH_USB_TX_STREAM_MODE_TAG 0x697e
+#define MAX_USB_REG_OUT_PIPE_MSG_SIZE 512
+#define MAX_USB_WLAN_TX_PIPE_MSG_SIZE 1600
(Sorry for not getting around to looking at this earlier)
/* FIXME: Verify these numbers (with Windows) */
#define MAX_TX_URB_NUM 8
#define MAX_TX_BUF_NUM 256
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index 3633f9eb2c55..3fad9cd4b1e6 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -760,6 +760,9 @@ static void ath9k_set_hw_capab(struct ath9k_htc_priv *priv,
hw->extra_tx_headroom = sizeof(struct tx_frame_hdr) +
sizeof(struct htc_frame_hdr) + 4;
+ hw->max_mtu = MAX_USB_WLAN_TX_PIPE_MSG_SIZE - sizeof(struct tx_frame_hdr)
+ - sizeof(struct htc_frame_hdr);
Shouldn't this be the same as the extra_tx_headroom set above? Not sure
what the +4 is for in that assignment, but it seems a bit odd to not be
consistent. Did you verify that an MTU of 1580 works without crashing?
Maybe this should just be:
hw->max_mtu = MAX_USB_WLAN_TX_PIPE_MSG_SIZE - hw->extra_tx_headroom;
just to be sure?
-Toke
(Sorry for not getting around to looking at this earlier)
Never mind.
(Sorry for not getting around to looking at this earlier)
Sure.
Shouldn't this be the same as the extra_tx_headroom set above? Not sure
what the +4 is for in that assignment, but it seems a bit odd to not be
consistent. Did you verify that an MTU of 1580 works without crashing?
Maybe this should just be:
hw->max_mtu = MAX_USB_WLAN_TX_PIPE_MSG_SIZE - hw->extra_tx_headroom;
just to be sure?
The +4 is for the header at the very beginning of the USB packet:
>/* hif_usb_send_mgmt() in hif_usb.c */
> hdr = skb_push(skb, 4);
> *hdr++ = cpu_to_le16(skb->len - 4);
> *hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
I suppose that the four bytes are consumed by the USB hardware and they
do not occupy buffer in the firmware. And my experiment proved this.
Unfortunately, setting MTU=1580 alone could not prevent the firmware
from crashing. The MTU only limits upper layer length, not taking MAC
overhead into account. That's why we need to take other measures such as
dropping packets before sending via USB, as proposed by my earlier patch.
Regards
Chien Wong
