nl80211: validate beacon head

PATCH In review Johannes Berg 2 participants 6 messages Oct 9, 2019 Stable branch

lore

Share & tools

lore

Johannes Berg

author

2019-10-09 06:41 UTC #0 lore raw

PATCH 1/1 nl80211: validate beacon head

From: Johannes Berg <[email protected]>

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 [email protected]

Fixes ed1b6cc7f80f ("cfg80211/nl80211: add beacon settings")

Link https://lore.kernel.org/r/1569009255-I7ac7fbe9436e9d8733439eab8acbbd35e55c74ef@changeid open here

Signed-off-by Johannes Berg <[email protected]>

---
 net/wireless/nl80211.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

net/wireless/nl80211.c +38 -0

index 6168db3c35e4..4a10ab388e0b 100644
@@ -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)

Signature · 1 line

2.20.1

Greg KH

2019-10-09 09:27 UTC #1 lore raw

Quoted 3 lines · Johannes Berg

From: Johannes Berg <[email protected]>
Commit 8a3347aa110c76a7f87771999aed491d1d8779a8 upstream.

I don't see that commit in Linus's tree :(

Is this f88eb7c0d002 ("nl80211: validate beacon head")?

thanks,

greg k-h

Johannes Berg

author

2019-10-09 09:29 UTC #2 lore raw

Quoted 6 lines · Greg KH

Quoted 3 lines · Johannes Berg
From: Johannes Berg <[email protected]>
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.

Quoted 1 line

Is this f88eb7c0d002 ("nl80211: validate beacon head")?

Yes.

johannes

Greg KH

2019-10-09 09:43 UTC #3 lore raw

In reply to Johannes Berg

Quoted 42 lines · Johannes Berg

From: Johannes Berg <[email protected]>
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 [email protected]
Fixes ed1b6cc7f80f ("cfg80211/nl80211: add beacon settings")
Link https://lore.kernel.org/r/1569009255-I7ac7fbe9436e9d8733439eab8acbbd35e55c74ef@changeid open here
Signed-off-by Johannes Berg <[email protected]>
---
 net/wireless/nl80211.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
net/wireless/nl80211.c +13 -0
index 6168db3c35e4..4a10ab388e0b 100644
@@ -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;
+

net/wireless/nl80211.c cfg80211_get_dev_from_info(struct net *netns, struct genl_info *info) · line 225

Quoted 10 lines

net/wireless/nl80211.c +10 -0

+	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

Johannes Berg

author

2019-10-09 09:44 UTC #4 lore raw

Quoted 7 lines · Greg KH

net/wireless/nl80211.c cfg80211_get_dev_from_info(struct net *netns, struct genl_info *info) · line 225
Quoted 4 lines
net/wireless/nl80211.c +3 -0
+	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

Greg KH

2019-10-09 13:37 UTC #5 lore raw

Quoted 9 lines · Johannes Berg

Quoted 7 lines · Greg KH
net/wireless/nl80211.c cfg80211_get_dev_from_info(struct net *netns, struct genl_info *info) · line 225
Quoted 4 lines
net/wireless/nl80211.c +3 -0
+	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 :(

Quoted 8 lines

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