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

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

[Nickolai Zeldovich]

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

Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---
 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

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

[Lennert Buytenhek]

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 <nickolai@csail.mit.edu>
> ---
>  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

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

[Nickolai Zeldovich]

On Sun, Jan 6, 2013 at 9:48 PM, Lennert Buytenhek
<buytenh@wantstofly.org> 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.

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

[Lennert Buytenhek]

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

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

[Julian Calaby]

Hi Lennert,

On Mon, Jan 7, 2013 at 1:48 PM, Lennert Buytenhek
<buytenh@wantstofly.org> 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: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/