| Message ID | 1537841701-3092-2-git-send-email-masashi.honma@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | None | expand |
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
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.
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
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); }
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(-)