lkml.org 
[lkml]   [2023]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
SubjectRe: [RFC] wifi: mwifiex: Asking for some light on this, please :)
From
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);
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. :)

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;
+ }
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`

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
>>
>

\
 
 \ /
  Last update: 2023-08-22 21:23    [W:0.076 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site