| Message ID | 1470138089-6864-1-git-send-email-masashi.honma@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
On 2016年08月02日 20:41, Masashi Honma wrote: > - FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, 16, > + FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, > + IEEE80211_HT_OP_MODE_PROTECTION | > + IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT | > + IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT, > mask, NL80211_MESHCONF_HT_OPMODE, > nl80211_check_u16); This patch could over write cfg->ht_opmode even though EINVAL. I will modify this. Masashi Honma. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> This patch could over write cfg->ht_opmode even though EINVAL. > I will modify this. > Don't think that actually matters since then it shouldn't be used, but the v3 patch looks good. I'm not sure we should bother to do cross-setting validation? Like, I mean, validating that non-GF and non-HT aren't set together, etc. Those are somewhat nonsense configurations, but we can't prevent them all. I'm actually half thinking that we could just remove all restrictions on this and allow any u16 value of this field, and rely on wpa_supplicant to do the right thing... Then we don't have to update this if we ever want to do something new either. What do you think? What does the validation actually help us with? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016年08月03日 15:52, Johannes Berg wrote: > I'm actually half thinking that we could just remove all restrictions > on this and allow any u16 value of this field, and rely on > wpa_supplicant to do the right thing... Then we don't have to update > this if we ever want to do something new either. > > What do you think? What does the validation actually help us with? I think checking the bits here is better than allowing all values. Because if we allow any values for ht_opmode, kernel developer needs to care about any bit combination working well. For example, kernel developer should test there is not any unexpected thing when non-GF and non-HT both flags are enabled. If we check invalid bit at the entrance, we don't need to care anymore about invalid combination. In any case we need to care about combination. Then, it is more easy to do it near the entrance. And I think checking only in wpa_supplicant is not good idea. Because other user application can access to the kernel API. If invalid flag combination causes kernel panic, it could be kernel vulnerability. Masashi Honma. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> And I think checking only in wpa_supplicant is not good idea. Because > other user application can access to the kernel API. If invalid flag > combination causes kernel panic, it could be kernel vulnerability. > I don't really see how that should confuse a driver into a panic, but fair enough. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 46417f9..b2af600 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -5471,9 +5471,49 @@ do { \ FILL_IN_MESH_PARAM_IF_SET(tb, cfg, rssi_threshold, -255, 0, mask, NL80211_MESHCONF_RSSI_THRESHOLD, nl80211_check_s32); - FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, 16, + FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, + IEEE80211_HT_OP_MODE_PROTECTION | + IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT | + IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT, mask, NL80211_MESHCONF_HT_OPMODE, nl80211_check_u16); + if (tb[NL80211_MESHCONF_HT_OPMODE]) { + /* + * Check HT operation mode based on IEEE 802.11 2012 8.4.2.59 + * HT Operation element. + */ + if (cfg->ht_opmode & (~(IEEE80211_HT_OP_MODE_PROTECTION | + IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT | + IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT))) + return -EINVAL; + + if ((cfg->ht_opmode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT) && + (cfg->ht_opmode & IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)) + return -EINVAL; + + switch (cfg->ht_opmode & IEEE80211_HT_OP_MODE_PROTECTION) { + case IEEE80211_HT_OP_MODE_PROTECTION_NONE: + if (cfg->ht_opmode & + IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT) + return -EINVAL; + break; + case IEEE80211_HT_OP_MODE_PROTECTION_NONMEMBER: + if (!(cfg->ht_opmode & + IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)) + return -EINVAL; + break; + case IEEE80211_HT_OP_MODE_PROTECTION_20MHZ: + if (cfg->ht_opmode & + IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT) + return -EINVAL; + break; + case IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED: + if (!(cfg->ht_opmode & + IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)) + return -EINVAL; + break; + } + } FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshHWMPactivePathToRootTimeout, 1, 65535, mask, NL80211_MESHCONF_HWMP_PATH_TO_ROOT_TIMEOUT,
Previously, NL80211_MESHCONF_HT_OPMODE rejected correct flag combination, ex) IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED | IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT. This was caused by simple comparison with value 16. This causes setting non-existent flag (like 0x08) and invalid flag combinations. So this commit implements some checks based on IEEE 802.11 2012 8.4.2.59 HT Operation element. Signed-off-by: Masashi Honma <masashi.honma@gmail.com> --- net/wireless/nl80211.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)