From: Johannes Berg johannes.berg@intel.com
Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.
We currently don't validate the beacon head, i.e. the header, fixed part and elements that are to go in front of the TIM element. This means that the variable elements there can be malformed, e.g. have a length exceeding the buffer size, but most downstream code from this assumes that this has already been checked.
Add the necessary checks to the netlink policy.
Cc: stable@vger.kernel.org Fixes: ed1b6cc7f80f ("cfg80211/nl80211: add beacon settings") Link: https://lore.kernel.org/r/1569009255-I7ac7fbe9436e9d8733439eab8acbbd35e55c74... Signed-off-by: Johannes Berg johannes.berg@intel.com --- net/wireless/nl80211.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 6168db3c35e4..4a10ab388e0b 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -200,6 +200,38 @@ cfg80211_get_dev_from_info(struct net *netns, struct genl_info *info) return __cfg80211_rdev_from_attrs(netns, info->attrs); }
+static int validate_beacon_head(const struct nlattr *attr, + struct netlink_ext_ack *extack) +{ + const u8 *data = nla_data(attr); + unsigned int len = nla_len(attr); + const struct element *elem; + const struct ieee80211_mgmt *mgmt = (void *)data; + unsigned int fixedlen = offsetof(struct ieee80211_mgmt, + u.beacon.variable); + + if (len < fixedlen) + goto err; + + if (ieee80211_hdrlen(mgmt->frame_control) != + offsetof(struct ieee80211_mgmt, u.beacon)) + goto err; + + data += fixedlen; + len -= fixedlen; + + for_each_element(elem, data, len) { + /* nothing */ + } + + if (for_each_element_completed(elem, data, len)) + return 0; + +err: + NL_SET_ERR_MSG_ATTR(extack, attr, "malformed beacon head"); + return -EINVAL; +} + /* policy for the attributes */ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { [NL80211_ATTR_WIPHY] = { .type = NLA_U32 }, @@ -4014,6 +4046,12 @@ static int nl80211_parse_beacon(struct nlattr *attrs[], memset(bcn, 0, sizeof(*bcn));
if (attrs[NL80211_ATTR_BEACON_HEAD]) { + int ret = validate_beacon_head(attrs[NL80211_ATTR_BEACON_HEAD], + NULL); + + if (ret) + return ret; + bcn->head = nla_data(attrs[NL80211_ATTR_BEACON_HEAD]); bcn->head_len = nla_len(attrs[NL80211_ATTR_BEACON_HEAD]); if (!bcn->head_len)
On Wed, Oct 09, 2019 at 08:41:09AM +0200, Johannes Berg wrote:
From: Johannes Berg johannes.berg@intel.com
Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.
I don't see that commit in Linus's tree :(
Is this f88eb7c0d002 ("nl80211: validate beacon head")?
thanks,
greg k-h
On Wed, 2019-10-09 at 11:27 +0200, Greg KH wrote:
On Wed, Oct 09, 2019 at 08:41:09AM +0200, Johannes Berg wrote:
From: Johannes Berg johannes.berg@intel.com
Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.
I don't see that commit in Linus's tree :(
Hmmm. Not sure what happened there. I had created this in reply to Sasha's bot, perhaps it told me a different commit ID or something.
Or maybe ... yes I think the bot had actually applied a *patch* rather than picked one up, and I didn't really know what to do, but forgot about this then.
Is this f88eb7c0d002 ("nl80211: validate beacon head")?
Yes.
johannes
On Wed, Oct 09, 2019 at 08:41:09AM +0200, Johannes Berg wrote:
From: Johannes Berg johannes.berg@intel.com
Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.
We currently don't validate the beacon head, i.e. the header, fixed part and elements that are to go in front of the TIM element. This means that the variable elements there can be malformed, e.g. have a length exceeding the buffer size, but most downstream code from this assumes that this has already been checked.
Add the necessary checks to the netlink policy.
Cc: stable@vger.kernel.org Fixes: ed1b6cc7f80f ("cfg80211/nl80211: add beacon settings") Link: https://lore.kernel.org/r/1569009255-I7ac7fbe9436e9d8733439eab8acbbd35e55c74... Signed-off-by: Johannes Berg johannes.berg@intel.com
net/wireless/nl80211.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 6168db3c35e4..4a10ab388e0b 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -200,6 +200,38 @@ cfg80211_get_dev_from_info(struct net *netns, struct genl_info *info) return __cfg80211_rdev_from_attrs(netns, info->attrs); } +static int validate_beacon_head(const struct nlattr *attr,
struct netlink_ext_ack *extack)
+{
- const u8 *data = nla_data(attr);
- unsigned int len = nla_len(attr);
- const struct element *elem;
- const struct ieee80211_mgmt *mgmt = (void *)data;
- unsigned int fixedlen = offsetof(struct ieee80211_mgmt,
u.beacon.variable);
- if (len < fixedlen)
goto err;
- if (ieee80211_hdrlen(mgmt->frame_control) !=
offsetof(struct ieee80211_mgmt, u.beacon))
goto err;
- data += fixedlen;
- len -= fixedlen;
- for_each_element(elem, data, len) {
/* nothing */
- }
for_each_element() is not in 4.4, 4.9, 4.14, or 4.19, so this breaks the build :(
I'll drop this from my queues for now.
thanks,
greg k-h
On Wed, 2019-10-09 at 11:43 +0200, Greg KH wrote:
- for_each_element(elem, data, len) {
/* nothing */
- }
for_each_element() is not in 4.4, 4.9, 4.14, or 4.19, so this breaks the build :(
Oh, right. I had previously also said:
You also need to cherry-pick 0f3b07f027f8 ("cfg80211: add and use strongly typed element iteration macros") 7388afe09143 ("cfg80211: Use const more consistently in for_each_element macros")
as dependencies - the latter doesn't apply cleanly as it has a change that doesn't apply, but that part of the change isn't necessary.
johannes
On Wed, Oct 09, 2019 at 11:44:03AM +0200, Johannes Berg wrote:
On Wed, 2019-10-09 at 11:43 +0200, Greg KH wrote:
- for_each_element(elem, data, len) {
/* nothing */
- }
for_each_element() is not in 4.4, 4.9, 4.14, or 4.19, so this breaks the build :(
Oh, right. I had previously also said:
You also need to cherry-pick 0f3b07f027f8 ("cfg80211: add and use strongly typed element iteration macros") 7388afe09143 ("cfg80211: Use const more consistently in for_each_element macros")
as dependencies - the latter doesn't apply cleanly as it has a change that doesn't apply, but that part of the change isn't necessary.
Ok, that worked, thanks. Well, almost worked, I had to do a bit more effort for 4.4.y, but all looks good now.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org