From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id E850D941335 for ; Mon, 6 Jan 2025 06:38:32 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=5qKNlpHg8H2I3h5zbzDTas1J0inpGI7SXvzJSYrC2Jk=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240830; t=1736145512; v=1; x=1736404711; b=trMjZ6jhdlQTNdbGD0ygHFio/5Jdf4QME6vN0hYfFYeU1dfeLfox3r2JutVjffJO/7WTqC7m VU3x83IRBJSfcWnc5iR4Hz16E8zyepKSF6QFXUmDVLlS8gNxtZimHsvSFp7HR5HYqbWLDEtGuzT HTqimLYj/2b701S3J09G5R0TVN5n6NOgz9cc7CNBJN8C2FccKuHGMuYVjd2fS3E3OihieByv7V4 sQmbx+RIqwHewe1UD/tfGH1a/Cz+L10yV5WD2TGsuuGZXGvJHxEnFwLmhewazjwa6X/q6hYijOt Mvh5jLFO0WV0z7Y8CEHmyccBpWeLqvmDUOA8QmWwnRjXQ== X-Received: by 127.0.0.2 with SMTP id KJP6YY7687511x74Dxo1rrMO; Sun, 05 Jan 2025 22:38:31 -0800 X-Received: from mail-vs1-f50.google.com (mail-vs1-f50.google.com [209.85.217.50]) by mx.groups.io with SMTP id smtpd.web11.54167.1736145510629162314 for ; Sun, 05 Jan 2025 22:38:30 -0800 X-Received: by mail-vs1-f50.google.com with SMTP id ada2fe7eead31-4affd0fb6adso3836097137.1 for ; Sun, 05 Jan 2025 22:38:30 -0800 (PST) X-Gm-Message-State: 6OMUpURS3DfZvjbcbFBzmjdtx7686176AA= X-Gm-Gg: ASbGncuVYgBJ/moHaLrl+yYB1u9C5LCCUHLoifCNenkCa+egqd1IT2YkNqQiBDLdyB0 /eQt3nrrlfmED4Qmz2CgNtWeF7z3lsJcSI8aKMcMGAAr70f7oITT2TdApij71DXa/72wkRls= X-Google-Smtp-Source: AGHT+IGcv0jUJrN0SH5cgYVLArwuBw5duu4Wn7sY0T1dx4vfQCc8lSYbONAEKQy8/NzHppvmQmVrmawIwKIIXWn+Z6I= X-Received: by 2002:a05:6102:9d8:b0:4b2:5c2a:cc9d with SMTP id ada2fe7eead31-4b2cc3919a0mr46108898137.16.1736145509570; Sun, 05 Jan 2025 22:38:29 -0800 (PST) MIME-Version: 1.0 References: <20231107062842.116670-1-rsingh@ventanamicro.com> <20231107062842.116670-3-rsingh@ventanamicro.com> In-Reply-To: From: "Ranbir Singh" Date: Mon, 6 Jan 2025 12:08:18 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 2/2] FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue To: Laszlo Ersek , Oliver Smith-Denny Cc: devel@edk2.groups.io, Ray Ni , Veeresh Sangolli Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Sun, 05 Jan 2025 22:38:30 -0800 Resent-From: rsingh@ventanamicro.com Reply-To: devel@edk2.groups.io,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="0000000000006071ed062b03e020" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=trMjZ6jh; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io --0000000000006071ed062b03e020 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Comment and code message update is doable with no effect on Patch's coverity status. So, this will be included in the final patch series. On Tue, Nov 7, 2023 at 11:09=E2=80=AFPM Laszlo Ersek wr= ote: > On 11/7/23 07:28, Ranbir Singh wrote: > > From: Ranbir Singh > > > > The function FatInitializeDiskCache evaluates an expression > > > > FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment > > > > and assigns it to DataCacheSize which is of type UINTN. > > > > As per Coverity report, > > FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment is a > > potentially overflowing expression with type "int" (32 bits, signed) > > evaluated using 32-bit arithmetic, and then used in a context that > > expects an expression of type "UINTN" (64 bits, unsigned). > > > > To avoid overflow, cast "FAT_DATACACHE_GROUP_COUNT" to type "UINTN". > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4249 > > > > Cc: Ray Ni > > Co-authored-by: Veeresh Sangolli > > Signed-off-by: Ranbir Singh > > Signed-off-by: Ranbir Singh > > --- > > FatPkg/EnhancedFatDxe/DiskCache.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c > b/FatPkg/EnhancedFatDxe/DiskCache.c > > index d1a34a6a646f..d56e338586d8 100644 > > --- a/FatPkg/EnhancedFatDxe/DiskCache.c > > +++ b/FatPkg/EnhancedFatDxe/DiskCache.c > > @@ -477,7 +477,7 @@ FatInitializeDiskCache ( > > DiskCache[CacheFat].BaseAddress =3D Volume->FatPos; > > DiskCache[CacheFat].LimitAddress =3D Volume->FatPos + Volume->FatSi= ze; > > FatCacheSize =3D FatCacheGroupCount << > DiskCache[CacheFat].PageAlignment; > > - DataCacheSize =3D FAT_DATACACHE_GROUP_COUNT << > DiskCache[CacheData].PageAlignment; > > + DataCacheSize =3D (UINTN)FAT_DATACACHE_GROUP_COU= NT > << DiskCache[CacheData].PageAlignment; > > // > > // Allocate the Fat Cache buffer > > // > > I don't understand Coverity here. This is the larger context (extract): > > // > // Configure the parameters of disk cache > // > if (Volume->FatType =3D=3D Fat12) { > DiskCache[CacheData].PageAlignment =3D FAT_DATACACHE_PAGE_MIN_ALIGNME= NT; > } else { > DiskCache[CacheData].PageAlignment =3D FAT_DATACACHE_PAGE_MAX_ALIGNME= NT; > } > > DataCacheSize =3D FAT_DATACACHE_GROUP_COUNT << > DiskCache[CacheData].PageAlignment; > > In practice, one of two things can happen: either > > DataCacheSize =3D 64 << 13; > > or > > DataCacheSize =3D 64 << 16; > > The larger value is 2 MB, it happily fits in an INT32. > > I don't mind the patch, but the commit message, and a new code comment as > well, should state that we're only casting to shut up Coverity. > > If the shift count "DiskCache[CacheData].PageAlignment" were *indeed* > unbounded, then we'd have much more serious problems. First, we could shi= ft > by 64 bits or more, which is undefined even if we cast to UINT64 at first= . > Second, even assuming "DataCacheSize" can be calculated correctly, just > below it we have > > CacheBuffer =3D AllocateZeroPool (FatCacheSize + DataCacheSize); > > where the *addition* can nicely overflow regardless. > > The patch is OK but it requires a comment, and a commit message update. > > Thanks > Laszlo > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#120958): https://edk2.groups.io/g/devel/message/120958 Mute This Topic: https://groups.io/mt/102438365/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --0000000000006071ed062b03e020 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Comment and code message update is doable with no effect o= n Patch's coverity status.

