Re: [edk2-devel] [edk2-platforms][PATCH v3 08/11] Ext4Pkg: Check VolumeName allocation correctness in Ext4GetVolumeName

["Marvin Häuser" <mhaeuser@posteo.de>]

This code makes me wish for an in-place conversion lib, there really is no reason for dynamic allocation here...

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

> On 27. Jan 2023, at 15:27, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com> wrote:
>> 
>> Missing check in some cases leads to failed StrCpyS call in
>> Ext4GetVolumeLabelInfo. Also correct condition that checks Inode pointer
>> for being NULL in Ext4AllocateInode
>> 
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Fixes: cfbbae595eec ("Ext4Pkg: Add handling of EFI_FILE_SYSTEM_VOLUME_LABEL GetInfo().")
>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>> ---
>> Features/Ext4Pkg/Ext4Dxe/File.c  | 10 ++++++++--
>> Features/Ext4Pkg/Ext4Dxe/Inode.c |  2 +-
>> 2 files changed, 9 insertions(+), 3 deletions(-)
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
>> index 9dde4a5d1a2d..677caf88fbdc 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/File.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/File.c
>> @@ -719,7 +719,11 @@ Ext4GetVolumeName (
>> 
>> 
>>     VolNameLength = StrLen (VolumeName);
>> 
>>   } else {
>> 
>> -    VolumeName    = AllocateZeroPool (sizeof (CHAR16));
>> 
>> +    VolumeName = AllocateZeroPool (sizeof (CHAR16));
>> 
>> +    if (VolumeName == NULL) {
>> 
>> +      return EFI_OUT_OF_RESOURCES;
>> 
>> +    }
>> 
>> +
>> 
>>     VolNameLength = 0;
>> 
>>   }
>> 
>> 
>> 
>> @@ -786,7 +790,9 @@ Ext4GetFilesystemInfo (
>>   Info->VolumeSize = MultU64x32 (TotalBlocks, Part->BlockSize);
>> 
>>   Info->FreeSpace  = MultU64x32 (FreeBlocks, Part->BlockSize);
>> 
>> 
>> 
>> -  StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
>> 
>> +  Status = StrCpyS (Info->VolumeLabel, VolNameLength + 1, VolumeName);
>> 
>> +
>> 
>> +  ASSERT_EFI_ERROR (Status);
>> 
>> 
>> 
>>   FreePool (VolumeName);
>> 
>> 
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> index e44b5638599f..90e3eb88f523 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> @@ -230,7 +230,7 @@ Ext4AllocateInode (
>> 
>> 
>>   Inode = AllocateZeroPool (InodeSize);
>> 
>> 
>> 
>> -  if (!Inode) {
>> 
>> +  if (Inode == NULL) {
>> 
>>     return NULL;
>> 
>>   }
>> 
>> 
>> 
>> --
>> 2.39.0
>> 
> 
> Embarrassing...
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> 
> -- 
> Pedro