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.
> 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.man...@nxp.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. 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"?
>
> return cpu_to_le32(temp);
> }
> --
> 2.25.1
>
