From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.2731.1685954680920999904 for ; Mon, 05 Jun 2023 01:44:41 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=DHm3bUTs; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 531A6619EE for ; Mon, 5 Jun 2023 08:44:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B81EBC43446 for ; Mon, 5 Jun 2023 08:44:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685954678; bh=TrrSXFUBhdN7+RBUPcjCR3BOjqgcWXgv+drQ9mAqcnw=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=DHm3bUTsAiLyI+p6Htcjr8ma0dyF5JFHEYGeiK+5LjLQebnmDOKAGyBC4Kuhm4yMg nQv0bcE9Mtwi8DMHvddqWTJ3nFu0U43WySl9Lf7cM9EF+ScobkZzrEdxxa5yFJ5VHq X/jKOX0LjjEO7yV+j8xt95oZxq7gpuhWQTQFmImH8Z3pAL6G9bfC1u3mz3Hk8zcWb0 57zOuEvgUK9gAuEget/QxPdcM03Ku1A9cQyHUg7kATLF86FL8uqJXQH1sZxLlsR9px DzWS0+iBWiQfHzqRcShOQ1UFa5AzJ5DoyEHiOlnxsx+5MxUSuO8syqOReqpQEafxBJ JPSeq7sRyj8VQ== Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-4f3bb395e69so5691597e87.2 for ; Mon, 05 Jun 2023 01:44:38 -0700 (PDT) X-Gm-Message-State: AC+VfDyZMmsrRGrzlKEHFoDstiKOnVxy5RPMqlmDfgIQEIZbJ9WKXqJA neP1D7IWj//mnkCKOOCHZymVC1hvK938ygRmZ1w= X-Google-Smtp-Source: ACHHUZ6XC8ys9NpYeEY0X4g4D5hfPcO7yC2OZPHuYtueCoTzNOmuaJMVXAOKJdzmcbKmPIfV+9pxtLRR6xpST/vNvQI= X-Received: by 2002:ac2:5d6c:0:b0:4f4:d5d9:7fca with SMTP id h12-20020ac25d6c000000b004f4d5d97fcamr4412388lft.53.1685954676658; Mon, 05 Jun 2023 01:44:36 -0700 (PDT) MIME-Version: 1.0 References: <20230602070946.83730-1-rsingh@ventanamicro.com> In-Reply-To: From: "Ard Biesheuvel" Date: Mon, 5 Jun 2023 10:44:25 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue To: Ranbir Singh Cc: devel@edk2.groups.io, Hao A Wu , Ray Ni Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 5 Jun 2023 at 09:54, Ranbir Singh wrote: > > Please read the Coverity report attached in BZ 4204 for more details on s= ign-extension issue. > I did read it - explanation below. > The data types of logic_sector_size_hi and logic_sector_size_lo are UINT1= 6 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 primar= ily happening because of the 16 bits left shift operation. I did test the p= atch 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 w= rote: >> >> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh wrot= e: >> > >> > 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_siz= e_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16)= ); >> > + BlockSize =3D (((UINT32)(IdentifyData->AtaData.logic_sector_siz= e_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).