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

[Dan Williams]

On Tue, 2023-08-22 at 13:23 -0600, Gustavo A. R. Silva wrote:
> Hi Dan,
> 
> Thanks a lot for the feedback!
> 
> Please, see my comments below.
> 
> On 8/22/23 11:00, Dan Williams wrote:
> > 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`?
> 
> OK. So, based on your feedback, it seems that my assumptions were correct.
> 
> So, first I'll send the following fix:
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> index 391793a16adc..9eade3aa2918 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> @@ -965,8 +965,8 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
>                          }
>                  }
> 
> -               tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_len);
> -               tmp = (u8 *)tlv_rxba + tlv_len + sizeof(*tlv_rxba);
> +               tlv_buf_left -= (sizeof(tlv_rxba->header) + tlv_len);
> +               tmp = (u8 *)tlv_rxba + tlv_len + sizeof(tlv_rxba->header);

Looks good, but just for style I'd switch the sizeof() and tlv_len to
match the new tlv_buf_left line just above.

>                  tlv_rxba = (struct mwifiex_ie_types_rxba_sync *)tmp;
>          }
>   }
> 
> Then, I'll do the flex-array transformation on top of the fix above:
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> index 9eade3aa2918..cb5a399cd56a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> @@ -918,7 +918,7 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
> 
>          mwifiex_dbg_dump(priv->adapter, EVT_D, "RXBA_SYNC event:",
>                           event_buf, len);
> -       while (tlv_buf_left >= sizeof(*tlv_rxba)) {
> +       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) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index f2168fac95ed..8e6db904e5b2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -779,7 +779,7 @@ struct mwifiex_ie_types_rxba_sync {
>          u8 reserved;
>          __le16 seq_num;
>          __le16 bitmap_len;
> -       u8 bitmap[1];
> +       u8 bitmap[];
>   } __packed;
> 
>   struct chan_band_param_set {
> 
> This happilly results in no binary output differences before/after changes. :)

Yeah, looks right to me.

> 
> Finally, to top it off, I can send this sanity check:
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> index cb5a399cd56a..237d0ee3573f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> @@ -929,6 +929,13 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
> 
>                  tlv_seq_num = le16_to_cpu(tlv_rxba->seq_num);
>                  tlv_bitmap_len = le16_to_cpu(tlv_rxba->bitmap_len);
> +               if (sizeof(*tlv_rxba) + tlv_bitmap_len > tlv_buf_left) {
> +                      mwifiex_dbg(priv->adapter, ERROR,
> +                                   "TLV size (%ld) overflows event_buf (%d)\n",
> +                                  sizeof(*tlv_rxba) + tlv_bitmap_len,
> +                                  tlv_buf_left);
> +                       return;
> +               }

Make the mwifiex_dbg() into a warning though. This is an error
condition and shouldn't be hidden.

>                  mwifiex_dbg(priv->adapter, INFO,
>                              "%pM tid=%d seq_num=%d bitmap_len=%d\n",
>                              tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num,
> 
> I wanted to used `sizeof(*tlv_rxba) + tlv_bitmap_len` here instead of
> `sizeof(tlv_rxba->header) + tlv_len` to avoid any issues in case there
> is any (buggy) discrepancy between `tlv_len` and `tlv_bitmap_len`.
> This is when for some (weird) reason
>         `tlv_len - (sizeof(*tlv_rxba) - sizeof(tlv_rxba->header)) != tlv_bitmap_len`

tlv_len absolutely should also be checked. But you don't need that
condition, just do the same thing right after tlv_len is retrieved from
the header:

if (sizeof(tlv_rxba->header) + tlv_len > tlv_buf_left) {
    <warn>
    return;
}

Dan

> 
> What do you think?
> 
> Thanks!
> --
> Gustavo
> 
> 
> > > 
> > > 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
> > > 
> > 
>