Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

["Ard Biesheuvel" <ardb@kernel.org>]

On Mon, 5 Jun 2023 at 09:54, Ranbir Singh <rsingh@ventanamicro.com> wrote:
>
> Please read the Coverity report attached in BZ 4204 for more details on sign-extension issue.
>

I did read it - explanation below.

> The data types of logic_sector_size_hi and logic_sector_size_lo are UINT16 and Isn't the return type of sizeof already unsigned ?
>

Yes, it is unsigned long, so 64 bits wide on LP64 architectures

> Now I have no means to run Coverity and test further changes.
> Anyway, my understanding back then was that the sign-extension was primarily happening because of the 16 bits left shift operation. I did test the patch for coverity warning resolution back in Jan'23 which went off.
>

The warning actually describes exactly what is happening:
- The type of a UINT16 shifted left by 16 is promoted to signed integer.
- The multiplication by sizeof() converts the former result to
unsigned integer before widening to unsigned long integer.

This means that the widening does not perform any sign extension,
which is what Coverity warns about.

None of this actually matters, given that those upper bits get
truncated again anyway when we assign to BlockSize, which is only 32
bits wide.

Removing the outer (UINT32) cast is not the correct solution, as it
may result in spurious truncation warnings on some toolchains.

If you want to silence this warning (I wouldn't call it a fix), you
need to either prevent the widening, or cast the intermediate result
to (UINT32), and the latter is what your patch does. So it silences
the warning, but now, the type of the right hand side of the
assignment is UINTN not UINT32.

So either leave the outer (UINT32) cast in place, or move it to before
the sizeof() as I suggested before: in both cases, there is no longer
any promotion to unsigned long, which should make the warning go away.



>
> On Mon, Jun 5, 2023 at 4:28 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh <rsingh@ventanamicro.com> wrote:
>> >
>> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> >
>> > Line number 1348 does contain a typecast with UINT32, but it is after
>> > all the operations (16-bit left shift followed by OR'ing) are over.
>> > To avoid any SIGN_EXTENSION, typecast the intermediate result after
>> > 16-bit left shift operation immediately with UINT32.
>> >
>>
>> What is wrong with sign extension?
>>
>> > Cc: Hao A Wu <hao.a.wu@intel.com>
>> > Cc: Ray Ni <ray.ni@intel.com>
>> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
>> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> > ---
>> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
>> > index 50406fe0270d..70c4ca27dc68 100644
>> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
>> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
>> > @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
>> >      // Check logical block size
>> >      //
>> >      if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
>> > -      BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
>> > +      BlockSize = (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
>>
>>
>> The outer parens are now redundant, which means you're assigning
>> something to BlockSize whose type is based on the type of <something>
>> * sizeof(UINT16), which is unsigned long not unsigned int, so this
>> will produce a truncation warning on some compilers.
>>
>> If you want to suppress the coverity warning without introducing new
>> ones, better to cast the sizeof() to (UINT32).