[PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[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));
-- 
2.20.1

Re: [PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[Oleksij Rempel]

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

Re: [PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

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

Re: [PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[Oleksij Rempel]

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

Re: [PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[Toke Høiland-Jørgensen]

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>

Re: [PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[Dan Carpenter]

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

Re: [PATCH] ath9k_htc: fix potential out of bounds access with invalid rxstatus->rs_keyix

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220409061225.GA5447@kili/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches