Re: [PATCH wireless-next 2/2] rtlwifi: rtl8821ae: phy: fix an undefined bitwise shift behavior

[Dan Carpenter <dan.carpenter@linaro.org>]

On Wed, Nov 22, 2023 at 05:02:12PM +0800, Su Hui wrote:
> Clang staic checker warning:
> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c:184:49:
> 	The result of the left shift is undefined due to shifting by '32',
> 	which is greater or equal to the width of type 'u32'.
> 	[core.UndefinedBinaryOperatorResult]
> 
> If the value of the right operand is negative or is greater than or
> equal to the width of the promoted left operand, the behavior is
> undefined.[1][2]
> 
> For example, when using different gcc's compilation optimizaation options
> (-O0 or -O2), the result of '(u32)data << 32' is different. One is 0, the
> other is old value of data. Adding an u64 cast to fix this problem.
> 
> [1]:https://stackoverflow.com/questions/11270492/what-does-the-c-
> standard-say-about-bitshifting-more-bits-than-the-width-of-type
> [2]:https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> 
> Fixes: 21e4b0726dc6 ("rtlwifi: rtl8821ae: Move driver from staging to regular tree")
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
> index 6df270e29e66..89713e0587b5 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
> @@ -106,7 +106,7 @@ u32 rtl8821ae_phy_query_bb_reg(struct ieee80211_hw *hw, u32 regaddr,
>  		regaddr, bitmask);
>  	originalvalue = rtl_read_dword(rtlpriv, regaddr);
>  	bitshift = _rtl8821ae_phy_calculate_bit_shift(bitmask);
> -	returnvalue = (originalvalue & bitmask) >> bitshift;
> +	returnvalue = (u64)(originalvalue & bitmask) >> bitshift;

This is a right shift, not a left shift. << vs >>.

>  
>  	rtl_dbg(rtlpriv, COMP_RF, DBG_TRACE,
>  		"BBR MASK=0x%x Addr[0x%x]=0x%x\n",
> @@ -128,7 +128,7 @@ void rtl8821ae_phy_set_bb_reg(struct ieee80211_hw *hw,
>  		originalvalue = rtl_read_dword(rtlpriv, regaddr);
>  		bitshift = _rtl8821ae_phy_calculate_bit_shift(bitmask);
>  		data = ((originalvalue & (~bitmask)) |
> -			((data << bitshift) & bitmask));
> +			(((u64)data << bitshift) & bitmask));

The checker is printing an accurate warning, however, I'm not sure the
fix is correct.  Obviously, shift wrapping is bad and your patch would
eliminate that possibility.  However, data is a u32 so we end up
discarding the high 32 bits.  I can imagine a different static checker
would complain about that.

Perhaps, a better way to silence the warning is to just change
_rtl8821ae_phy_calculate_bit_shift() to not return 32 bits?  Do we
really ever pass bitmask 0?  No idea...

regards,
dan carpenter

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
index 5323ead30db0..42885e3a458f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
@@ -29,9 +29,7 @@ static void _rtl8821ae_phy_rf_serial_write(struct ieee80211_hw *hw,
 					   u32 data);
 static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask)
 {
-	u32 i = ffs(bitmask);
-
-	return i ? i - 1 : 32;
+	return ffs(bitmask) - 1;
 }
 static bool _rtl8821ae_phy_bb8821a_config_parafile(struct ieee80211_hw *hw);
 /*static bool _rtl8812ae_phy_config_mac_with_headerfile(struct ieee80211_hw *hw);*/