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 <[email protected]>
---
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));
--
2.20.1
Dan Carpenter <[email protected]> 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 <[email protected]>
Acked-by: Toke Høiland-Jørgensen <[email protected]>
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 <[email protected]>
> ---
> 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
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
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
Toke Høiland-Jørgensen <[email protected]> writes:
> Dan Carpenter <[email protected]> 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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Tue, Apr 12, 2022 at 04:26:58PM +0300, Kalle Valo wrote:
> Toke H?iland-J?rgensen <[email protected]> writes:
>
> > Dan Carpenter <[email protected]> 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 <[email protected]> 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 <[email protected]>
> Acked-by: Toke Høiland-Jørgensen <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
2dc509305cf9 ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix
--
https://patchwork.kernel.org/project/linux-wireless/patch/20220409061225.GA5447@kili/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches