<div dir="ltr"><div dir="ltr">Thanks Ard for adding the explanation. <div> </div><div>Ok, so retaining outer cast along with intermediate casting</div><div> 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. </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 5, 2023 at 2:14 PM Ard Biesheuvel <<a href="mailto:ardb@kernel.org">ardb@kernel.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, 5 Jun 2023 at 09:54, Ranbir Singh <<a href="mailto:rsingh@ventanamicro.com" target="_blank">rsingh@ventanamicro.com</a>> 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 <<a href="mailto:ardb@kernel.org" target="_blank">ardb@kernel.org</a>> wrote: >> >> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh <<a href="mailto:rsingh@ventanamicro.com" target="_blank">rsingh@ventanamicro.com</a>> 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 <<a href="mailto:hao.a.wu@intel.com" target="_blank">hao.a.wu@intel.com</a>> >> > Cc: Ray Ni <<a href="mailto:ray.ni@intel.com" target="_blank">ray.ni@intel.com</a>> >> > REF: <a href="https://bugzilla.tianocore.org/show_bug.cgi?id=4204" rel="noreferrer" target="_blank">https://bugzilla.tianocore.org/show_bug.cgi?id=4204</a> >> > 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). </blockquote></div></div> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/105721">View/Reply Online (#105721)</a> | | <a target="_blank" href="https://groups.io/mt/99293622/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>