Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

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

On 03/12/20 11:40, Leif Lindholm wrote:
> On Thu, Mar 12, 2020 at 01:30:10 +0100, Laszlo Ersek wrote:
>> On 03/11/20 20:36, Leif Lindholm wrote:
>>> On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote:
>>>>>> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
>>>>>> +  { EfiACPIMemoryNVS,       0x004 },
>>>>>> +  { EfiACPIReclaimMemory,   0x008 },
>>>>>> +  { EfiReservedMemoryType,  0x004 },
>>>>>> +  { EfiRuntimeServicesData, 0x024 },
>>>>>> +  { EfiRuntimeServicesCode, 0x030 },
>>>>>> +  { EfiBootServicesCode,    0x180 },
>>>>>> +  { EfiBootServicesData,    0xF00 },
>>>>>> +  { EfiMaxMemoryType,       0x000 }
>>>>>> +};
>>>>>
>>>>> Could you add a comment as to where these page counts come from?
>>>>> Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c.
>>>>>
>>>>> It *would* be nice if that could be cleaned up at the same time...
>>>>
>>>> Sorry, I don't understand -- what kind of cleanup do you have in mind
>>>> precisely? The table is not copied, but moved from the original place,
>>>> so I'm unsure what's left in "Platform.c" to clean up.
>>>
>>> Not left to clean up there, but something to consider addressing when
>>> moving it here. Yes, it's just a move, so we could argue it doesn't
>>> need fixing - but it's a struct with a bunch of live-coded numerical
>>> values completely without explanation.
>>>
>>> "I'd rather not" is an acceptable answer, but I figured I should point
>>> it out.
>>
>> Good point!
>>
>> Yet, I'd rather not :) Long read ahead:
>>
>> This table is used for priming the memory type BINs in the DXE Core.
>> After this set, in non-SMM builds, the functionality will remain the
>> same (the table stays in effect for every boot); in SMM builds, the
>> table is only a starting point for the feedback loop.
>>
>> What's important is that the numbers in the table are entirely ad-hoc.
>> They were last updated in commit 991d95636264, in 2010. They capture a
>> set of BIN hints that made sense at the time, for *some* (now unknown)
>> workloads / boot paths. That's the important trait of this table: it
>> made sense at some point in time, for some use case. That's the property
>> we should not regress.
>>
>> So let's consider the possible ways to improve the table.
> 
> There is fixing and there is improving.
> 
> The preceding paragraph as a comment block would prevent the next
> person who comes across it going "Where the $EXPLETIVE did these
> numbers come from?".
> 
> (Adding the preceding paragraph as well would have saved me another
> minute of grepping, but that is more down to the fact that this is a
> repeating pattern implemented differently for different platforms -
> for most ARM platforms partly hidden away in EmbeddedPkg:
> https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/EmbeddedPkg.dec#L104)
> 
>> (1) Given that in SMM builds, the table will function only as a starting
>> point for the feedback loop, start using two tables. A new one (for the
>> SMM build) with nice numbers (everything 0x1, or everyting 0x1000,
>> whatever), and keep the old one for the non-SMM build.
>>
>> Unfortunately, this "improvement" is a net negative, because then we'd
>> have *more* stuff, on top of the *current* dump-of-obscure-numbers.
>>
>> (2) Keep the one table, but replace the values with nicer looking
>> numbers (see examples above). Unfortunately, larger numbers could waste
>> memory (stuck in BINs and hence in the UEFI memmap) for some boots, and
>> smaller page counts would increase memmap fragmentation.
>>
>> We might cause some (at best, superficial) regressions. And we'd lose
>> the property "this table made sense at some point in time" -- because
>> the new ad-hoc numbers wouldn't even be rooted in measurements.
>>
>> (3) Actually measure various boots and try to derive new page counts
>> from those.
>>
>> This is something I'm not prepared to do. The memory needs (considering
>> the various memory types too), depend on a bunch of stuff:
>>
>> - ACPI tables generated by QEMU (influences AcpiNVS, AcpiReclaim, and
>> even Reserved -- some opcodes in QEMU's ACPI linker/loader script
>> require the production of S3 boot script opcodes). For example if the
>> QEMU command line specifies the vmgenid device, then the S3 boot script
>> stuff applies.
>>
>> - S3 support enabled/disabled in general on the QEMU cmdline.
>>
>> - OS bootloader footprint.
> 
> - Separately loaded drivers (including Option ROMs?), making these
>   numers impossible to precisely determine statically.
> 
>> This approach would uphold the property "has been useful at some point
>> in time, for some workloads" -- but it's too much time to research, and
>> it's anyway obviated by the dynamic / adaptive approach that this series
>> enables for OVMF (in the SMM build).
>>
>> (4) OK, so let's not touch the numeric values, but move them out of the
>> table?
>>
>> (4a) Introduce macros.
>>
>> Not a good idea, as these numbers are never referenced anywhere else.
>> The following:
>>
>> #define MTI_RT_DATA_PAGE_COUNT 0x024
>> ...
>>   { EfiRuntimeServicesData, MTI_RT_DATA_PAGE_COUNT },
>>
>> is not one bit more readable or expressive, but is more wasteful, than
>> the current
>>
>>   { EfiRuntimeServicesData, 0x024 },
>>
>> (4b) Introduce PCDs.
>>
>> Even worse: it elevates these ad-hoc numbers to the OvmfPkg.dec file,
>> without any plan or intent whatsoever to ever update the constants, or
>> to reference them in any other modules, or to override them in any of
>> the locations where PCDs can be overridden (DSC file, FDF file, and for
>> dynamic access PCDs: C code).
> 
> See EmbeddedPkg.
> 
> Basically, all of the variants you enumerate exist in the tree(s)
> today.
> 
>> These numbers are basically a state-dump from a time when they had been
>> found somewhat useful. They still work acceptably, and without an
>> interest in (3), I wouldn't like to touch them with a ten foot pole. :)
> 
> Sure.
> 
> So what I'm *actually* after is.
> 
> (5) *Somehow* standardise how platforms build up the HOB
> 
> I think this means *hiding* BuildGuidDataHob() behind some
> higher-level functions, backed by some market-segment (or
> market-segment:architecture tuple) specific defaults.
> 
> 
> But for this patch, if you could add the archaeology bit in a comment
> block, I think that would be useful for whatever next lost soul
> stumbles upon it.
> 
> With or without that, for the series:
> Acked-by: Leif Lindholm <leif@nuviainc.com>
> 

Merged as-is, in commit range 7d325f93e190..d42fdd6f8384, via
<https://github.com/tianocore/edk2/pull/442>.

I am going to submit a separate patch with the suggested comment.

Thank you!
Laszlo