linux-kernel - Re: [PATCH net] net: ethernet: ti: cpsw_ale: Fix cpsw_ale_get_field()/cpsw_ale_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Jul 2023 12:00:41 +0530
From:   "Patil, Tanmay" <t-patil@...com>
To:     Roger Quadros <rogerq@...nel.org>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <srk@...com>
Subject: Re: [PATCH net] net: ethernet: ti: cpsw_ale: Fix
 cpsw_ale_get_field()/cpsw_ale_set_field()



On 7/12/2023 7:20 PM, Roger Quadros wrote:
> 
> 
> On 12/07/2023 14:06, Siddharth Vadapalli wrote:
>> From: Tanmay Patil <t-patil@...com>
>>
>> CPSW ALE has 75 bit ALE entries which are stored within three 32 bit words.
>> The cpsw_ale_get_field() and cpsw_ale_set_field() functions assume that the
>> field will be strictly contained within one word. However, this is not
>> guaranteed to be the case and it is possible for ALE field entries to span
>> across up to two words at the most.
>>
>> Fix the methods to handle getting/setting fields spanning up to two words.
>>
>> Fixes: db82173f23c5 ("netdev: driver: ethernet: add cpsw address lookup engine support")
>> Signed-off-by: Tanmay Patil <t-patil@...com>
>> [s-vadapalli@...com: rephrased commit message and added Fixes tag]
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
>> ---
>>   drivers/net/ethernet/ti/cpsw_ale.c | 24 +++++++++++++++++++-----
>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
>> index 0c5e783e574c..64bf22cd860c 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
>> @@ -106,23 +106,37 @@ struct cpsw_ale_dev_id {
>>   
>>   static inline int cpsw_ale_get_field(u32 *ale_entry, u32 start, u32 bits)
>>   {
>> -	int idx;
>> +	int idx, idx2;
>> +	u32 hi_val = 0;
>>   
>>   	idx    = start / 32;
>> +	idx2 = (start + bits - 1) / 32;
>> +	/* Check if bits to be fetched exceed a word */
>> +	if (idx != idx2) {
>> +		idx2 = 2 - idx2; /* flip */
>> +		hi_val = ale_entry[idx2] << ((idx2 * 32) - start);
>> +	}
>>   	start -= idx * 32;
>>   	idx    = 2 - idx; /* flip */
>> -	return (ale_entry[idx] >> start) & BITMASK(bits);
>> +	return (hi_val + (ale_entry[idx] >> start)) & BITMASK(bits);
> 
> Should this be bit-wise OR instead of ADD?
> 

As the hi_val has been declared and left shifted in this function, we 
are sure that the trailing bits are all '0'. Hence we can directly add them.

>>   }
>>   
>>   static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
>>   				      u32 value)
>>   {
>> -	int idx;
>> +	int idx, idx2;
>>   
>>   	value &= BITMASK(bits);
>> -	idx    = start / 32;
>> +	idx = start / 32;
>> +	idx2 = (start + bits - 1) / 32;
>> +	/* Check if bits to be set exceed a word */
>> +	if (idx != idx2) {
>> +		idx2 = 2 - idx2; /* flip */
>> +		ale_entry[idx2] &= ~(BITMASK(bits + start - (idx2 * 32)));
>> +		ale_entry[idx2] |= (value >> ((idx2 * 32) - start));
>> +	}
>>   	start -= idx * 32;
>> -	idx    = 2 - idx; /* flip */
>> +	idx = 2 - idx; /* flip */
>>   	ale_entry[idx] &= ~(BITMASK(bits) << start);
>>   	ale_entry[idx] |=  (value << start);
>>   }
>