* [RFC] wifi: mwifiex: Asking for some light on this, please :)
@ 2023-08-16 0:52 Gustavo A. R. Silva
2023-08-16 1:06 ` Gustavo A. R. Silva
2023-08-22 17:00 ` Dan Williams
0 siblings, 2 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-08-16 0:52 UTC (permalink / raw)
To: Brian Norris, Kalle Valo
Cc: linux-wireless, linux-kernel, Gustavo A. R. Silva,
linux-hardening
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);
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);
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);
tlv_rxba = (struct mwifiex_ie_types_rxba_sync *)tmp;
}
}
Thanks in advance for any feedback!
--
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] wifi: mwifiex: Asking for some light on this, please :)
2023-08-16 0:52 [RFC] wifi: mwifiex: Asking for some light on this, please :) Gustavo A. R. Silva
@ 2023-08-16 1:06 ` Gustavo A. R. Silva
2023-08-22 17:00 ` Dan Williams
1 sibling, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-08-16 1:06 UTC (permalink / raw)
To: Brian Norris, Kalle Valo
Cc: linux-wireless, linux-kernel, Gustavo A. R. Silva,
linux-hardening
On 8/15/23 18:52, Gustavo A. R. Silva wrote:
> + tmp = (u8 *)tlv_rxba + tlv_bitmap_len + sizeof(*tlv_rxba - 1);
I meant:
tmp = (u8 *)tlv_rxba + tlv_bitmap_len + sizeof(*tlv_rxba) - 1;
--
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] wifi: mwifiex: Asking for some light on this, please :)
2023-08-16 0:52 [RFC] wifi: mwifiex: Asking for some light on this, please :) Gustavo A. R. Silva
2023-08-16 1:06 ` Gustavo A. R. Silva
@ 2023-08-22 17:00 ` Dan Williams
2023-08-22 19:23 ` Gustavo A. R. Silva
1 sibling, 1 reply; 6+ messages in thread
From: Dan Williams @ 2023-08-22 17:00 UTC (permalink / raw)
To: Gustavo A. R. Silva, Brian Norris, Kalle Valo
Cc: linux-wireless, linux-kernel, Gustavo A. R. Silva,
linux-hardening
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] wifi: mwifiex: Asking for some light on this, please :)
2023-08-22 17:00 ` Dan Williams
@ 2023-08-22 19:23 ` Gustavo A. R. Silva
2023-08-23 14:31 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-08-22 19:23 UTC (permalink / raw)
To: Dan Williams, Brian Norris, Kalle Valo
Cc: linux-wireless, linux-kernel, Gustavo A. R. Silva,
linux-hardening
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
>>
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] wifi: mwifiex: Asking for some light on this, please :)
2023-08-22 19:23 ` Gustavo A. R. Silva
@ 2023-08-23 14:31 ` Dan Williams
2023-08-25 3:16 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2023-08-23 14:31 UTC (permalink / raw)
To: Gustavo A. R. Silva, Brian Norris, Kalle Valo
Cc: linux-wireless, linux-kernel, Gustavo A. R. Silva,
linux-hardening
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
> > >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] wifi: mwifiex: Asking for some light on this, please :)
2023-08-23 14:31 ` Dan Williams
@ 2023-08-25 3:16 ` Gustavo A. R. Silva
0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-08-25 3:16 UTC (permalink / raw)
To: Dan Williams, Brian Norris, Kalle Valo
Cc: linux-wireless, linux-kernel, Gustavo A. R. Silva,
linux-hardening
> 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;
> }
>
Thanks for all the feedback, Dan:
https://lore.kernel.org/linux-hardening/cover.1692931954.git.gustavoars@kernel.org/
--
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-25 3:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-16 0:52 [RFC] wifi: mwifiex: Asking for some light on this, please :) Gustavo A. R. Silva
2023-08-16 1:06 ` Gustavo A. R. Silva
2023-08-22 17:00 ` Dan Williams
2023-08-22 19:23 ` Gustavo A. R. Silva
2023-08-23 14:31 ` Dan Williams
2023-08-25 3:16 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox