| Message ID | 20220409061225.GA5447@kili (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2dc509305cf956381532792cb8dceef2b1504765 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix | expand |
Hi Dan, thank you for your patch. Am 09.04.22 um 08:12 schrieb Dan Carpenter: > The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to > ensure that it is within than bitmap. > > drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept() > error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()' > > Fixes: 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c > index 6a850a0bfa8a..a23eaca0326d 100644 > --- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c > +++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c > @@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv, > goto rx_next; > } > > + if (rxstatus->rs_keyix >= ATH_KEYMAX && > + rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) { > + ath_dbg(common, ANY, > + "Invalid keyix, dropping (keyix: %d)\n", > + rxstatus->rs_keyix); > + goto rx_next; > + } > + > /* Get the RX status information */ > > memset(rx_status, 0, sizeof(struct ieee80211_rx_status)); Looks ok to me. By the way, rs_keyix seems to have a wrong type. It is declared as u8, but used as s8. -- Regards, Oleksij
Dan Carpenter <dan.carpenter@oracle.com> writes: > The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to > ensure that it is within than bitmap. s/than/the/ ? This I think Kalle can fix up when applying :) > drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept() > error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()' > > Fixes: 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
On Sat, Apr 09, 2022 at 09:53:53AM +0200, Oleksij Rempel wrote: > > diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c > > index 6a850a0bfa8a..a23eaca0326d 100644 > > --- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c > > +++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c > > @@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv, > > goto rx_next; > > } > > > > + if (rxstatus->rs_keyix >= ATH_KEYMAX && > > + rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) { > > + ath_dbg(common, ANY, > > + "Invalid keyix, dropping (keyix: %d)\n", > > + rxstatus->rs_keyix); > > + goto rx_next; > > + } > > + > > /* Get the RX status information */ > > > > memset(rx_status, 0, sizeof(struct ieee80211_rx_status)); > > Looks ok to me. Thanks! > > By the way, rs_keyix seems to have a wrong type. It is declared as u8, but used as s8. That sounds like something outside the scope of the patch... Why do you mean "used as s8"? Which function are you talking about? You made me panic briefly because ATH9K_RXKEYIX_INVALID is a u8 so it needs to be u8. I would have thought instinctively that u8 would be the right type for an index like this. regards, dan carpenter
Am 11.04.22 um 09:54 schrieb Dan Carpenter: > On Sat, Apr 09, 2022 at 09:53:53AM +0200, Oleksij Rempel wrote: >>> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c >>> index 6a850a0bfa8a..a23eaca0326d 100644 >>> --- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c >>> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c >>> @@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv, >>> goto rx_next; >>> } >>> >>> + if (rxstatus->rs_keyix >= ATH_KEYMAX && >>> + rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) { >>> + ath_dbg(common, ANY, >>> + "Invalid keyix, dropping (keyix: %d)\n", >>> + rxstatus->rs_keyix); >>> + goto rx_next; >>> + } >>> + >>> /* Get the RX status information */ >>> >>> memset(rx_status, 0, sizeof(struct ieee80211_rx_status)); >> >> Looks ok to me. > > Thanks! > >> >> By the way, rs_keyix seems to have a wrong type. It is declared as u8, but used as s8. > > That sounds like something outside the scope of the patch... ack :) > Why do you mean "used as s8"? Which function are you talking about? > > You made me panic briefly because ATH9K_RXKEYIX_INVALID is a u8 so it > needs to be u8. I would have thought instinctively that u8 would be the > right type for an index like this. Because ATH_KEYMAX == S8_MAX and ATH9K_RXKEYIX_INVALID is ((u8)-1) All bitmap values within drivers/net/wireless/ath should never have BIT(7) set, except it is -1. -- Regards, Oleksij
Toke Høiland-Jørgensen <toke@toke.dk> writes: > Dan Carpenter <dan.carpenter@oracle.com> writes: > >> The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to >> ensure that it is within than bitmap. > > s/than/the/ ? > > This I think Kalle can fix up when applying :) Yup, fixed now in the pending branch.
On Tue, Apr 12, 2022 at 04:26:58PM +0300, Kalle Valo wrote: > Toke Høiland-Jørgensen <toke@toke.dk> writes: > > > Dan Carpenter <dan.carpenter@oracle.com> writes: > > > >> The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to > >> ensure that it is within than bitmap. > > > > s/than/the/ ? > > > > This I think Kalle can fix up when applying :) > > Yup, fixed now in the pending branch. Thanks! regards, dan carpenter
Dan Carpenter <dan.carpenter@oracle.com> wrote: > The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to > ensure that it is within the bitmap. > > drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept() > error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()' > > Fixes: 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 2dc509305cf9 ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c index 6a850a0bfa8a..a23eaca0326d 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c @@ -1016,6 +1016,14 @@ static bool ath9k_rx_prepare(struct ath9k_htc_priv *priv, goto rx_next; } + if (rxstatus->rs_keyix >= ATH_KEYMAX && + rxstatus->rs_keyix != ATH9K_RXKEYIX_INVALID) { + ath_dbg(common, ANY, + "Invalid keyix, dropping (keyix: %d)\n", + rxstatus->rs_keyix); + goto rx_next; + } + /* Get the RX status information */ memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
The "rxstatus->rs_keyix" eventually gets passed to test_bit() so we need to ensure that it is within than bitmap. drivers/net/wireless/ath/ath9k/common.c:46 ath9k_cmn_rx_accept() error: passing untrusted data 'rx_stats->rs_keyix' to 'test_bit()' Fixes: 4ed1a8d4a257 ("ath9k_htc: use ath9k_cmn_rx_accept") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 8 ++++++++ 1 file changed, 8 insertions(+)