Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg: Fix the failure to find uncompressed inner FV.

["Laszlo Ersek" <lersek@redhat.com>]

On 10/27/23 04:21, Xu, Wei6 wrote:
> Hi Laszlo,
> 
> Thanks a lot for the review.
> 
> I send review the patch v2 to fix:
> - memory leaks on error paths
> - missing object size checks before casting pointers to header types
> (https://edk2.groups.io/g/devel/message/110160)

Thanks, will check it.

> 
> Regarding to 'unbounded recursion', I couldn't come up with a good solution to fix the problem, let's fix the others first.

We've had the same issue in both the PEI Core and the DXE Core.

The PEI core issue was CVE-2018-12183. We have two TianoCore BZs related
to that, #1137 and #1126. I don't know / remember how the issue was
ultimately fixed. Presumably with the PEI Stack Guard. I don't think
that's a great solution, but either way, the issue seems hardly
exploitable (because it's arguably not easy for an attacker to inject
FVs in the PEI phase).

The DXE Core issue was CVE-2021-28210 -- TianoCore BZ#1743. The fix for
that was commit range 6c8dd15c4ae4..47343af30435. We introduced
PcdFwVolDxeMaxEncapsulationDepth to arbitrarily limit the depth of
recursion. It's a practical fix. I think the same approach could be
taken in the Standalone MM Core as well.

Laszlo

> 
> 
> BR,
> Wei
> 
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com> 
> Sent: Tuesday, October 24, 2023 8:03 PM
> To: devel@edk2.groups.io; Xu, Wei6 <wei6.xu@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] StandaloneMmPkg: Fix the failure to find uncompressed inner FV.
> 
> On 10/24/23 07:53, Xu, Wei6 wrote:
>> The MmCoreFfsFindMmDriver only checks for encapsulated compressed FVs.
>> When an inner FV is uncompressed, StandaloneMmCore will miss the FV 
>> and all the MM drivers in the FV will not be dispatched.
>> Add checks for uncompressed inner FV to fix this issue.
>>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
>> ---
>>  StandaloneMmPkg/Core/FwVol.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/StandaloneMmPkg/Core/FwVol.c 
>> b/StandaloneMmPkg/Core/FwVol.c index 1f6d7714ba97..1a85d80eb9f7 100644
>> --- a/StandaloneMmPkg/Core/FwVol.c
>> +++ b/StandaloneMmPkg/Core/FwVol.c
>> @@ -104,6 +104,17 @@ MmCoreFfsFindMmDriver (
>>        break;
>>      }
>>  
>> +    Status = FfsFindSectionData (
>> +               EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
>> +               FileHeader,
>> +               &SectionData,
>> +               &SectionDataSize
>> +               );
>> +    if (!EFI_ERROR (Status)) {
>> +      InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)SectionData;
>> +      MmCoreFfsFindMmDriver (InnerFvHeader);
>> +    }
>> +
>>      Status = FfsFindSectionData (
>>                 EFI_SECTION_GUID_DEFINED,
>>                 FileHeader,
> 
> I'd recommend fixing other, more foundational issues first, in this function, such as:
> 
> - memory leaks on error paths
> 
> - unbounded recursion
> 
> - missing object size checks before casting pointers to header types
> 
> At the same time I agree that this change doesn't seem to make things worse than they are.
> 
> Laszlo
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110240): https://edk2.groups.io/g/devel/message/110240
Mute This Topic: https://groups.io/mt/102152694/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-