2013-01-07 01:44:10

by Nickolai Zeldovich

Subject: [PATCH] drivers/net/wireless/mwl8k.c: avoid use-after-free

Do not dereference p->station_id after kfree(cmd) because p
points into the cmd data structure.

Signed-off-by: Nickolai Zeldovich <[email protected]>
---
drivers/net/wireless/mwl8k.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index f221b95..83564d3 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -4250,9 +4250,11 @@ static int mwl8k_cmd_update_stadb_add(struct ieee80211_hw *hw,
p->amsdu_enabled = 0;

rc = mwl8k_post_cmd(hw, &cmd->header);
+ if (!rc)
+ rc = p->station_id;
kfree(cmd);

- return rc ? rc : p->station_id;
+ return rc;
}

static int mwl8k_cmd_update_stadb_del(struct ieee80211_hw *hw,
--
1.7.10.4

2013-01-07 02:57:44

by Lennert Buytenhek

Subject: Re: [PATCH] drivers/net/wireless/mwl8k.c: avoid use-after-free

On Sun, Jan 06, 2013 at 08:27:22PM -0500, Nickolai Zeldovich wrote:

> Do not dereference p->station_id after kfree(cmd) because p
> points into the cmd data structure.

Good catch, but the patch would be better titled "mwl8k.c: avoid
having a working driver", as the station_id return code _is_ needed
by the caller in case of success.


> Signed-off-by: Nickolai Zeldovich <[email protected]>
> ---
> drivers/net/wireless/mwl8k.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
> index f221b95..83564d3 100644
> --- a/drivers/net/wireless/mwl8k.c
> +++ b/drivers/net/wireless/mwl8k.c
> @@ -4250,9 +4250,11 @@ static int mwl8k_cmd_update_stadb_add(struct ieee80211_hw *hw,
> p->amsdu_enabled = 0;
>
> rc = mwl8k_post_cmd(hw, &cmd->header);
> + if (!rc)
> + rc = p->station_id;
> kfree(cmd);
>
> - return rc ? rc : p->station_id;
> + return rc;
> }
>
> static int mwl8k_cmd_update_stadb_del(struct ieee80211_hw *hw,
> --
> 1.7.10.4

2013-01-07 03:03:32

by Julian Calaby

Subject: Re: [PATCH] drivers/net/wireless/mwl8k.c: avoid use-after-free

Hi Lennert,

On Mon, Jan 7, 2013 at 1:48 PM, Lennert Buytenhek
<[email protected]> wrote:
> On Sun, Jan 06, 2013 at 08:27:22PM -0500, Nickolai Zeldovich wrote:
>
>> Do not dereference p->station_id after kfree(cmd) because p
>> points into the cmd data structure.
>
> Good catch, but the patch would be better titled "mwl8k.c: avoid
> having a working driver", as the station_id return code _is_ needed
> by the caller in case of success.

Are you sure?

>> diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
>> index f221b95..83564d3 100644
>> --- a/drivers/net/wireless/mwl8k.c
>> +++ b/drivers/net/wireless/mwl8k.c
>> @@ -4250,9 +4250,11 @@ static int mwl8k_cmd_update_stadb_add(struct ieee80211_hw *hw,
>> p->amsdu_enabled = 0;
>>
>> rc = mwl8k_post_cmd(hw, &cmd->header);
>> + if (!rc)
>> + rc = p->station_id;
>> kfree(cmd);
>>
>> - return rc ? rc : p->station_id;

If I'm reading this right, the removed line is equivalent to:

if( rc )
return rc;
else
return p->station_id;

which is equivalent to:

if( !rc )
rc = p->station_id;

return rc;


Thanks,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

2013-01-07 03:02:35

by Nickolai Zeldovich

Subject: Re: [PATCH] drivers/net/wireless/mwl8k.c: avoid use-after-free

On Sun, Jan 6, 2013 at 9:48 PM, Lennert Buytenhek
<[email protected]> wrote:
> Good catch, but the patch would be better titled "mwl8k.c: avoid
> having a working driver", as the station_id return code _is_ needed
> by the caller in case of success.

I'm not quite sure what you mean -- is there something subtle going on
here? I believe my patch preserves the semantics of the original
code: it returns the value of p->station_id if mwl8k_post_cmd()
returned 0, but it just does so by reading p->station_id first before
calling kfree(cmd).

Nickolai.

2013-01-07 03:19:22

by Lennert Buytenhek

Subject: Re: [PATCH] drivers/net/wireless/mwl8k.c: avoid use-after-free

On Sun, Jan 06, 2013 at 10:02:14PM -0500, Nickolai Zeldovich wrote:

> > Good catch, but the patch would be better titled "mwl8k.c: avoid
> > having a working driver", as the station_id return code _is_ needed
> > by the caller in case of success.
>
> I'm not quite sure what you mean -- is there something subtle going on
> here? I believe my patch preserves the semantics of the original
> code: it returns the value of p->station_id if mwl8k_post_cmd()
> returned 0, but it just does so by reading p->station_id first before
> calling kfree(cmd).

Oops! You're right. Sorry about that.

/me goes to order some crow for dinner