netdev - RE: [PATCH net v2] enetc: Avoid implicit sign extension

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: <AM0PR04MB6754458AFCA2834FEAC852E4967E9@AM0PR04MB6754.eurprd04.prod.outlook.com>
Date:   Mon, 29 Mar 2021 17:08:23 +0000
From:   Claudiu Manoil <claudiu.manoil@....com>
To:     Vladimir Oltean <vladimir.oltean@....com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>
Subject: RE: [PATCH net v2] enetc: Avoid implicit sign extension

>-----Original Message-----
>From: Vladimir Oltean <vladimir.oltean@....com>
>Sent: Monday, March 29, 2021 7:24 PM
>To: Claudiu Manoil <claudiu.manoil@....com>
>Cc: netdev@...r.kernel.org; Jakub Kicinski <kuba@...nel.org>; David S .
>Miller <davem@...emloft.net>
>Subject: Re: [PATCH net v2] enetc: Avoid implicit sign extension
>
>On Mon, Mar 29, 2021 at 05:14:43PM +0300, Claudiu Manoil wrote:
>> Static analysis tool reports:
>> "Suspicious implicit sign extension - 'flags' with type u8 (8 bit,
>> unsigned) is promoted in 'flags' << 24 to type int (32 bits, signed),
>> then sign-extended to type unsigned long long (64 bits, unsigned).
>> If flags << 24 is greater than 0x7FFFFFFF, the upper bits of the result
>
>This is a backwards way of saying 'if flags & BIT(7) is set', no? But
>BIT(7) is ENETC_TXBD_FLAGS_F (the 'final BD' bit), and I've been testing
>SO_TXTIME with single BD frames, and haven't seen this problem.
>

Better be safe than sorry.

>> will all be 1."
>>
>> Use lower_32_bits() to avoid this scenario.
>>
>> Fixes: 82728b91f124 ("enetc: Remove Tx checksumming offload code")
>>
>> Signed-off-by: Claudiu Manoil <claudiu.manoil@....com>
>> ---
>> v2 - added 'fixes' tag
>>
>>  drivers/net/ethernet/freescale/enetc/enetc_hw.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
>b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
>> index 00938f7960a4..07e03df8af94 100644
>> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
>> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
>> @@ -535,8 +535,8 @@ static inline __le32 enetc_txbd_set_tx_start(u64
>tx_start, u8 flags)
>>  {
>>  	u32 temp;
>>
>> -	temp = (tx_start >> 5 & ENETC_TXBD_TXSTART_MASK) |
>> -	       (flags << ENETC_TXBD_FLAGS_OFFSET);
>> +	temp = lower_32_bits(tx_start >> 5 & ENETC_TXBD_TXSTART_MASK)
>|
>> +	       (u32)(flags << ENETC_TXBD_FLAGS_OFFSET);
>
>I don't actually understand why lower_32_bits called on the TX time
>helps, considering that the value is masked already. 

Just want to ensure it's handled as u32 and not u64. I also think lower_32_bits()
is the cleanest way to convert from u64 to u32, in this case at least.

>The static analysis
>tool says that the right hand side of the "|" operator is what is
>sign-extended:
>
>	       (flags << ENETC_TXBD_FLAGS_OFFSET);
>
>Isn't it sufficient that you replace "u8 flags" in the function
>prototype with "u32 flags"?
>

I prefer to cast it to u32 after the shift. The 'flags' argument passed to this helper
function is always u8 as it matches the 8-bit field of the Tx BD DMA structure.