Hi Marvin,
Thanks for comments!
> On 20 Jul 2022, at 23:54, Marvin Häuser <mhaeu...@posteo.de> wrote:
>
> On 20. Jul 2022, at 07:36, Savva Mitrofanov <savva...@gmail.com> wrote:
>>
>> This changes tends to improve security of code sections by fixing
>> integer overflows, missing aligment checks, unsafe casts, also
>> simplified some routines, fixed compiler warnings and corrected some
>> code mistakes.
>>
>> - Set HoleLen to UINT64 to perform safe cast to UINTN in ternary
>> operator at WasRead assignment.
>
> In my opinion, just "to prevent truncation".
>
>> - Replace EXT4_BLOCK_NR with 32-bit EXT2_BLOCK_NR in BlockMap, because
>> we consider BlockMap is 32-bit fs ext2/3 feature.
>> - Replace UNREACHABLE with ASSERT (FALSE) in case of new checksum
>> algorithms, due to it is an invariant violation rather than unreachable
>> path.
>> - Solve compiler warnings. Init all fields in gExt4BindingProtocol.
>> Fix comparison of integer expressions of different signedness.
>> - Field name_len has type CHAR8, while filename limit is 255
>> (EXT4_NAME_MAX), so because structure EXT4_DIR_ENTRY would be
>> unchangeable in future, we could drop this check without any
>> assertions
>> - Simplify Ext4RemoveDentry logic by using IsNodeInList
>> - Fix possible int overflow in Ext4ExtentsMapKeyCompare
>> - Return bad block type in Ext4GetBlockpath
>> - Adds 4-byte aligned check for superblock group descriptor size field
>>
>> Cc: Marvin Häuser <mhaeu...@posteo.de>
>> Cc: Pedro Falcato <pedro.falc...@gmail.com>
>> Cc: Vitaly Cheptsov <vit9...@protonmail.com>
>> Signed-off-by: Savva Mitrofanov <savva...@gmail.com>
>> ---
>> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 3 +-
>> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 2 +-
>> Features/Ext4Pkg/Ext4Dxe/BlockMap.c | 18 ++++++++----
>> Features/Ext4Pkg/Ext4Dxe/Directory.c | 29 ++------------------
>> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 10 ++++---
>> Features/Ext4Pkg/Ext4Dxe/Extents.c | 5 ++--
>> Features/Ext4Pkg/Ext4Dxe/Inode.c | 8 +++---
>> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 13 +++++----
>> 8 files changed, 38 insertions(+), 50 deletions(-)
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> index a55cd2fa68ad..7a19d2f79d53 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>> @@ -338,7 +338,7 @@ STATIC_ASSERT (
>> #define EXT4_TIND_BLOCK 14
>> #define EXT4_NR_BLOCKS 15
>>
>> -#define EXT4_GOOD_OLD_INODE_SIZE 128
>> +#define EXT4_GOOD_OLD_INODE_SIZE 128U
>>
>> typedef struct _Ext4_I_OSD2_Linux {
>> UINT16 l_i_blocks_high;
>> @@ -463,6 +463,7 @@ typedef struct {
>> #define EXT4_EXTENT_MAX_INITIALIZED (1 << 15)
>>
>> typedef UINT64 EXT4_BLOCK_NR;
>> +typedef UINT32 EXT2_BLOCK_NR;
>> typedef UINT32 EXT4_INO_NR;
>>
>> // 2 is always the root inode number in ext4
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> index b1508482b0a7..b446488b2112 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> @@ -1165,7 +1165,7 @@ EFI_STATUS
>> Ext4GetBlocks (
>> IN EXT4_PARTITION *Partition,
>> IN EXT4_FILE *File,
>> - IN EXT4_BLOCK_NR LogicalBlock,
>> + IN EXT2_BLOCK_NR LogicalBlock,
>
> This looks awkward, using an "EXT2" type in an "Ext4" function. Maybe just
> use UINT32?
The idea to use such type naming belongs to Pedro, and in recent discussions he
insisted on this, due EXT2_BLOCK_NR is more explicit than using UINT32 directly.
So, I'll keep it in v3.
>
>> OUT EXT4_EXTENT *Extent
>> );
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
>> b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
>> index 1a06ac9fbf86..2bc629fe9d38 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/BlockMap.c
>> @@ -70,7 +70,7 @@ UINTN
>> Ext4GetBlockPath (
>> IN CONST EXT4_PARTITION *Partition,
>> IN UINT32 LogicalBlock,
>> - OUT EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]
>> + OUT EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH]
>> )
>> {
>> // The logic behind the block map is very much like a page table
>> @@ -123,7 +123,7 @@ Ext4GetBlockPath (
>> break;
>> default:
>> // EXT4_TYPE_BAD_BLOCK
>> - return -1;
>> + break;
>> }
>>
>> return Type + 1;
>> @@ -213,12 +213,12 @@ EFI_STATUS
>> Ext4GetBlocks (
>> IN EXT4_PARTITION *Partition,
>> IN EXT4_FILE *File,
>> - IN EXT4_BLOCK_NR LogicalBlock,
>> + IN EXT2_BLOCK_NR LogicalBlock,
>> OUT EXT4_EXTENT *Extent
>> )
>> {
>> EXT4_INODE *Inode;
>> - EXT4_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH];
>> + EXT2_BLOCK_NR BlockPath[EXT4_MAX_BLOCK_PATH];
>> UINTN BlockPathLength;
>> UINTN Index;
>> UINT32 *Buffer;
>> @@ -230,7 +230,7 @@ Ext4GetBlocks (
>>
>> BlockPathLength = Ext4GetBlockPath (Partition, LogicalBlock, BlockPath);
>>
>> - if (BlockPathLength == (UINTN)-1) {
>> + if (BlockPathLength - 1 == EXT4_TYPE_BAD_BLOCK) {
>> // Bad logical block (out of range)
>> return EFI_NO_MAPPING;
>> }
>> @@ -272,7 +272,13 @@ Ext4GetBlocks (
>> }
>> }
>>
>> - Ext4GetExtentInBlockMap (Buffer, Partition->BlockSize / sizeof (UINT32),
>> BlockPath[BlockPathLength - 1], Extent);
>> + Ext4GetExtentInBlockMap (
>> + Buffer,
>> + Partition->BlockSize / sizeof (UINT32),
>> + BlockPath[BlockPathLength - 1],
>> + Extent
>> + );
>> +
>> FreePool (Buffer);
>>
>> return EFI_SUCCESS;
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> index 682f66ad5525..4441e6d192b6 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
>> @@ -74,7 +74,7 @@ Ext4ValidDirent (
>> }
>>
>> // Dirent sizes need to be 4 byte aligned
>> - if (Dirent->rec_len % 4) {
>> + if ((Dirent->rec_len % 4) != 0) {
>> return FALSE;
>> }
>>
>> @@ -160,17 +160,6 @@ Ext4RetrieveDirent (
>> return EFI_VOLUME_CORRUPTED;
>> }
>>
>> - // Ignore names bigger than our limit.
>> -
>> - /* Note: I think having a limit is sane because:
>> - 1) It's nicer to work with.
>> - 2) Linux and a number of BSDs also have a filename limit of 255.
>> - */
>> - if (Entry->name_len > EXT4_NAME_MAX) {
>> - BlockOffset += Entry->rec_len;
>> - continue;
>> - }
>> -
>> // Unused entry
>> if (Entry->inode == 0) {
>> BlockOffset += Entry->rec_len;
>> @@ -548,20 +537,8 @@ Ext4RemoveDentry (
>> IN OUT EXT4_DENTRY *ToBeRemoved
>> )
>> {
>> - EXT4_DENTRY *D;
>> - LIST_ENTRY *Entry;
>> - LIST_ENTRY *NextEntry;
>> -
>> - BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children) {
>> - D = EXT4_DENTRY_FROM_DENTRY_LIST (Entry);
>> -
>> - if (D == ToBeRemoved) {
>> - RemoveEntryList (Entry);
>> - return;
>> - }
>> - }
>> -
>> - DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the asked-for
>> dentry\n"));
>> + ASSERT (IsNodeInList (&ToBeRemoved->ListNode, &Parent->Children));
>> + RemoveEntryList (&ToBeRemoved->ListNode);
>> }
>>
>> /**
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> index 43b9340d3956..2a4f5a7bd0ef 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> @@ -260,10 +260,12 @@ Ext4Stop (
>>
>> EFI_DRIVER_BINDING_PROTOCOL gExt4BindingProtocol =
>> {
>> - Ext4IsBindingSupported,
>> - Ext4Bind,
>> - Ext4Stop,
>> - EXT4_DRIVER_VERSION
>> + .Supported = Ext4IsBindingSupported,
>> + .Start = Ext4Bind,
>> + .Stop = Ext4Stop,
>> + .Version = EXT4_DRIVER_VERSION,
>> + .ImageHandle = NULL,
>> + .DriverBindingHandle = NULL
>> };
>>
>> /**
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c
>> b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>> index c3874df71751..d9ff69f21c14 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>> @@ -259,7 +259,8 @@ Ext4GetExtent (
>>
>> if (!(Inode->i_flags & EXT4_EXTENTS_FL)) {
>> // If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent using
>> the block map
>> - Status = Ext4GetBlocks (Partition, File, LogicalBlock, Extent);
>> + // We cast LogicalBlock to UINT32, considering ext2/3 are 32-bit
>
> The comment is misleading, because this is safe for EXT4 as well.
As we discussed, in 64-bit fs we have 2^64 maximum blocks. But if we dig
deeper, we find that files which are not using extents, but using blockmaps
must be placed within the first 2^32 blocks of the filesystem by the spec, so
in this case it is safe, and the comment about the 32-bit nature of ext2/3 is
improper here.
We should rewrite it like this:
// If this is an older ext2/ext3 filesystem, emulate Ext4GetExtent using
the block map
// By specification files using block maps must be placed within the first
2^32 blocks
// of a filesystem, so we can safely cast LogicalBlock to uint32
>
>> + Status = Ext4GetBlocks (Partition, File, (UINT32)LogicalBlock, Extent);
>>
>> if (!EFI_ERROR (Status)) {
>> Ext4CacheExtents (File, Extent, 1);
>> @@ -420,7 +421,7 @@ Ext4ExtentsMapKeyCompare (
>> Extent = UserStruct;
>> Block = (UINT32)(UINTN)StandaloneKey;
>>
>> - if ((Block >= Extent->ee_block) && (Block < Extent->ee_block +
>> Ext4GetExtentLength (Extent))) {
>> + if ((Block >= Extent->ee_block) && (Block - Extent->ee_block <
>> Ext4GetExtentLength (Extent))) {
>> return 0;
>> }
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> index 831f5946e870..4860cf576377 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> @@ -100,7 +100,7 @@ Ext4Read (
>> EFI_STATUS Status;
>> BOOLEAN HasBackingExtent;
>> UINT32 HoleOff;
>> - UINTN HoleLen;
>> + UINT64 HoleLen;
>> UINT64 ExtentStartBytes;
>> UINT64 ExtentLengthBytes;
>> UINT64 ExtentLogicalBytes;
>> @@ -155,10 +155,10 @@ Ext4Read (
>> HoleLen = (Ext4GetExtentLength (&Extent) * Partition->BlockSize) -
>> HoleOff;
>> }
>>
>> - WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
>> + WasRead = HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleLen;
>> // Potential improvement: In the future, we could get the file hole's
>> total
>> // size and memset all that
>> - SetMem (Buffer, WasRead, 0);
>> + ZeroMem (Buffer, WasRead);
>> } else {
>> ExtentStartBytes = MultU64x32 (
>> LShiftU64 (Extent.ee_start_hi, 32) |
>> @@ -431,7 +431,7 @@ Ext4FileCreateTime (
>> Inode = File->Inode;
>>
>> if (!EXT4_INODE_HAS_FIELD (Inode, i_crtime)) {
>> - SetMem (Time, sizeof (EFI_TIME), 0);
>> + ZeroMem (Time, sizeof (EFI_TIME));
>> return;
>> }
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> index 47fc3a65507a..a57728a9abe6 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> @@ -257,16 +257,17 @@ Ext4OpenSuperblock (
>> ));
>>
>> if (EXT4_IS_64_BIT (Partition)) {
>> + // s_desc_size should be 4 byte aligned and
>> + // 64 bit filesystems need DescSize to be 64 bytes
>> + if (((Sb->s_desc_size % 4) != 0) || (Sb->s_desc_size <
>> EXT4_64BIT_BLOCK_DESC_SIZE)) {
>> + return EFI_VOLUME_CORRUPTED;
>> + }
>> +
>> Partition->DescSize = Sb->s_desc_size;
>> } else {
>> Partition->DescSize = EXT4_OLD_BLOCK_DESC_SIZE;
>> }
>>
>> - if ((Partition->DescSize < EXT4_64BIT_BLOCK_DESC_SIZE) && EXT4_IS_64_BIT
>> (Partition)) {
>> - // 64 bit filesystems need DescSize to be 64 bytes
>> - return EFI_VOLUME_CORRUPTED;
>> - }
>> -
>> if (!Ext4VerifySuperblockChecksum (Partition, Sb)) {
>> DEBUG ((DEBUG_ERROR, "[ext4] Bad superblock checksum %lx\n",
>> Ext4CalculateSuperblockChecksum (Partition, Sb)));
>> return EFI_VOLUME_CORRUPTED;
>> @@ -342,7 +343,7 @@ Ext4CalculateChecksum (
>> // For some reason, EXT4 really likes non-inverted CRC32C checksums, so
>> we stick to that here.
>> return ~CalculateCrc32c(Buffer, Length, ~InitialValue);
>> default:
>> - UNREACHABLE ();
>> + ASSERT (FALSE);
>> return 0;
>> }
>> }
>> --
>> 2.37.0
>>
>
I already took into account all the remarks in v3, and will send it soon.
Best regards,
Savva Mitrofanov
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91969): https://edk2.groups.io/g/devel/message/91969
Mute This Topic: https://groups.io/mt/92531474/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
