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

[Ranbir Singh <rsingh@ventanamicro.com>]

[-- Attachment #1: Type: text/plain, Size: 4701 bytes --]

Thanks Ard for adding the explanation.

Ok, so retaining outer cast along with intermediate casting

BlockSize = (UINT32)(((UINT32)(IdentifyData->AtaData.logic_sector_size_hi
<< 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));

should be acceptable. I considered outer cast is being implicitly done as
per the data type of *BlockSize *and also there was no Coverity warning
after the removal*.*




On Mon, Jun 5, 2023 at 2:14 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> 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).
>

[-- Attachment #2: Type: text/html, Size: 6878 bytes --]