| Message ID | 1570603265-@changeid (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [4.4,4.9,4.14,4.19] nl80211: validate beacon head | expand |
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-I7ac7fbe9436e9d8733439eab8acbbd35e55c74ef@changeid > 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
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)