2019-11-21 22:43:14

by Brian Norris

Subject: [PATCH] iw: scan: fix double-free in error paths

Hit when, for instance, I'm stupid enough to type an invalid scan
command:

# iw wlan0 scan -h
BUG at file position lib/msg.c:572:void nlmsg_free(struct nl_msg *)
iw: lib/msg.c:572: void nlmsg_free(struct nl_msg *): Assertion `0' failed.
Aborted (core dumped)

Fixes: 2f74c59cf11e ("iw: fix memory leaks inside handle_scan")
Cc: John Crispin <[email protected]>
Signed-off-by: Brian Norris <[email protected]>
---
scan.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/scan.c b/scan.c
index 1418da73a624..bfd39e4b1a1c 100644
--- a/scan.c
+++ b/scan.c
@@ -448,8 +448,6 @@ static int handle_scan(struct nl80211_state *state,
}
/* fall through - this is an error */
case DONE:
- nlmsg_free(ssids);
- nlmsg_free(freqs);
err = 1;
goto nla_put_failure;
case FREQ:
--
2.24.0.432.g9d3f5f5b63-goog

2019-11-21 23:25:27

by John Crispin

Subject: Re: [PATCH] iw: scan: fix double-free in error paths

On 21/11/2019 23:41, Brian Norris wrote:
> Hit when, for instance, I'm stupid enough to type an invalid scan
> command:
>
> # iw wlan0 scan -h
> BUG at file position lib/msg.c:572:void nlmsg_free(struct nl_msg *)
> iw: lib/msg.c:572: void nlmsg_free(struct nl_msg *): Assertion `0' failed.
> Aborted (core dumped)
>
> Fixes: 2f74c59cf11e ("iw: fix memory leaks inside handle_scan")
> Cc: John Crispin <[email protected]>
wasn't me, nobody saw do anything
try
367e7dd3 (Amit Khatri 2015-06-26 09:02:36 +0000 451)
nlmsg_free(ssids);
367e7dd3 (Amit Khatri 2015-06-26 09:02:36 +0000 452)
nlmsg_free(freqs);
???
> Signed-off-by: Brian Norris <[email protected]>
> ---
> scan.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/scan.c b/scan.c
> index 1418da73a624..bfd39e4b1a1c 100644
> --- a/scan.c
> +++ b/scan.c
> @@ -448,8 +448,6 @@ static int handle_scan(struct nl80211_state *state,
> }
> /* fall through - this is an error */
> case DONE:
> - nlmsg_free(ssids);
> - nlmsg_free(freqs);
> err = 1;
> goto nla_put_failure;
> case FREQ:
>

2019-11-21 23:32:03

by Brian Norris

Subject: Re: [PATCH] iw: scan: fix double-free in error paths

On Thu, Nov 21, 2019 at 3:24 PM John Crispin <[email protected]> wrote:
>
> On 21/11/2019 23:41, Brian Norris wrote:
> > Hit when, for instance, I'm stupid enough to type an invalid scan
> > command:
> >
> > # iw wlan0 scan -h
> > BUG at file position lib/msg.c:572:void nlmsg_free(struct nl_msg *)
> > iw: lib/msg.c:572: void nlmsg_free(struct nl_msg *): Assertion `0' failed.
> > Aborted (core dumped)
> >
> > Fixes: 2f74c59cf11e ("iw: fix memory leaks inside handle_scan")
> > Cc: John Crispin <[email protected]>
> wasn't me, nobody saw do anything
> try
> 367e7dd3 (Amit Khatri 2015-06-26 09:02:36 +0000 451)
> nlmsg_free(ssids);
> 367e7dd3 (Amit Khatri 2015-06-26 09:02:36 +0000 452)
> nlmsg_free(freqs);
> ???

I don't really care about "who", but it's nice to correctly note "what":

Your patch added 'goto nla_put_failure' in the DONE case (or
fallthrough from NONE), which introduced the double-free. Previously,
it was just a 'return', which meant we needed to do the cleanup in
'case DONE'.

For Amit's patch: note how there's a 'return', which makes his code
the only possible call to nlmsg_free() (i.e., no double-free).

Brian

2019-11-21 23:33:12

by John Crispin

Subject: Re: [PATCH] iw: scan: fix double-free in error paths

On 22/11/2019 00:30, Brian Norris wrote:
> On Thu, Nov 21, 2019 at 3:24 PM John Crispin <[email protected]> wrote:
>>
>> On 21/11/2019 23:41, Brian Norris wrote:
>>> Hit when, for instance, I'm stupid enough to type an invalid scan
>>> command:
>>>
>>> # iw wlan0 scan -h
>>> BUG at file position lib/msg.c:572:void nlmsg_free(struct nl_msg *)
>>> iw: lib/msg.c:572: void nlmsg_free(struct nl_msg *): Assertion `0' failed.
>>> Aborted (core dumped)
>>>
>>> Fixes: 2f74c59cf11e ("iw: fix memory leaks inside handle_scan")
>>> Cc: John Crispin <[email protected]>
>> wasn't me, nobody saw do anything
>> try
>> 367e7dd3 (Amit Khatri 2015-06-26 09:02:36 +0000 451)
>> nlmsg_free(ssids);
>> 367e7dd3 (Amit Khatri 2015-06-26 09:02:36 +0000 452)
>> nlmsg_free(freqs);
>> ???
>
> I don't really care about "who", but it's nice to correctly note "what":
>
> Your patch added 'goto nla_put_failure' in the DONE case (or
> fallthrough from NONE), which introduced the double-free. Previously,
> it was just a 'return', which meant we needed to do the cleanup in
> 'case DONE'.
>
> For Amit's patch: note how there's a 'return', which makes his code
> the only possible call to nlmsg_free() (i.e., no double-free).
>
> Brian
>

point taken, I see it now :(
John