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 Maybe add a comment above, like: /* Values larger than these can crash the firmware */ > /* 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
