2016-06-06 14:57:10

by Eduardo Abinader

Subject: [PATCH] nl80211: avoid possible memleak on nl80211_set_reg

Setting NULL just after freeing regdomain.

Signed-off-by: Eduardo Abinader <[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 d120449..39d107d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5839,10 +5839,11 @@ static int nl80211_set_reg(struct sk_buff *skb,
struct genl_info *info)

r = set_regdom(rd, REGD_SOURCE_CRDA);
/* set_regdom took ownership */
- rd = NULL;

bad_reg:
kfree(rd);
+ rd = NULL;
+
return r;
}
#endif /* CONFIG_CFG80211_CRDA_SUPPORT */
--
2.5.0

2016-06-09 08:36:06

by Arend Van Spriel

Subject: Re: [PATCH] nl80211: avoid possible memleak on nl80211_set_reg

On 9-6-2016 9:58, Johannes Berg wrote:
> On Mon, 2016-06-06 at 16:56 +0200, Eduardo Abinader wrote:
>> Setting NULL just after freeing regdomain.
>>
>> Signed-off-by: Eduardo Abinader <[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 d120449..39d107d 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -5839,10 +5839,11 @@ static int nl80211_set_reg(struct sk_buff
>> *skb, struct genl_info *info)
>>
>> r = set_regdom(rd, REGD_SOURCE_CRDA);
>> /* set_regdom took ownership */
>> - rd = NULL;
>>
>> bad_reg:
>> kfree(rd);
>> + rd = NULL;
>
> To this I can only say: what?

The patch is bad, but the confusion starts with the original code
(ab)using kfree() behaviour by setting rd to NULL. Personally, I do not
like it, but prefer it over bugs ;-)

Regards,
Arend

2016-06-09 07:58:04

by Johannes Berg

Subject: Re: [PATCH] nl80211: avoid possible memleak on nl80211_set_reg

On Mon, 2016-06-06 at 16:56 +0200, Eduardo Abinader wrote:
> Setting NULL just after freeing regdomain.
>
> Signed-off-by: Eduardo Abinader <[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 d120449..39d107d 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -5839,10 +5839,11 @@ static int nl80211_set_reg(struct sk_buff
> *skb, struct genl_info *info)
>  
>   r = set_regdom(rd, REGD_SOURCE_CRDA);
>   /* set_regdom took ownership */
> - rd = NULL;
>  
>   bad_reg:
>   kfree(rd);
> + rd = NULL;

To this I can only say: what?

johannes

2016-06-09 08:39:59

by Johannes Berg

Subject: Re: [PATCH] nl80211: avoid possible memleak on nl80211_set_reg

> > > +++ b/net/wireless/nl80211.c
> > > @@ -5839,10 +5839,11 @@ static int nl80211_set_reg(struct sk_buff
> > > *skb, struct genl_info *info)
> > >  
> > >   r = set_regdom(rd, REGD_SOURCE_CRDA);
> > >   /* set_regdom took ownership */
> > > - rd = NULL;
> > >  
> > >   bad_reg:
> > >   kfree(rd);
> > > + rd = NULL;
> > To this I can only say: what?
> The patch is bad, but the confusion starts with the original code
> (ab)using kfree() behaviour by setting rd to NULL. Personally, I do
> not like it, but prefer it over bugs ;-)
>

Yeah, fair enough. I'll make the following patch:

- r = set_regdom(rd, REGD_SOURCE_CRDA);
- /* set_regdom took ownership */
- rd = NULL;
+ /* set_regdom takes ownership of rd */
+ return set_regdom(rd, REGD_SOURCE_CRDA);

johannes