[PATCH v6 02/17] nl80211: Migrate to current netlink key message format

Alexander Wetzel alexander at wetzel-home.de
Thu Sep 19 09:16:09 PDT 2019


> On Sun, Sep 15, 2019 at 10:08:22PM +0200, Alexander Wetzel wrote:
>> @@ -3045,26 +3046,31 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss,
> 
>> +	key_msg = nlmsg_alloc();
> ...
> 
>> +	if (nla_put_u8(key_msg, NL80211_KEY_IDX, key_idx) ||
>> +	    nla_put_nested(msg, NL80211_ATTR_KEY, key_msg))
>>   		goto fail;
> ...
> 
>> +	key_msg = nlmsg_alloc();
> 
> This seems to leak memory (that nla_put_nested() used key_msg, but did
> not free it). And also leave in key information in heap.

Thanks for pointing that out...

It kind of makes sense: I was simply assuming that nla_put_nested() 
would fold the information into the existing netlink message and never 
tough to cross check that. But when it's copying the information we have 
a memory leak...

So I've also now added
	nlmsg_free(key_msg);
after each call to nla_put_nested() in my tree.


>> +	if (nla_put_nested(msg, NL80211_ATTR_KEY, key_msg))
>> +		goto fail;
>> +
>>   	ret = send_and_recv_msgs(drv, msg, NULL, NULL);
> 
> Same here.
> 
>> +fail2:
>> +	nl80211_nlmsg_clear(key_msg);
>> +	nlmsg_free(key_msg);
> 
> These need to be done in the success cases as well.
> 
> No need to send this patch again because of this, though, since I've
> already addressed that in my work version.
> 

Ok.

But as always when sending a patch out I already found another issue:

handle_extended_key_id() for hostapd is kind of stupid in the current 
patchset. By moving it some lines we can also fold the TKIP check into 
it. I've mostly done that - more tests pending - but then I also noticed 
that handle_extended_key() should be ok to check less and want to look 
into that, too. (Should be trivial, so I probably can do that today.)

I'm also thinking about to dropping all the "& 0x03" in 
src/ap/wpa_auth.c. The one I added for keyidx_active should be "& 0x01" 
but since we do that on trusted internal variables which already have 
the correct values there should be no need to do that at all...

Do you maybe prefer just a diff with the changes compared to V6?
I could either act like V6 has been merged or even just send you one 
patch with the accumulated fixes.


Alexander



More information about the Hostap mailing list