So, this will be included in the fina= l patch series.

<= div dir=3D"ltr" class=3D"gmail_attr">On Tue, Nov 7, 2023 at 11:09=E2=80=AFP= M Laszlo Ersek <lersek@redhat.com> wrote:
On= 11/7/23 07:28, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The function FatInitializeDiskCache evaluates an expression
>
>=C2=A0 =C2=A0 =C2=A0FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheD= ata].PageAlignment
>
> and assigns it to DataCacheSize which is of type UINTN.
>
> As per Coverity report,
> FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment = is a
> potentially overflowing expression with type "int" (32 bits,= signed)
> evaluated using 32-bit arithmetic, and then used in a context that
> expects an expression of type "UINTN" (64 bits, unsigned). >
> To avoid overflow, cast "FAT_DATACACHE_GROUP_COUNT" to type = "UINTN".
>
> REF: https://bugzilla.tianocore.org/show_b= ug.cgi?id=3D4249
>
> Cc: Ray Ni <r= ay.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>=C2=A0 FatPkg/EnhancedFatDxe/DiskCache.c | 2 +-
>=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c b/FatPkg/EnhancedFatDxe= /DiskCache.c
> index d1a34a6a646f..d56e338586d8 100644
> --- a/FatPkg/EnhancedFatDxe/DiskCache.c
> +++ b/FatPkg/EnhancedFatDxe/DiskCache.c
> @@ -477,7 +477,7 @@ FatInitializeDiskCache (
>=C2=A0 =C2=A0 DiskCache[CacheFat].BaseAddress=C2=A0 =C2=A0=3D Volume-&g= t;FatPos;
>=C2=A0 =C2=A0 DiskCache[CacheFat].LimitAddress=C2=A0 =3D Volume->Fat= Pos + Volume->FatSize;
>=C2=A0 =C2=A0 FatCacheSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D FatCacheGroupCount << DiskCache[C= acheFat].PageAlignment;
> -=C2=A0 DataCacheSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D FAT_DATACACHE_GROUP_COUNT << DiskCache= [CacheData].PageAlignment;
> +=C2=A0 DataCacheSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D (UINTN)FAT_DATACACHE_GROUP_COUNT << Di= skCache[CacheData].PageAlignment;
>=C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 // Allocate the Fat Cache buffer
>=C2=A0 =C2=A0 //

I don't understand Coverity here. This is the larger context (extract):=

=C2=A0 //
=C2=A0 // Configure the parameters of disk cache
=C2=A0 //
=C2=A0 if (Volume->FatType =3D=3D Fat12) {
=C2=A0 =C2=A0 DiskCache[CacheData].PageAlignment =3D FAT_DATACACHE_PAGE_MIN= _ALIGNMENT;
=C2=A0 } else {
=C2=A0 =C2=A0 DiskCache[CacheData].PageAlignment =3D FAT_DATACACHE_PAGE_MAX= _ALIGNMENT;
=C2=A0 }

=C2=A0 DataCacheSize=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0=3D FAT_DATACACHE_GROUP_COUNT << DiskCache[Cache= Data].PageAlignment;

In practice, one of two things can happen: either

=C2=A0 DataCacheSize =3D 64 << 13;

or

=C2=A0 DataCacheSize =3D 64 << 16;

The larger value is 2 MB, it happily fits in an INT32.

I don't mind the patch, but the commit message, and a new code comment = as well, should state that we're only casting to shut up Coverity.

If the shift count "DiskCache[CacheData].PageAlignment" were *ind= eed* unbounded, then we'd have much more serious problems. First, we co= uld shift by 64 bits or more, which is undefined even if we cast to UINT64 = at first. Second, even assuming "DataCacheSize" can be calculated= correctly, just below it we have

=C2=A0 CacheBuffer =3D AllocateZeroPool (FatCacheSize + DataCacheSize);

where the *addition* can nicely overflow regardless.

The patch is OK but it requires a comment, and a commit message update.

Thanks
Laszlo

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#120958) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--0000000000006071ed062b03e020--