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