From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.103119.1674829468160884149 for ; Fri, 27 Jan 2023 06:24:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=Rdlmcxva; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 8BCA724054B for ; Fri, 27 Jan 2023 15:24:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1674829466; bh=+e9FibaE/nR3L8ijvYJ8vgHq/iXRaCfRhBLmiE7LU6g=; h=From:Subject:Date:Cc:To:From; b=RdlmcxvafpTd/LIGaTIgko8Fwh4NVKg5QM8IL8tB1i1INB/sPov5S69PA2tYMkSC5 kIWARTvE6EDjk1zs+M09fcZJD3Z7SFp1qcjQuuaZnFIOhUzKJhvy2FpVVZ2ovIOekW xOAZAImI8hv341bKNaO+Y/pWcgm6wUaYh86GNFb3lZfSjj8QiW+BCpHpzcK/oV5oik UwiAUf51Jz5fprrN5f4+KGEUdnGdGioRkDYa6pxEAQzGL3wevKf0vj4DrFFTMmoP/g DtCK7EBVT8RGSIn5Rq4YKyAjlavjcGgnBjFPa4T5hzWVI8JhisaFNcJO4/ig/4O/a6 7Lp5beJoouyZA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4P3KZx6Mrzz9rxN; Fri, 27 Jan 2023 15:24:25 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\)) Subject: Re: [edk2-platforms][PATCH v3 05/11] Ext4Pkg: Fix shift out of bounds in Ext4OpenSuperblock Date: Fri, 27 Jan 2023 14:24:15 +0000 In-Reply-To: Cc: Savva Mitrofanov , devel@edk2.groups.io, Vitaly Cheptsov To: Pedro Falcato References: <20230127092945.94389-1-savvamtr@gmail.com> <20230127092945.94389-6-savvamtr@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_3254B222-EF45-4531-A783-EE3CE19E2D23" --Apple-Mail=_3254B222-EF45-4531-A783-EE3CE19E2D23 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Reviewed-by: Marvin H=C3=A4user > On 27. Jan 2023, at 15:22, Pedro Falcato = wrote: >=20 > On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov > wrote: >>=20 >> Missing check for wrong s_log_block_size exponent leads to shift out = of >> bounds. Limit block size to 2 MiB >>=20 >> Cc: Marvin H=C3=A4user >> Cc: Pedro Falcato >> Cc: Vitaly Cheptsov >> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.") >> Signed-off-by: Savva Mitrofanov >> --- >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 ++++++++++++++ >> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 5 +++++ >> 2 files changed, 19 insertions(+) >>=20 >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> index 2e489ce4dd86..a23323319a59 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> @@ -40,6 +40,20 @@ >> #define EXT4_EFI_PATH_MAX 4096 >> #define EXT4_DRIVER_VERSION 0x0000 >>=20 >> +// >> +// The EXT4 Specification doesn't strictly limit block size and this = value could be up to 2^31, >> +// but in practice it is limited by PAGE_SIZE due to performance = significant impact. >> +// Many EXT4 implementations have size of block limited to = PAGE_SIZE. In many cases it's limited >> +// to 4096, which is a commonly supported page size on most = MMU-capable hardware, and up to 65536. >> +// So, to take a balance between compatibility and security = measures, it is decided to use the >> +// value of 2MiB as the limit, which is equal to page size on new = hardware. > Nit: s/page size/large page size/ > I can change this for you when pushing, no need for a v4 on this one. >> +// As for supporting big block sizes, EXT4 has a RO_COMPAT_FEATURE = called BIGALLOC, which changes >> +// EXT4 to use clustered allocation, so that each bit in the ext4 = block allocation bitmap addresses >> +// a power of two number of blocks. So it would be wiser to = implement and use this feature >> +// if there is such a need instead of big block size. >> +// >> +#define EXT4_LOG_BLOCK_SIZE_MAX 11 >> + >> /** >> Opens an ext4 partition and installs the Simple File System = protocol. >>=20 >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c = b/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> index be3527e4d618..3f56de93c105 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> @@ -248,6 +248,11 @@ Ext4OpenSuperblock ( >> return EFI_VOLUME_CORRUPTED; >> } >>=20 >> + if (Sb->s_log_block_size > EXT4_LOG_BLOCK_SIZE_MAX) { >> + DEBUG ((DEBUG_ERROR, "[ext4] SuperBlock s_log_block_size %lu is = too big\n", Sb->s_log_block_size)); >> + return EFI_UNSUPPORTED; >> + } >> + >> Partition->BlockSize =3D (UINT32)LShiftU64 (1024, = Sb->s_log_block_size); >>=20 >> // The size of a block group can also be calculated as 8 * = Partition->BlockSize >> -- >> 2.39.0 >>=20 >=20 > Reviewed-by: Pedro Falcato > >=20 > --=20 > Pedro --Apple-Mail=_3254B222-EF45-4531-A783-EE3CE19E2D23 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Reviewed-by: = Marvin H=C3=A4user <mhaeuser@posteo.de>

