Skip to contentSkip to conversation

nl80211: fix nlmsg allocation in cfg80211_ft_event

PATCH In review Dedy Lansky 3 participants 5 messages May 17, 2018 – May 22, 2018 802.11 (including cfg80211/NL80211)

lore
Share & tools
lore

Dedy Lansky

author
#0 lore raw

PATCH 1/1 nl80211: fix nlmsg allocation in cfg80211_ft_event

From: Dedy Lansky <[email protected]>

Allocation size of nlmsg in cfg80211_ft_event is based on ric_ies_len and doesn't take into account ies_len. This leads to NL80211_CMD_FT_EVENT message construction failure in case ft_event contains large enough ies buffer.
Add ies_len to the nlmsg allocation size.

Signed-off-by Dedy Lansky <[email protected]>
---
 net/wireless/nl80211.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/wireless/nl80211.c +2 -1
index afbe510..64afd04 100644
@@ -15755,7 +15755,8 @@ void cfg80211_ft_event(struct net_device *netdev,
 	if (!ft_event->target_ap)
 		return;
 
-	msg = nlmsg_new(100 + ft_event->ric_ies_len, GFP_KERNEL);
+	msg = nlmsg_new(100 + ft_event->ies_len + ft_event->ric_ies_len,
+			GFP_KERNEL);
 	if (!msg)
 		return;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 18 lines · Dedy Lansky

From: Dedy Lansky <[email protected]>

Allocation size of nlmsg in cfg80211_ft_event is based on ric_ies_len and doesn't take into account ies_len. This leads to NL80211_CMD_FT_EVENT message construction failure in case ft_event contains large enough ies buffer.
Add ies_len to the nlmsg allocation size.

Signed-off-by Dedy Lansky <[email protected]>
---
 net/wireless/nl80211.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/wireless/nl80211.c +0 -0
index afbe510..64afd04 100644

net/wireless/nl80211.c void cfg80211_ft_event(struct net_device · line 15762

Quoted 9 lines
net/wireless/nl80211.c +2 -1
@@ -15755,7 +15755,8 @@ void cfg80211_ft_event(struct net_device 
 *netdev,
 	if (!ft_event->target_ap)
 		return;
 
-	msg = nlmsg_new(100 + ft_event->ric_ies_len, GFP_KERNEL);
+	msg = nlmsg_new(100 + ft_event->ies_len + ft_event->ric_ies_len,
+			GFP_KERNEL);
 	if (!msg)
 		return;

should these really be nla_total_size(ft_event->ies_len) + nla_total_size(ft_event->ric_ies_len) to properly account for the NLA header + padding? or do we consider that to be noise captured by the "100"?

maintainer
#2 lore raw
Quoted 11 lines · Jeff Johnson

net/wireless/nl80211.c void cfg80211_ft_event(struct net_device *netdev, · line 15761

Quoted 6 lines
net/wireless/nl80211.c +2 -1
-	msg = nlmsg_new(100 + ft_event->ric_ies_len, GFP_KERNEL);
+	msg = nlmsg_new(100 + ft_event->ies_len + ft_event->ric_ies_len,
+			GFP_KERNEL);
 	if (!msg)
 		return;

should these really be nla_total_size(ft_event->ies_len) + nla_total_size(ft_event->ric_ies_len) to properly account for the NLA header + padding? or do we consider that to be noise captured by the "100"?

We do, technically we should have something like nla_total_size() of various things including all those wiphy, ifindex, MAC attributes etc. so we just get lazy...

johannes

author
#3 lore raw
Quoted 1 line

From: [email protected] =

[mailto:[email protected]] On Behalf Of Johannes Berg

Quoted 5 lines · Jeff Johnson
Quoted 3 lines

=20

Review

Quoted 2 lines
(quoted diff) +1 -1
-	msg =3D nlmsg_new(100 + ft_event->ric_ies_len, GFP_KERNEL);
+	msg =3D nlmsg_new(100 + ft_event->ies_len + =

ft_event->ric_ies_len,

Quoted 6 lines
Quoted 6 lines

net/wireless/nl80211.c void cfg80211_ft_event(struct net_device *netdev, · line 15761

Quoted 3 lines
net/wireless/nl80211.c +1 -0
+			GFP_KERNEL);
 	if (!msg)
 		return;

=20
should these really be nla_total_size(ft_event->ies_len) + nla_total_size(ft_event->ric_ies_len) to properly account for the =

NLA=20

Quoted 5 lines
Quoted 3 lines

header + padding? or do we consider that to be noise captured by the =

"100"?

We do, technically we should have something like nla_total_size() of =

various things including all those wiphy, ifindex, MAC attributes etc.

Quoted 1 line

so we just get lazy...

nla_total_size is currently not used in nl80211.c (actually not used in = net\wireless\ for that matters).
IMO, switching nl80211/cfg80211 to use nla_total_size should be done = separately.
This patch is for fixing a very specific and small bug. Using = nla_total_size in a single function in the file (cfg80211_ft_event) = would be awkward.

Thanks,
Dedy.

maintainer
#4 lore raw
Quoted 5 lines · Dedy Lansky
Quoted 2 lines

We do, technically we should have something like nla_total_size() of various things including all those wiphy, ifindex, MAC attributes etc. so we just get lazy...

nla_total_size is currently not used in nl80211.c (actually not used in net\wireless\ for that matters).

Like I said, we're lazy ;-)

Quoted 2 lines

IMO, switching nl80211/cfg80211 to use nla_total_size should be done separately.

I don't see much value in doing it at all, TBH.

johannes