From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f170.google.com (mail-pg1-f170.google.com [209.85.215.170]) by mx.groups.io with SMTP id smtpd.web11.3200.1685956540677828770 for ; Mon, 05 Jun 2023 02:15:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ventanamicro.com header.s=google header.b=a2jR3i9P; spf=pass (domain: ventanamicro.com, ip: 209.85.215.170, mailfrom: rsingh@ventanamicro.com) Received: by mail-pg1-f170.google.com with SMTP id 41be03b00d2f7-543a6cc5f15so939336a12.2 for ; Mon, 05 Jun 2023 02:15:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1685956540; x=1688548540; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=bBf5a7UN4UlKodBjaLVrFAmRXLQiIl28eTgw/MK0GZ4=; b=a2jR3i9P5R/WWgk0OqCtWc49Rwl7S67UmmImZYx0uU+o5YEDBJULgUn5C323ymfMJa I6I55qHMXGR5seZo7MuUUS+jrpfTSswAFT2jj74nW5aPX5EHHqdKO5KD/8J4VqOBWIIa 840TcfEsmGHP2ORIyEu9QPobtHBGG9UbwKjiKL7c7NXDFLoyh8Q3f/0Mb8zIYjylhrb9 tZZAT6+RvSlTpZJ8NTMMgVS4Imgu1hQR+cmDOOl0mZwwefnVtLHF0gYI5T+6tQQs7LT6 tJQaHCez5GXwPYacmNCjqeG1huUg6d0G5LVWa1F9/vZlQkpB648EihMvTHzj6cslKOND OI1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685956540; x=1688548540; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=bBf5a7UN4UlKodBjaLVrFAmRXLQiIl28eTgw/MK0GZ4=; b=EeMEeZIzkStFjlym5r/DM0VS5SkPEhzyxHByzrzIQYJix2Hqt9/TcgzTx9bodXsJQp D0Jn0tog7ioXuH5l7NDCZV2j0pwURctbZDoSmq0xY5yca4NpjAwTMHwZuF7uUTShkD8l hFYk15C4bVpoHAThg/KNohIpH2X6ZDqbfIwpUrjLUb/WdsJl6BpfUSeJKABWdzUERwWY 2pvmhVjDdPZFJ3XE2fFTcnEwPjDKCF+XqhZJWQtvO98Jon0dguDWhgJAdv+FIOKuyG51 L9FCGgEbE5BcDqGyKxCKPLbF5dnqt+VGkR4A4rfiep3DF4iOcEVgCv5y4Jju13qFifpq 1CHQ== X-Gm-Message-State: AC+VfDzO0t6cHseSi7nEuulS+AEjDEGxkE79QeH3fsNo45lxTrkkJ7kW KGJ+K3mugNRkPYxVAQzlfZaQHRsCCdIcJ8snpYz1ow== X-Google-Smtp-Source: ACHHUZ7eGG50ToIVqel8GiE5ajoCu4OAqo8c4vocj9/PFo2ibBWCv6URrSs+c5dt+T+R6Dxmn2gnuryPQ+WkKzIf3HQ= X-Received: by 2002:a17:902:8506:b0:1ae:5914:cbec with SMTP id bj6-20020a170902850600b001ae5914cbecmr6726804plb.10.1685956539899; Mon, 05 Jun 2023 02:15:39 -0700 (PDT) MIME-Version: 1.0 References: <20230602070946.83730-1-rsingh@ventanamicro.com> In-Reply-To: From: Ranbir Singh Date: Mon, 5 Jun 2023 14:45:29 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue To: Ard Biesheuvel Cc: devel@edk2.groups.io, Hao A Wu , Ray Ni Content-Type: multipart/alternative; boundary="000000000000aaf2a305fd5e5800" --000000000000aaf2a305fd5e5800 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks Ard for adding the explanation. Ok, so retaining outer cast along with intermediate casting BlockSize =3D (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=E2=80=AFPM Ard Biesheuvel wro= te: > On Mon, 5 Jun 2023 at 09:54, Ranbir Singh 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 te= st > 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=E2=80=AFAM Ard Biesheuvel = wrote: > >> > >> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh > wrote: > >> > > >> > From: Ranbir Singh > >> > > >> > Line number 1348 does contain a typecast with UINT32, but it is afte= r > >> > 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 > >> > Cc: Ray Ni > >> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4204 > >> > Signed-off-by: Ranbir Singh > >> > --- > >> > 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) != =3D > 0) { > >> > - BlockSize =3D > (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) | > IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16)); > >> > + BlockSize =3D > (((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 > >> * 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). > --000000000000aaf2a305fd5e5800 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks Ard for adding the explanation.

Ok, so retaining outer cast along with intermediate ca= sting
=09 =09

BlockSize =3D=C2=A0(UINT32)(((UINT32)(IdentifyData->AtaData.logic_se= ctor_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));

should be acceptable. I considered outer cast is being imp= licitly 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=E2=80=AFPM Ard Biesheuvel <ardb@kernel.org> wrote:
On Mon, 5 Jun 2023 at 09:54, Ran= bir Singh <= rsingh@ventanamicro.com> wrote:
>
> Please read the Coverity report attached in BZ 4204 for more details o= n sign-extension issue.
>

I did read it - explanation below.

> The data types of logic_sector_size_hi and logic_sector_size_lo are UI= NT16 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 pri= marily happening because of the 16 bits left shift operation. I did test th= e patch for coverity warning resolution back in Jan'23 which went off.<= br> >

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=E2=80=AFAM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh <rsingh@ventanamicro.com> wr= ote:
>> >
>> > 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.o= rg/show_bug.cgi?id=3D4204
>> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com> >> > ---
>> >=C2=A0 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.= c | 2 +-
>> >=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPa= ssThru.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 (
>> >=C2=A0 =C2=A0 =C2=A0 // Check logical block size
>> >=C2=A0 =C2=A0 =C2=A0 //
>> >=C2=A0 =C2=A0 =C2=A0 if ((IdentifyData->AtaData.phy_logic_s= ector_support & BIT12) !=3D 0) {
>> > -=C2=A0 =C2=A0 =C2=A0 BlockSize =3D (UINT32)(((IdentifyData-&= gt;AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.log= ic_sector_size_lo) * sizeof (UINT16));
>> > +=C2=A0 =C2=A0 =C2=A0 BlockSize =3D (((UINT32)(IdentifyData-&= gt;AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.log= ic_sector_size_lo) * sizeof (UINT16));
>>
>>
>> The outer parens are now redundant, which means you're assigni= ng
>> something to BlockSize whose type is based on the type of <some= thing>
>> * 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 n= ew
>> ones, better to cast the sizeof() to (UINT32).
--000000000000aaf2a305fd5e5800--