On 27. Jan 2023, at 15:22, Pedro Falcato = <pedro.falcato@gmail.com> wrote:

On Fri, Jan 27, 2023 at 9:29 AM Savva = Mitrofanov <savvamtr@gmail.com> = wrote:

Missing = check for wrong s_log_block_size exponent leads to shift out = of
bounds. Limit block size to 2 MiB

Cc: Marvin H=C3=A4user = <mhaeuser@posteo.de>
Cc: Pedro Falcato = <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov = <vit9696@protonmail.com>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add = Ext4Dxe driver.")
Signed-off-by: Savva Mitrofanov = <savvamtr@gmail.com>
---
Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h =    | 14 = ++++++++++++++
Features/Ext4Pkg/Ext4Dxe/Superblock.c |  5 = +++++
2 files changed, 19 insertions(+)

diff --git = a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 2e489ce4dd86..a23323319a59 = 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ = b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -40,6 +40,20 @@
#define = EXT4_EFI_PATH_MAX    4096
#define EXT4_DRIVER_VERSION =  0x0000

+//
+// The EXT4 Specification doesn't strictly = limit block size and this value could be up to 2^31,
+// but in = practice it is limited by PAGE_SIZE due to performance significant = impact.
+// Many EXT4 implementations have size of block limited to = PAGE_SIZE. In many cases it's limited
+// to 4096, which is a = commonly supported page size on most MMU-capable hardware, and up to = 65536.
+// So, to take a balance between compatibility and security = measures, it is decided to use the
+// value of 2MiB as the limit, = which is equal to page size on new hardware.
Nit: s/page size/large page size/
I can change this for you when pushing, no = need for a v4 on this one.
+// As for = supporting big block sizes, EXT4 has a RO_COMPAT_FEATURE called = BIGALLOC, which changes
+// EXT4 to use clustered allocation, so that = each bit in the ext4 block allocation bitmap addresses
+// a power of = two number of blocks. So it would be wiser to implement and use this = feature
+// if there is such a need instead of big block = size.
+//
+#define EXT4_LOG_BLOCK_SIZE_MAX =  11
+
/**
   Opens an ext4 partition and = installs the Simple File System protocol.

diff --git = a/Features/Ext4Pkg/Ext4Dxe/Superblock.c = b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index = be3527e4d618..3f56de93c105 100644
--- = a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ = b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -248,6 +248,11 @@ = Ext4OpenSuperblock (
    return = EFI_VOLUME_CORRUPTED;
  }

+  if = (Sb->s_log_block_size > EXT4_LOG_BLOCK_SIZE_MAX) {
+ =    DEBUG ((DEBUG_ERROR, "[ext4] SuperBlock = s_log_block_size %lu is too big\n", Sb->s_log_block_size));
+ =    return EFI_UNSUPPORTED;
+ =  }
+
  Partition->BlockSize =3D = (UINT32)LShiftU64 (1024, Sb->s_log_block_size);

  // = The size of a block group can also be calculated as 8 * = Partition->BlockSize
--
2.39.0


Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro

= --Apple-Mail=_3254B222-EF45-4531-A783-EE3CE19E2D23--