| Message ID | 20191121224139.58281-1-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | iw: scan: fix double-free in error paths | expand |
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 <john@phrozen.org> 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 <briannorris@chromium.org> > --- > 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: >
On Thu, Nov 21, 2019 at 3:24 PM John Crispin <john@phrozen.org> 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 <john@phrozen.org> > 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
On 22/11/2019 00:30, Brian Norris wrote: > On Thu, Nov 21, 2019 at 3:24 PM John Crispin <john@phrozen.org> 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 <john@phrozen.org> >> 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
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:
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 <john@phrozen.org> Signed-off-by: Brian Norris <briannorris@chromium.org> --- scan.c | 2 -- 1 file changed, 2 deletions(-)