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 for Android: free password hash cracker in your pocket

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

Message-ID: <57677b6102f4424093875fb8f3d5e2f2@AcuMS.aculab.com>
Date:   Tue, 30 Mar 2021 08:40:50 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Vladimir Oltean' <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....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

From: Vladimir Oltean
> Sent: 29 March 2021 17:24
> 
> 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.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.

Not only that the high bits get thrown away by the assignment.
The change just gives the reader more to parse for zero benefit.

> 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"?

That would be much better.
It may save the value having to be masked with 0xff as well.

Regardless of the domain of function parameters/results (and local
variables) using machine-register sized types will typically give
better code.
x86 is probably unique in having sub-32bit arithmetic.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)