Re: [RFC] wifi: mwifiex: Asking for some light on this, please :)

[Dan Williams]

On Tue, 2023-08-15 at 18:52 -0600, Gustavo A. R. Silva wrote:
> Hi all,
> 
> While working on flex-array transformations I ran into the following
> implementation:
> 
> drivers/net/wireless/marvell/mwifiex/fw.h:775:
> struct mwifiex_ie_types_rxba_sync {
>         struct mwifiex_ie_types_header header;
>         u8 mac[ETH_ALEN];
>         u8 tid;
>         u8 reserved;
>         __le16 seq_num;
>         __le16 bitmap_len;
>         u8 bitmap[1];
> } __packed;
> 
> `bitmap` is currently being used as a fake-flex array and we should
> transform it into a proper flexible-array member.
> 
> However, while doing that, I noticed something in the following function
> that's not clear to me and I wanted to ask you for feedback:
> 
> drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:907:
> void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
>                                  u8 *event_buf, u16 len)
> {
>         struct mwifiex_ie_types_rxba_sync *tlv_rxba = (void *)event_buf;
>         u16 tlv_type, tlv_len;
>         struct mwifiex_rx_reorder_tbl *rx_reor_tbl_ptr;
>         u8 i, j;
>         u16 seq_num, tlv_seq_num, tlv_bitmap_len;
>         int tlv_buf_left = len;
>         int ret;
>         u8 *tmp;
> 
>         mwifiex_dbg_dump(priv->adapter, EVT_D, "RXBA_SYNC event:",
>                          event_buf, len);
>         while (tlv_buf_left >= sizeof(*tlv_rxba)) {

>                 tlv_type = le16_to_cpu(tlv_rxba->header.type);
>                 tlv_len  = le16_to_cpu(tlv_rxba->header.len);

>                 if (tlv_type != TLV_TYPE_RXBA_SYNC) {
>                         mwifiex_dbg(priv->adapter, ERROR,
>                                     "Wrong TLV id=0x%x\n", tlv_type);
>                         return;
>                 }
> 
>                 tlv_seq_num = le16_to_cpu(tlv_rxba->seq_num);
>                 tlv_bitmap_len = le16_to_cpu(tlv_rxba->bitmap_len);

This seems superfluous since couldn't the bitmap_len be calculated from
the tlv_len and sizeof(*tlv_rxba)? But whatever, sure.

Seems like there should be some input validation here to ensure that
tlv_bitmap_len and tlv_len don't overrun event_buf's memory though, if
the firmware is hosed or malicious.

But that's not your problem since you're not touching this code.

>                 mwifiex_dbg(priv->adapter, INFO,
>                             "%pM tid=%d seq_num=%d bitmap_len=%d\n",
>                             tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num,
>                             tlv_bitmap_len);
> 
>                 rx_reor_tbl_ptr =
>                         mwifiex_11n_get_rx_reorder_tbl(priv, tlv_rxba->tid,
>                                                        tlv_rxba->mac);
>                 if (!rx_reor_tbl_ptr) {
>                         mwifiex_dbg(priv->adapter, ERROR,
>                                     "Can not find rx_reorder_tbl!");
>                         return;
>                 }
> 
>                 for (i = 0; i < tlv_bitmap_len; i++) {
>                         for (j = 0 ; j < 8; j++) {
>                                 if (tlv_rxba->bitmap[i] & (1 << j)) {
>                                         seq_num = (MAX_TID_VALUE - 1) &
>                                                 (tlv_seq_num + i * 8 + j);
> 
>                                         mwifiex_dbg(priv->adapter, ERROR,
>                                                     "drop packet,seq=%d\n",
>                                                     seq_num);
> 
>                                         ret = mwifiex_11n_rx_reorder_pkt
>                                         (priv, seq_num, tlv_rxba->tid,
>                                          tlv_rxba->mac, 0, NULL);
> 
>                                         if (ret)
>                                                 mwifiex_dbg(priv->adapter,
>                                                             ERROR,
>                                                             "Fail to drop packet");
>                                 }
>                         }
>                 }
> 
>                 tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_len);

Now we have to subtract the size of the whole TLV (including the header
and flexarray) from the remaining bytes in event_buf.

But this looks pretty sketchy. Marvell TLVs have a header (the TL of
the TLV) and header->len says how long the V is. Most Marvell kernel
driver code (mwifiex, libertas, etc) does something like this:

	pos += ssid_tlv->header + ssid_tlv->header.len;

but tlv_rxba is much more than just the header; I think this code is
going to *over* count how many bytes were just consumed.

I'm not the only one thinking it's sketchy:

https://www.spinics.net/lists/linux-wireless/msg174231.html

>                 tmp = (u8 *)tlv_rxba + tlv_len + sizeof(*tlv_rxba);
>                 
> What's the relation between tlv_len, sizeof(*tlv_rxba) and tlv_bitmap_len?
> 
> Isn't `sizeof(*tlv_rxba) + tlv_len` and `tlv_len + sizeof(*tlv_rxba)`
> double-counting some fields in `struct mwifiex_ie_types_rxba_sync`?
> 
> Shouldn't this be something like this, instead (before the flex-array
> transformation, of course):
> 
> -               tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_len);
> -               tmp = (u8 *)tlv_rxba + tlv_len + sizeof(*tlv_rxba);
> +               tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_bitmap_len - 1);
> +               tmp = (u8 *)tlv_rxba + tlv_bitmap_len + sizeof(*tlv_rxba - 1);

If my assertion about tlv->header.len is correct then we can do:

tlv_buf_left -= sizeof(tlv_rxba->header) + tlv_len;
tmp = (u8 *)tlv_rxba + sizeof(tlv_rxba->header) + tlv_len;

> 
> 
>                 tlv_rxba = (struct mwifiex_ie_types_rxba_sync *)tmp;

This is silly; instead of tmp we could do:

u16 bytes_used;

...

bytes_used = sizeof(tlv_rxba->header) + tlv_len;
tlv_buf_left -= bytes_used;
tlv_rxba += bytes_used;

(with appropriate casting).

Dan

>         }
> }
> 
> Thanks in advance for any feedback!
> 
> --
> Gustavo
>