From: Johannes Berg <[email protected]>
Ilan reported that sometimes nl80211 messages weren't working if
the frames being transported got very large, which was really a
problem for userspace-to-kernel messages, but prompted me to look
at the code.
Upon review, I found various places where variable-length data is
transported in an nl80211 message but the message isn't allocated
taking that into account. This shouldn't cause any problems since
the frames aren't really that long, apart in one place where two
(possibly very long frames) might not fit.
Fix all the places (that I found) that get variable length data
from the driver and put it into a message to take the length of
the variable data into account. The 100 there is just a safe
constant for the remaining message overhead (it's usually around
50 for most messages.)
Signed-off-by: Johannes Berg <[email protected]>
---
net/wireless/nl80211.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 23692658fe98..f55b251e4b0d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13249,7 +13249,7 @@ void nl80211_send_disconnected(struct cfg80211_registered_device *rdev,
struct sk_buff *msg;
void *hdr;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
if (!msg)
return;
@@ -13325,7 +13325,7 @@ void cfg80211_notify_new_peer_candidate(struct net_device *dev, const u8 *addr,
trace_cfg80211_notify_new_peer_candidate(dev, addr);
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ msg = nlmsg_new(100 + ie_len, gfp);
if (!msg)
return;
@@ -13362,7 +13362,7 @@ void nl80211_michael_mic_failure(struct cfg80211_registered_device *rdev,
struct sk_buff *msg;
void *hdr;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
if (!msg)
return;
@@ -13456,7 +13456,7 @@ static void nl80211_send_remain_on_chan_event(
struct sk_buff *msg;
void *hdr;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ msg = nlmsg_new(100 + len, gfp);
if (!msg)
return;
@@ -13696,7 +13696,7 @@ int nl80211_send_mgmt(struct cfg80211_registered_device *rdev,
struct sk_buff *msg;
void *hdr;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ msg = nlmsg_new(100 + len, gfp);
if (!msg)
return -ENOMEM;
@@ -13740,7 +13740,7 @@ void cfg80211_mgmt_tx_status(struct wireless_dev *wdev, u64 cookie,
trace_cfg80211_mgmt_tx_status(wdev, cookie, ack);
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ msg = nlmsg_new(100 + len, gfp);
if (!msg)
return;
@@ -14046,7 +14046,7 @@ static void nl80211_ch_switch_notify(struct cfg80211_registered_device *rdev,
struct sk_buff *msg;
void *hdr;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
if (!msg)
return;
@@ -14551,7 +14551,7 @@ void cfg80211_ft_event(struct net_device *netdev,
if (!ft_event->target_ap)
return;
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ msg = nlmsg_new(100 + ft_event->ric_ies_len, GFP_KERNEL);
if (!msg)
return;
--
2.9.3
On 9-1-2017 11:10, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> Ilan reported that sometimes nl80211 messages weren't working if
> the frames being transported got very large, which was really a
> problem for userspace-to-kernel messages, but prompted me to look
> at the code.
>
> Upon review, I found various places where variable-length data is
> transported in an nl80211 message but the message isn't allocated
> taking that into account. This shouldn't cause any problems since
> the frames aren't really that long, apart in one place where two
> (possibly very long frames) might not fit.
>
> Fix all the places (that I found) that get variable length data
> from the driver and put it into a message to take the length of
> the variable data into account. The 100 there is just a safe
> constant for the remaining message overhead (it's usually around
> 50 for most messages.)
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/wireless/nl80211.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 23692658fe98..f55b251e4b0d 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -13249,7 +13249,7 @@ void nl80211_send_disconnected(struct cfg80211_registered_device *rdev,
> struct sk_buff *msg;
> void *hdr;
>
> - msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
Don't you want the '100' to be a define?
Regards,
Arend
On 01/09/2017 05:28 PM, IgorMitsyanko wrote:
> On 01/09/2017 03:39 PM, Johannes Berg wrote:
>>>> - msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>> + msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
>>>
>>> Don't you want the '100' to be a define?
>>
>> I didn't want to glorify it too much - some places may need more or
>> less over time. There's no significance to this number.
>>
>> johannes
>>
>
> Looking at what NLMSG_DEFAULT_SIZE is, wouldn't it be more descriptive
> to replace 100 with something like:
>
> #define NLMSG_OVERHEAD (SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
> NLMSG_HDRLEN)
>
Now I see, it's not for msg overhead, but for other fixed-width netlink
attributes like WIPHY, IFINDEX etc
Hi Johannes,
[auto build test ERROR on mac80211-next/master]
[also build test ERROR on v4.10-rc3 next-20170106]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Johannes-Berg/cfg80211-size-various-nl80211-messages-correctly/20170109-185808
base: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: x86_64-kexec (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
net/wireless/nl80211.c: In function 'nl80211_michael_mic_failure':
>> net/wireless/nl80211.c:13365:24: error: 'req_ie_len' undeclared (first use in this function)
msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
^~~~~~~~~~
net/wireless/nl80211.c:13365:24: note: each undeclared identifier is reported only once for each function it appears in
>> net/wireless/nl80211.c:13365:37: error: 'resp_ie_len' undeclared (first use in this function)
msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
^~~~~~~~~~~
net/wireless/nl80211.c: In function 'nl80211_send_remain_on_chan_event':
>> net/wireless/nl80211.c:13459:24: error: 'len' undeclared (first use in this function)
msg = nlmsg_new(100 + len, gfp);
^~~
net/wireless/nl80211.c: In function 'nl80211_ch_switch_notify':
net/wireless/nl80211.c:14049:24: error: 'req_ie_len' undeclared (first use in this function)
msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
^~~~~~~~~~
net/wireless/nl80211.c:14049:37: error: 'resp_ie_len' undeclared (first use in this function)
msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
^~~~~~~~~~~
vim +/req_ie_len +13365 net/wireless/nl80211.c
13359 enum nl80211_key_type key_type, int key_id,
13360 const u8 *tsc, gfp_t gfp)
13361 {
13362 struct sk_buff *msg;
13363 void *hdr;
13364
13365 msg = nlmsg_new(100 + req_ie_len + resp_ie_len, gfp);
13366 if (!msg)
13367 return;
13368
13369 hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_MICHAEL_MIC_FAILURE);
13370 if (!hdr) {
13371 nlmsg_free(msg);
13372 return;
13373 }
13374
13375 if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
13376 nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex) ||
13377 (addr && nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, addr)) ||
13378 nla_put_u32(msg, NL80211_ATTR_KEY_TYPE, key_type) ||
13379 (key_id != -1 &&
13380 nla_put_u8(msg, NL80211_ATTR_KEY_IDX, key_id)) ||
13381 (tsc && nla_put(msg, NL80211_ATTR_KEY_SEQ, 6, tsc)))
13382 goto nla_put_failure;
13383
13384 genlmsg_end(msg, hdr);
13385
13386 genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
13387 NL80211_MCGRP_MLME, gfp);
13388 return;
13389
13390 nla_put_failure:
13391 genlmsg_cancel(msg, hdr);
13392 nlmsg_free(msg);
13393 }
13394
13395 void nl80211_send_beacon_hint_event(struct wiphy *wiphy,
13396 struct ieee80211_channel *channel_before,
13397 struct ieee80211_channel *channel_after)
13398 {
13399 struct sk_buff *msg;
13400 void *hdr;
13401 struct nlattr *nl_freq;
13402
13403 msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
13404 if (!msg)
13405 return;
13406
13407 hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_REG_BEACON_HINT);
13408 if (!hdr) {
13409 nlmsg_free(msg);
13410 return;
13411 }
13412
13413 /*
13414 * Since we are applying the beacon hint to a wiphy we know its
13415 * wiphy_idx is valid
13416 */
13417 if (nla_put_u32(msg, NL80211_ATTR_WIPHY, get_wiphy_idx(wiphy)))
13418 goto nla_put_failure;
13419
13420 /* Before */
13421 nl_freq = nla_nest_start(msg, NL80211_ATTR_FREQ_BEFORE);
13422 if (!nl_freq)
13423 goto nla_put_failure;
13424 if (nl80211_msg_put_channel(msg, channel_before, false))
13425 goto nla_put_failure;
13426 nla_nest_end(msg, nl_freq);
13427
13428 /* After */
13429 nl_freq = nla_nest_start(msg, NL80211_ATTR_FREQ_AFTER);
13430 if (!nl_freq)
13431 goto nla_put_failure;
13432 if (nl80211_msg_put_channel(msg, channel_after, false))
13433 goto nla_put_failure;
13434 nla_nest_end(msg, nl_freq);
13435
13436 genlmsg_end(msg, hdr);
13437
13438 rcu_read_lock();
13439 genlmsg_multicast_allns(&nl80211_fam, msg, 0,
13440 NL80211_MCGRP_REGULATORY, GFP_ATOMIC);
13441 rcu_read_unlock();
13442
13443 return;
13444
13445 nla_put_failure:
13446 genlmsg_cancel(msg, hdr);
13447 nlmsg_free(msg);
13448 }
13449
13450 static void nl80211_send_remain_on_chan_event(
13451 int cmd, struct cfg80211_registered_device *rdev,
13452 struct wireless_dev *wdev, u64 cookie,
13453 struct ieee80211_channel *chan,
13454 unsigned int duration, gfp_t gfp)
13455 {
13456 struct sk_buff *msg;
13457 void *hdr;
13458
13459 msg = nlmsg_new(100 + len, gfp);
13460 if (!msg)
13461 return;
13462
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 01/09/2017 03:39 PM, Johannes Berg wrote:
>>> - msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> + msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
>>
>> Don't you want the '100' to be a define?
>
> I didn't want to glorify it too much - some places may need more or
> less over time. There's no significance to this number.
>
> johannes
>
Looking at what NLMSG_DEFAULT_SIZE is, wouldn't it be more descriptive
to replace 100 with something like:
#define NLMSG_OVERHEAD (SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
NLMSG_HDRLEN)
> > - msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > + msg = nlmsg_new(100 + ie_len, GFP_KERNEL);
>
> Don't you want the '100' to be a define?
I didn't want to glorify it too much - some places may need more or
less over time. There's no significance to this number.
johannes