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]

Message-ID: <837f1d5f-64fa-2496-6379-09e5e95252f4@kernel.org>
Date:   Wed, 12 Jul 2023 16:50:55 +0300
From:   Roger Quadros <rogerq@...nel.org>
To:     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 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?

>  }
>  
>  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);
>  }

-- 
cheers,
-roger