Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <22bf2f78-587d-429b-867f-f73e542018d2@quicinc.com>
Date:   Thu, 5 May 2022 10:40:15 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH v2] cfg80211: Add support for sending more than two AKMs
 in crypto settings
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>
References: <1650344143-1615-1-git-send-email-quic_vjakkam@quicinc.com>
 <cb419675d2ae276d9b4eac8ab5deafe62167051f.camel@sipsolutions.net>
From:   Veerendranath Jakkam <quic_vjakkam@quicinc.com>
In-Reply-To: <cb419675d2ae276d9b4eac8ab5deafe62167051f.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/5/2022 2:24 AM, Johannes Berg wrote:
> I don't know why, but this patch seems to cause memory corruption and
> various issues when I run the hwsim tests.
> 
Oh.. I ran some hwsim tests from hostap.git to verify the changes but 
didn't face any issues. Are you seeing the issues with any specific 
hwsim tests? Also, the issues are coming without any of the below 
changes also(i.e. with actual patch i sent)?

> I did make a few minor changes, but I think those were OK.
> 
> 
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -1112,7 +1112,7 @@ struct cfg80211_crypto_settings {
>>   	int n_ciphers_pairwise;
>>   	u32 ciphers_pairwise[NL80211_MAX_NR_CIPHER_SUITES];
>>   	int n_akm_suites;
>> -	u32 akm_suites[NL80211_MAX_NR_AKM_SUITES];
>> +	u32 *akm_suites;
> 
> I made this const, that required a bit of juggling in the wext code, but
> it's probably still better overall.
> 
>>   
>> +	/*
>> +	 * Maximum number of AKM suites allowed for NL80211_CMD_CONECT,
>> +	 * NL80211_CMD_ASSOCIATE and NL80211_CMD_START_AP, must be at least
>> +	 * NL80211_MAX_NR_AKM_SUITES to avoid issues with legacy userspace.
>> +	 */
>> +	if (WARN_ON(wiphy->max_num_akm_suites &&
>> +		    wiphy->max_num_akm_suites < NL80211_MAX_NR_AKM_SUITES))
>> +		return -EINVAL;
> 
> I made it so here if it's 0 we set it to NL80211_MAX_NR_AKM_SUITES, that
> way we don't need to have all the questions about it being 0 later.
> 
>> +		if (nla_put_u16(msg, NL80211_ATTR_MAX_NUM_AKM_SUITES,
>> +				rdev->wiphy.max_num_akm_suites ?
>> +				rdev->wiphy.max_num_akm_suites :
>> +				NL80211_MAX_NR_AKM_SUITES))
>> +			goto nla_put_failure;
> 
> e.g. here, that was then without the ternary operator
> 
>> +		int max_num_akm_suites = NL80211_MAX_NR_AKM_SUITES;
>>   
>>   		data = nla_data(info->attrs[NL80211_ATTR_AKM_SUITES]);
>>   		len = nla_len(info->attrs[NL80211_ATTR_AKM_SUITES]);
>> @@ -10261,10 +10269,13 @@ static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev,
>>   		if (len % sizeof(u32))
>>   			return -EINVAL;
>>   
>> -		if (settings->n_akm_suites > NL80211_MAX_NR_AKM_SUITES)
>> +		if (rdev->wiphy.max_num_akm_suites)
>> +			max_num_akm_suites = rdev->wiphy.max_num_akm_suites;
>> +
>> +		if (settings->n_akm_suites > max_num_akm_suites)
>>   			return -EINVAL;
>>   
>> -		memcpy(settings->akm_suites, data, len);
>> +		settings->akm_suites = data;
> 
> and all that complexity also goes away
> 
>> +	if (connect->crypto.n_akm_suites) {
>> +		wdev->conn->params.crypto.akm_suites =
>> +			kcalloc(connect->crypto.n_akm_suites, sizeof(u32),
>> +				GFP_KERNEL);
>> +		if (!wdev->conn->params.crypto.akm_suites) {
>> +			cfg80211_sme_free(wdev);
>> +			return -ENOMEM;
>> +		}
>> +
>> +		wdev->conn->params.crypto.n_akm_suites =
>> +			connect->crypto.n_akm_suites;
>> +		memcpy(wdev->conn->params.crypto.akm_suites,
>> +		       connect->crypto.akm_suites,
>> +		       sizeof(u32) * connect->crypto.n_akm_suites);
> 
> I made this use kmemdup(), I don't think it makes a big difference.
> 
> Anyway, it crashes. Perhaps an invalid free, because the middle of some
> netlink packet is being freed or so? >
> Please check.
Sure, I will try with suggested changes and update you

> 
> johannes

-- 
veeru