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 <[email protected]> > Sent: Tuesday, October 24, 2023 8:03 PM > To: [email protected]; Xu, Wei6 <[email protected]> > Cc: Ard Biesheuvel <[email protected]>; Sami Mujawar > <[email protected]>; Ni, Ray <[email protected]> > 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 <[email protected]> >> Cc: Sami Mujawar <[email protected]> >> Cc: Ray Ni <[email protected]> >> Signed-off-by: Wei6 Xu <[email protected]> >> --- >> 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/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
