[PATCH 1/2] nl80211: Fix possible Spectre-v1 for NL80211_TXRATE_HT

[PATCH 1/2] nl80211: Fix possible Spectre-v1 for NL80211_TXRATE_HT

[Masashi Honma]

Use array_index_nospec() to sanitize ridx with respect to speculation.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/wireless/nl80211.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 0827cbd..3c469c1 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3748,6 +3748,7 @@ static bool ht_rateset_to_mask(struct ieee80211_supported_band *sband,
 			return false;
 
 		/* check availability */
+		ridx = array_index_nospec(ridx, IEEE80211_HT_MCS_MASK_LEN);
 		if (sband->ht_cap.mcs.rx_mask[ridx] & rbit)
 			mcs[ridx] |= rbit;
 		else
-- 
2.7.4

[PATCH 2/2] nl80211: Fix possible Spectre-v1 for CQM RSSI thresholds

[Masashi Honma]

Use array_index_nospec() to sanitize i with respect to speculation.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/wireless/nl80211.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3c469c1..4f47502 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10227,7 +10227,7 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	s32 last, low, high;
 	u32 hyst;
-	int i, n;
+	int i, n, low_index;
 	int err;
 
 	/* RSSI reporting disabled? */
@@ -10264,10 +10264,19 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
 		if (last < wdev->cqm_config->rssi_thresholds[i])
 			break;
 
-	low = i > 0 ?
-		(wdev->cqm_config->rssi_thresholds[i - 1] - hyst) : S32_MIN;
-	high = i < n ?
-		(wdev->cqm_config->rssi_thresholds[i] + hyst - 1) : S32_MAX;
+	low_index = i - 1;
+	if (low_index >= 0) {
+		low_index = array_index_nospec(low_index, n);
+		low = wdev->cqm_config->rssi_thresholds[low_index] - hyst;
+	} else {
+		low = S32_MIN;
+	}
+	if (i < n) {
+		i = array_index_nospec(i, n);
+		high = wdev->cqm_config->rssi_thresholds[i] + hyst - 1;
+	} else {
+		high = S32_MAX;
+	}
 
 	return rdev_set_cqm_rssi_range_config(rdev, dev, low, high);
 }
-- 
2.7.4

Re: [PATCH 2/2] nl80211: Fix possible Spectre-v1 for CQM RSSI thresholds

[Johannes Berg]

On Tue, 2018-09-25 at 11:15 +0900, Masashi Honma wrote:
> Use array_index_nospec() to sanitize i with respect to speculation.

I applied the first patch in the seies, but I don't understand why this
patch should be necessary.

The value of i isn't controlled by the user, so it shouldn't need to be
sanitized?

The context was *just* missing, added by me:

        for (i = 0; i < n; i++)
>  		if (last < wdev->cqm_config->rssi_thresholds[i])
>  			break;

This loop determines i, and the user doesn't even control "last", but
even if they did, the possible values of i could only end up being in
the range 0..n-1, so no problems?

johannes

Re: [PATCH 2/2] nl80211: Fix possible Spectre-v1 for CQM RSSI thresholds

[Masashi Honma]

On 2018/09/26 18:23, Johannes Berg wrote:> I applied the first patch in 
the seies, but I don't understand why this
> patch should be necessary.
> 
> The value of i isn't controlled by the user, so it shouldn't need to be
> sanitized?
> 
> The context was *just* missing, added by me:
> 
>          for (i = 0; i < n; i++)
>>   		if (last < wdev->cqm_config->rssi_thresholds[i])
>>   			break;
> 
> This loop determines i, and the user doesn't even control "last", but
> even if they did, the possible values of i could only end up being in
> the range 0..n-1, so no problems?

The variable i could be n after the loop when this condition is not 
satisfied for all rssi_thresholds[i].

 >>   		if (last < wdev->cqm_config->rssi_thresholds[i])
 >>   			break;

And user could control rssi_thresholds[i] by using 
NL80211_ATTR_CQM_RSSI_THOLD.

For example, I could set 4 rssi_thresholds -400, -300, -200, -100.
And then last is -34. I could get i = n = 4 after the loop.

Regards,
Masashi Honma.

Re: [PATCH 2/2] nl80211: Fix possible Spectre-v1 for CQM RSSI thresholds

[Johannes Berg]

On Thu, 2018-09-27 at 07:26 +0900, Masashi Honma wrote:
> On 2018/09/26 18:23, Johannes Berg wrote:> I applied the first patch in 
> the seies, but I don't understand why this
> > patch should be necessary.
> > 
> > The value of i isn't controlled by the user, so it shouldn't need to be
> > sanitized?
> > 
> > The context was *just* missing, added by me:
> > 
> >          for (i = 0; i < n; i++)
> > >   		if (last < wdev->cqm_config->rssi_thresholds[i])
> > >   			break;
> > 
> > This loop determines i, and the user doesn't even control "last", but
> > even if they did, the possible values of i could only end up being in
> > the range 0..n-1, so no problems?
> 
> The variable i could be n after the loop when this condition is not 
> satisfied for all rssi_thresholds[i].
> 
>  >>   		if (last < wdev->cqm_config->rssi_thresholds[i])
>  >>   			break;
> 
> And user could control rssi_thresholds[i] by using 
> NL80211_ATTR_CQM_RSSI_THOLD.
> 
> For example, I could set 4 rssi_thresholds -400, -300, -200, -100.
> And then last is -34. I could get i = n = 4 after the loop.

Yes, good point, thanks for the explanation.

I'll merge this then.

johannes