| Message ID | 20201201095639.63936-1-anant.thazhemadam@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mac80211: cfg: enforce sanity checks for key_index in ieee80211_del_key() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 33 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, 2020-12-01 at 15:26 +0530, Anant Thazhemadam wrote: > Currently, it is assumed that key_idx values that are passed to > ieee80211_del_key() are all valid indexes as is, and no sanity checks > are performed for it. > However, syzbot was able to trigger an array-index-out-of-bounds bug > by passing a key_idx value of 5, when the maximum permissible index > value is (NUM_DEFAULT_KEYS - 1). > Enforcing sanity checks helps in preventing this bug, or a similar > instance in the context of ieee80211_del_key() from occurring. I think we should do this more generally in cfg80211, like in nl80211_new_key() we do it via cfg80211_validate_key_settings(). I suppose we cannot use the same function, but still, would be good to address this generally in nl80211 for all drivers. johannes
On 01/12/20 3:30 pm, Johannes Berg wrote: > On Tue, 2020-12-01 at 15:26 +0530, Anant Thazhemadam wrote: >> Currently, it is assumed that key_idx values that are passed to >> ieee80211_del_key() are all valid indexes as is, and no sanity checks >> are performed for it. >> However, syzbot was able to trigger an array-index-out-of-bounds bug >> by passing a key_idx value of 5, when the maximum permissible index >> value is (NUM_DEFAULT_KEYS - 1). >> Enforcing sanity checks helps in preventing this bug, or a similar >> instance in the context of ieee80211_del_key() from occurring. > I think we should do this more generally in cfg80211, like in > nl80211_new_key() we do it via cfg80211_validate_key_settings(). > > I suppose we cannot use the same function, but still, would be good to > address this generally in nl80211 for all drivers. Hello, This gave me the idea of trying to use cfg80211_validate_key_settings() directly in ieee80211_del_key(). I did try that out, tested it, and this bug doesn't seem to be getting triggered anymore. If this is okay, then I can send in a v2 soon. :) If there is any reason that I'm missing as to why cfg80211_validate_key_settings() cannot be used in this context, please let me know. Thanks, Anant
On Tue, 2020-12-01 at 17:26 +0530, Anant Thazhemadam wrote: > On 01/12/20 3:30 pm, Johannes Berg wrote: > > On Tue, 2020-12-01 at 15:26 +0530, Anant Thazhemadam wrote: > > > Currently, it is assumed that key_idx values that are passed to > > > ieee80211_del_key() are all valid indexes as is, and no sanity checks > > > are performed for it. > > > However, syzbot was able to trigger an array-index-out-of-bounds bug > > > by passing a key_idx value of 5, when the maximum permissible index > > > value is (NUM_DEFAULT_KEYS - 1). > > > Enforcing sanity checks helps in preventing this bug, or a similar > > > instance in the context of ieee80211_del_key() from occurring. > > I think we should do this more generally in cfg80211, like in > > nl80211_new_key() we do it via cfg80211_validate_key_settings(). > > > > I suppose we cannot use the same function, but still, would be good to > > address this generally in nl80211 for all drivers. > > Hello, > > This gave me the idea of trying to use cfg80211_validate_key_settings() > directly in ieee80211_del_key(). I did try that out, tested it, and this bug > doesn't seem to be getting triggered anymore. > If this is okay, then I can send in a v2 soon. :) > > If there is any reason that I'm missing as to why cfg80211_validate_key_settings() > cannot be used in this context, please let me know. If it works then I guess that's OK. I thought we didn't have all the right information, e.g. whether a key is pairwise or not? johannes
On 01/12/20 5:36 pm, Johannes Berg wrote: > On Tue, 2020-12-01 at 17:26 +0530, Anant Thazhemadam wrote: >> On 01/12/20 3:30 pm, Johannes Berg wrote: >>> On Tue, 2020-12-01 at 15:26 +0530, Anant Thazhemadam wrote: >>>> Currently, it is assumed that key_idx values that are passed to >>>> ieee80211_del_key() are all valid indexes as is, and no sanity checks >>>> are performed for it. >>>> However, syzbot was able to trigger an array-index-out-of-bounds bug >>>> by passing a key_idx value of 5, when the maximum permissible index >>>> value is (NUM_DEFAULT_KEYS - 1). >>>> Enforcing sanity checks helps in preventing this bug, or a similar >>>> instance in the context of ieee80211_del_key() from occurring. >>> I think we should do this more generally in cfg80211, like in >>> nl80211_new_key() we do it via cfg80211_validate_key_settings(). >>> >>> I suppose we cannot use the same function, but still, would be good to >>> address this generally in nl80211 for all drivers. >> Hello, >> >> This gave me the idea of trying to use cfg80211_validate_key_settings() >> directly in ieee80211_del_key(). I did try that out, tested it, and this bug >> doesn't seem to be getting triggered anymore. >> If this is okay, then I can send in a v2 soon. :) >> >> If there is any reason that I'm missing as to why cfg80211_validate_key_settings() >> cannot be used in this context, please let me know. > If it works then I guess that's OK. I thought we didn't have all the > right information, e.g. whether a key is pairwise or not? > > johannes > Well, cfg80211_supported_cipher_suite(&rdev->wiphy, params->cipher) returned false, and thus it worked for the syzbot reproducer. Would it be a safer idea to enforce the conditions that I initially put (in ieee80211_del_key()) directly in cfg80211_validate_key_settings() itself - by updating max_key_index, and checking accordingly? Thanks, Anant
On Tue, 2020-12-01 at 18:15 +0530, Anant Thazhemadam wrote: > > cfg80211_supported_cipher_suite(&rdev->wiphy, params->cipher) returned > false, and thus it worked for the syzbot reproducer. > Would it be a safer idea to enforce the conditions that I initially put (in > ieee80211_del_key()) directly in cfg80211_validate_key_settings() itself - by > updating max_key_index, and checking accordingly? Yes, I think so. But similarly to cfg80211_validate_key_settings() it should look at the device capabilities (beacon protection, etc.) Thanks! johannes
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 7276e66ae435..d349e33134e6 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -516,12 +516,30 @@ static int ieee80211_del_key(struct wiphy *wiphy, struct net_device *dev, if (!sta) goto out_unlock; - if (pairwise) + if (pairwise) { + if (key_idx >= NUM_DEFAULT_KEYS) { + ret = -EINVAL; + goto out_unlock; + } key = key_mtx_dereference(local, sta->ptk[key_idx]); - else + } else { + if (key_idx >= (NUM_DEFAULT_KEYS + + NUM_DEFAULT_MGMT_KEYS + + NUM_DEFAULT_BEACON_KEYS)) { + ret = -EINVAL; + goto out_unlock; + } key = key_mtx_dereference(local, sta->gtk[key_idx]); - } else + } + } else { + if (key_idx >= (NUM_DEFAULT_KEYS + + NUM_DEFAULT_MGMT_KEYS + + NUM_DEFAULT_BEACON_KEYS)) { + ret = -EINVAL; + goto out_unlock; + } key = key_mtx_dereference(local, sdata->keys[key_idx]); + } if (!key) { ret = -ENOENT;