From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f178.google.com (mail-pg1-f178.google.com [209.85.215.178]) by mx.groups.io with SMTP id smtpd.web10.2124.1685951647004357250 for ; Mon, 05 Jun 2023 00:54:07 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=V/R1NB7J; spf=pass (domain: ventanamicro.com, ip: 209.85.215.178, mailfrom: rsingh@ventanamicro.com) Received: by mail-pg1-f178.google.com with SMTP id 41be03b00d2f7-543b17343baso276254a12.0 for ; Mon, 05 Jun 2023 00:54:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1685951646; x=1688543646; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=/Jerbv8R/saA3kGYfvqkuE3jjzNna57cGwlP2HkaNNc=; b=V/R1NB7Jfrn6KoWVpe8R0qYjuMrfqBgoBzeMwoERZbTVzNGdhDxuvFk5KtUOUSGqQS b3xeT0mffdFZddveQKw0JQgWgbEgKZufkNJf43NaDBCjEty40wLfAo8v4m2L4K4ueUvk bNWXuOuvipbTbJSpyYy9FzmOSAeLawXECthaIzSIxAqRhma9efgumItC9OeHBCB5GtFc iG56/bxlNhwiBQNOPrqECWMnfCK5Bho2CF7LA28j7xjFfua4275j/HrvcCaNTCtErYf8 HmMWsx5nOlwTbmftlhmMk1AmqTfFsvH2OGEMKeK1GV4BQPiWIbcE4Bmhz7dnp6bsjVuX ywmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685951646; x=1688543646; 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=/Jerbv8R/saA3kGYfvqkuE3jjzNna57cGwlP2HkaNNc=; b=CvNzY16Z3HkvKpTDhx/JVTYpxwJSJgI6EroBl/ai8yRrDErVpSfI/rDtG/5i5lBbg6 t4irZEJoW+wY3hFUCs40fJ5Fh0OMNF5PP3EuSqr27T2AzSYNmGI1yy2/UPxO785h+qee NstI1rwYxR4+cI5T4pi0lVXphj9xvJb6jPqIR/OAhxLh8HmrAey90QCqHf6oZij+AJ+k j/uYK+MgH7ePmO4GCHMiosw7e4qawBQqcGTrl7qTLkpEKwWfNDi1kZ5KWG+kraxUrvqO 5aIkPcrCOLcT4PxhQu13ghbdtKXDQ5gTXjFhieBiDO/ht+w4aQeY6/8tleii8HffO8Dn ls7A== X-Gm-Message-State: AC+VfDw92b41aWTj9HydSsT9Fx1cj508ccTGgi9gDmt2DM81Z3p5n06K JsWdCsIipjA/wT+qcXz4nSEmr2V1QhpmB+c2uJZotw== X-Google-Smtp-Source: ACHHUZ64CknQHkg2J7RIg6dupKSOHTD7LZGxCEskkCBR3Y/OtzpOvkRgWWQQUoJmoyO7BREvlhuMhKoPcbFvUWXW80Q= X-Received: by 2002:a17:90a:de8e:b0:255:6133:f561 with SMTP id n14-20020a17090ade8e00b002556133f561mr2417875pjv.10.1685951646312; Mon, 05 Jun 2023 00:54:06 -0700 (PDT) MIME-Version: 1.0 References: <20230602070946.83730-1-rsingh@ventanamicro.com> In-Reply-To: From: Ranbir Singh Date: Mon, 5 Jun 2023 13:23:55 +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="000000000000fcbf6705fd5d347d" --000000000000fcbf6705fd5d347d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Please read the Coverity report attached in BZ 4204 for more details on sign-extension issue. 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 ? 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. On Mon, Jun 5, 2023 at 4:28=E2=80=AFAM Ard Biesheuvel wro= te: > 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 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 > > 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). > --000000000000fcbf6705fd5d347d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Please read the Coverity report attached in BZ 4204 for mo= re details on sign-extension issue.

The data types of=C2= =A0logic_sector_size_hi and=C2=A0logic_sector_size_lo are UINT16 and Isn'= ;t the return type of sizeof already unsigned ?=C2=A0
=
Now I have no means to run Coverity and test further ch= anges.
Anyway, my understanding back then was that the sign-exten= sion was primarily happening because of the 16 bits left shift operation. I= did test the patch for coverity warning resolution back in Jan'23 whic= h went off.


=
On Mon, Jun 5, 2023 at 4:28=E2=80=AFA= M Ard Biesheuvel <ardb@kernel.org= > wrote:
On F= ri, 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<= br> > 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 <r= ay.ni@intel.com>
> REF: https://bugzilla.tianocore.org/show_b= ug.cgi?id=3D4204
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> ---
>=C2=A0 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-<= br> >=C2=A0 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 (
>=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_sector_sup= port & BIT12) !=3D 0) {
> -=C2=A0 =C2=A0 =C2=A0 BlockSize =3D (UINT32)(((IdentifyData->AtaDat= a.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector= _size_lo) * sizeof (UINT16));
> +=C2=A0 =C2=A0 =C2=A0 BlockSize =3D (((UINT32)(IdentifyData->AtaDat= a.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).
--000000000000fcbf6705fd5d347d--