Re: [PATCH] wifi: ath9k_htc: limit MTU

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

Chien Wong <m@xv97.com> 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@xv97.com>

(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