2018-05-17 13:25:11

by Dedy Lansky

[permalink] [raw]
Subject: [PATCH] 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(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index afbe510..64afd04 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -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;

--
1.9.1


2018-05-22 09:00:19

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] nl80211: fix nlmsg allocation in cfg80211_ft_event

On Mon, 2018-05-21 at 10:23 +0300, Dedy Lansky wrote:
> > 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 ;-)

> 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

2018-05-17 19:29:08

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] nl80211: fix nlmsg allocation in cfg80211_ft_event

On Thu, 2018-05-17 at 11:43 -0700, Jeff Johnson wrote:
>
> > - 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

2018-05-21 07:23:12

by Dedy Lansky

[permalink] [raw]
Subject: RE: [PATCH] nl80211: fix nlmsg allocation in cfg80211_ft_event

> From: [email protected] =
[mailto:[email protected]] On Behalf Of Johannes Berg
>
> On Thu, 2018-05-17 at 11:43 -0700, Jeff Johnson wrote:
> >=20
> > > - 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,
> > > + 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
> > 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...

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.

2018-05-17 18:43:13

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH] nl80211: fix nlmsg allocation in cfg80211_ft_event

On 2018-05-17 06:25, Dedy Lansky wrote:
> 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(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index afbe510..64afd04 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -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"?