Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek <lersek@redhat.com>]

On 04/30/17 02:48, Jordan Justen wrote:
> On 2017-04-29 13:14:59, Laszlo Ersek wrote:
>> The "Confirm64KilobytesOfUnauthenticatedVariableStorage" test case of the
>> Secure Boot Logo Test ("Microsoft.UefiSecureBootLogo.Tests") suite in the
>> Microsoft Hardware Certification Kit expects to be able to populate the
>> variable store up to roughly 64 KB, with a series of 1 KB sized,
>> unauthenticated variables. OVMF's current live varstore area is too small
>> for this: 56 KB.
>>
>> Introduce the FD_SIZE_4MB build macro, which
>>
>> - enlarges the full flash image to 4MB -- QEMU supports up to 8MB, see
>>   FLASH_MAP_BASE_MIN in "hw/i386/pc_sysfw.c" --,
>>
>> - inside that, grows the varstore area / pflash chip to 512 KB, and within
>>   it, the live area from 56 KB to 248 KB.
>>
>> (For the latter, Hyper-V considers 128 KB generous:
> 
> Today, we give 128k to non-volatile storage, which due to various
> reasons results in 56k of variable storage. Personally I already
> consider this generous, especially when I look at the 128k size of the
> seabios bios.bin. :)

I don't consider our current state generous. In a default setup /
minimal enrollment that is just enough to pass the Secure Boot Logo
Test, the live area usage is approx 0x4300 bytes (~17 KB). MokManager /
shim use a separate set of variables for storing certificate material.
If you enable the efi-pstore backend in Linux, to save crash messages in
efi variables, that can take up a further 10+ KB. DBX is only growing,
never shrinking. I wouldn't like to run into the same issue in a few
years with, say, RHEL-7.9.

> 
>>   Re: [edk2] Secure Boot & NV storage size
>>   http://mid.mail-archive.com/24f63595e68c476eb98cf87e7abfa1bc@BL2PR03MB242.namprd03.prod.outlook.com
>>
>> so we should be larger than that, for future proofing.)
> 
> I don't want to try to predict the future of Microsoft requirements,
> nor be overly generous in the meantime. :)

What's wrong with providing more, and being more robust in the face of
increased future demands?

> As pointed out by a
> colleague, as of Fall 2016, Microsoft expects 120 KB as the current
> requirement:
> 
> http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf

Slide 8 also says "there is no maximum NVRAM storage limit".

> 
> I tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:

RELEASE builds of virtual UEFI firmware are unsupportable in an
enterprise distribution. On tenths of occasions I've been able to
analyze OVMF and ArmVirtQemu error reports from:
- failed ASSERTs, and
- DEBUG messages
that would have been all lost in a RELEASE build.

QEMU (even upstream QEMU) rejects being built with NDEBUG; they consider
the asserts so important. For example, in "hw/virtio/virtio.c":

#ifdef NDEBUG
#error building with NDEBUG is not supported
#endif

See also this recent thread:

http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05499.html

Edk2 in particular still uses ASSERT in a bunch of places for error
checking.

> 
>  * SECURE_BOOT_ENABLE=1
>  * SMM_REQUIRE=1
> 
> There was > 600k available.

Do you mean in FVMAIN_COMPACT?

> 
> I also tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:
> 
>  * SECURE_BOOT_ENABLE=1
>  * NETWORK_IP6_ENABLE=1
>  * HTTP_BOOT_ENABLE=1
>  * SMM_REQUIRE=1
>  * TLS_ENABLE=1
> 
> I don't think we consider those network items as standard
> requirements, yet there was still ~373k available. In order to bump
> the variables to 120k, we need to use 2 * (120 - 56) => 128k.

Do I understand correctly that you suggest adding 64K to the live area,
64K to the spare area, and decreasing FVMAIN_COMPACT by 128k?

I think that's a step in the wrong direction.

$ build \
  -b DEBUG \
  -a IA32 -a X64 \
  -p OvmfPkg/OvmfPkgIa32X64.dsc \
  -t GCC48 \
  -D SMM_REQUIRE \
  -D SECURE_BOOT_ENABLE \
  -D HTTP_BOOT_ENABLE \
  -D NETWORK_IP6_ENABLE \
  -D TLS_ENABLE

> FV Space Information
> SECFV [14%Full] 212992 total, 30560 used, 182432 free
> FVMAIN_COMPACT [98%Full] 1753088 total, 1731168 used, 21920 free
> DXEFV [69%Full] 10485760 total, 7292952 used, 3192808 free
> PEIFV [33%Full] 917504 total, 304752 used, 612752 free

21920 bytes left free in FVMAIN_COMPACT, and that's with two thirds of
PEIFV, and one third of DXEFV, left un-utilized (hence almost perfectly
compressible).

After adding -D FD_SIZE_4MB:

> FV Space Information
> SECFV [14%Full] 212992 total, 30560 used, 182432 free
> FVMAIN_COMPACT [50%Full] 3457024 total, 1730864 used, 1726160 free
> DXEFV [69%Full] 10485760 total, 7292952 used, 3192808 free
> PEIFV [33%Full] 917504 total, 304752 used, 612752 free

> 
> So, what about adjusting our 2MB image to this?

I disagree with the idea. I disagree both with bumping the live varstore
size to only 120KB, and with doing that at the expense of
FVMAIN_COMPACT's size.

Changing the live varstore's size from 56 KB to anything else already
breaks compatibility, in itself, between old varstore files and new
firmware binaries. We should benefit as much as possible from that pain;
in other words, push the next such time out as far as possible to the
future. By keeping the final flash image size 2MB, we'd just save 2MB
otherwise fully useless MMIO space under 4GB.

> 
> Regarding how to 'upgrade' a system from using the smaller storage
> size, I don't think there are any good answers. (Which also applies to
> the 4MB case.)

I agree, on both points.

Which is why I think it's time to make the big jump now, and be safe for
the next several years.

In my vocabulary that means the most distant RHEL7 support deadline I
can presently look at, which seems to be 30 June 2024, from
<https://en.wikipedia.org/wiki/Red_Hat_Enterprise_Linux#Product_life_cycle>.

And, I don't see how this change would hurt, rather than benefit, other
downstreams, or any upstream users. The only cost is 2MB of currently
unused MMIO space under 4GB, which I had set aside in QEMU specifically
for this purpose (i.e., future growth), in commit 637a5acb46b3
("hw/i386/pc_sysfw: support two flash drives", 2013-11-28).

It's not like we have to pay for the silicon on the i440fx / Q35
motherboards.

> Does fw-cfg tell us the rom/flash regions?

It doesn't.

Even if it did, I don't see how the varstore template could be built
non-statically.

A statically built varstore template is necessary, because when creating
a new virtual machine, libvirt uses a (firmware binary <-> varstore
template) mapping, to create the private varstore file for the VM, from
the template that matches the selected firmware binary.

The (firmware binary <-> varstore template) mapping in the libvirt
configuration (the "nvram" stanza in /etc/libvirt/qemu.conf)
accommodates arbitrary pairings. FD_SIZE_4MB is just one use case that
puts this flexibility to use. Other examples include the historical case
when an SB-enabled OVMF binary needed a different varstore template from
an SB-disabled binary, and the current case when you have AARCH64 (TCG)
and x86_64 (KVM) VMs on the same x86_64 host, managed by the same
libvirt installation. In that case, the ArmVirtQemu firmware(s) have to
be paired with their matching (independent of OVMF) varstore templates.

The nvram stanza on my laptop looks like this:

nvram = [
  "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd",
  "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd",

  "/home/virt-images/OVMF_CODE.32.fd:/home/virt-images/OVMF_VARS.fd",
  "/home/virt-images/OVMF_CODE.3264.fd:/home/virt-images/OVMF_VARS.fd",
  "/home/virt-images/OVMF_CODE.fd:/home/virt-images/OVMF_VARS.fd",

  "/home/virt-images/OVMF_CODE.4m.32.fd:/home/virt-images/OVMF_VARS.4m.fd",

"/home/virt-images/OVMF_CODE.4m.3264.fd:/home/virt-images/OVMF_VARS.4m.fd",
  "/home/virt-images/OVMF_CODE.4m.fd:/home/virt-images/OVMF_VARS.4m.fd",

  "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/-AAVMF/AAVMF_VARS.fd",
  "/usr/share/AAVMF/AAVMF_CODE.verbose.fd:/usr/share/AAVMF/AAVMF_VARS.fd",


"/home/virt-images/arm/QEMU_EFI.fd.padded:/home/virt-images/arm/QEMU_VARS.fd.padded"
]

Thanks
Laszlo

> 
> -Jordan
> 
>> Importantly, a firmware binary built with -D FD_SIZE_4MB will *not* be
>> compatible with a variable store that originates from a variable store
>> template built *without* -D FD_SIZE_4MB. This is the reason for the large
>> increase, as every such change breaks compatibility between a new firmware
>> binary and old varstore files.
>>
>> Enlarging the varstore should not impact the performance of normal
>> operations, as we keep the varstore block size 4KB. The performance of
>> reclaim is affected, but that is expected (since reclaim has to rework the
>> full live area). And, reclaim should occur proportionally less frequently.
>>
>> While at it, the FVMAIN_COMPACT volume (with the compressed FFS file in
>> it) is also enlarged significantly, so that we have plenty of room for
>> future DXEFV (and perhaps PEIFV) increments -- DXEFV has been growing
>> steadily, and that increase shows through compression too. Right now the
>> PEIFV and DXEFV volumes need no resizing.
>>
>> Here's a summary:
>>
>>   Description                Compression type                Size [KB]
>>   -------------------------  -----------------  ----------------------
>>   Non-volatile data storage  open-coded binary    128 ->   512 ( +384)
>>                                data
>>     Variable store                                 56 ->   248 ( +192)
>>     Event log                                       4 ->     4 (   +0)
>>     Working block                                   4 ->     4 (   +0)
>>     Spare area                                     64 ->   256 ( +192)
>>
>>   FVMAIN_COMPACT             uncompressed        1712 ->  3376 (+1664)
>>     FV FFS file              LZMA compressed
>>       PEIFV                  uncompressed         896 ->   896 (   +0)
>>         individual PEI       uncompressed
>>           modules
>>       DXEFV                  uncompressed       10240 -> 10240 (   +0)
>>         individual DXE       uncompressed
>>           modules
>>
>>   SECFV                      uncompressed         208 ->   208 (   +0)
>>     SEC driver
>>     reset vector code
>>
>> Cc: Gary Ching-Pang Lin <glin@suse.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/OvmfPkg.fdf.inc  | 29 ++++++++++++
>>  OvmfPkg/VarStore.fdf.inc | 46 +++++++++++++++++++-
>>  2 files changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
>> index fce6c7b8ce64..a1eb2d380167 100644
>> --- a/OvmfPkg/OvmfPkg.fdf.inc
>> +++ b/OvmfPkg/OvmfPkg.fdf.inc
>> @@ -20,8 +20,35 @@
>>  #
>>  # Defining FD_SIZE_1MB on the build command line can override this.
>>  #
>> +# A firmware binary built for the default 2MB flash size, and a firmware binary
>> +# built with FD_SIZE_1MB defined, use the same variable store layout.
>> +#
>> +# Defining FD_SIZE_4MB results in a different (much larger) variable store
>> +# structure that is incompatible with both the default and the FD_SIZE_1MB
>> +# firmware binaries.
>> +#
>>  
>>  DEFINE BLOCK_SIZE        = 0x1000
>> +
>> +!ifdef $(FD_SIZE_4MB)
>> +
>> +DEFINE VARS_SIZE         = 0x80000
>> +DEFINE VARS_BLOCKS       = 0x80
>> +DEFINE VARS_LIVE_SIZE    = 0x3E000
>> +DEFINE VARS_SPARE_SIZE   = 0x40000
>> +
>> +DEFINE FW_BASE_ADDRESS   = 0xFFC00000
>> +DEFINE FW_SIZE           = 0x00400000
>> +DEFINE FW_BLOCKS         = 0x400
>> +DEFINE CODE_BASE_ADDRESS = 0xFFC80000
>> +DEFINE CODE_SIZE         = 0x00380000
>> +DEFINE CODE_BLOCKS       = 0x380
>> +DEFINE FVMAIN_SIZE       = 0x0034C000
>> +DEFINE SECFV_OFFSET      = 0x003CC000
>> +DEFINE SECFV_SIZE        = 0x34000
>> +
>> +!else
>> +
>>  DEFINE VARS_SIZE         = 0x20000
>>  DEFINE VARS_BLOCKS       = 0x20
>>  DEFINE VARS_LIVE_SIZE    = 0xE000
>> @@ -53,6 +80,8 @@
>>  
>>  !endif
>>  
>> +!endif
>> +
>>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress     = $(FW_BASE_ADDRESS)
>>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize    = $(FW_SIZE)
>>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE)
>> diff --git a/OvmfPkg/VarStore.fdf.inc b/OvmfPkg/VarStore.fdf.inc
>> index ce901c0109b1..7325b698157d 100644
>> --- a/OvmfPkg/VarStore.fdf.inc
>> +++ b/OvmfPkg/VarStore.fdf.inc
>> @@ -15,7 +15,11 @@
>>  #
>>  ##
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x00000000|0x0003e000
>> +!else
>>  0x00000000|0x0000e000
>> +!endif
>>  #NV_VARIABLE_STORE
>>  DATA = {
>>    ## This is the EFI_FIRMWARE_VOLUME_HEADER
>> @@ -27,14 +31,33 @@
>>    #     { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
>>    0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
>>    0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # FvLength: 0x80000
>> +  0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +!else
>>    # FvLength: 0x20000
>>    0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +!endif
>>    # Signature "_FVH"       # Attributes
>>    0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00,
>> -  # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision
>> -  0x48, 0x00, 0x19, 0xF9, 0x00, 0x00, 0x00, 0x02,
>> +  # HeaderLength
>> +  0x48, 0x00,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # CheckSum
>> +  0xB3, 0xF8,
>> +!else
>> +  # CheckSum
>> +  0x19, 0xF9,
>> +!endif
>> +  # ExtHeaderOffset #Reserved #Revision
>> +  0x00, 0x00, 0x00, 0x02,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # Blockmap[0]: 0x80 Blocks * 0x1000 Bytes / Block
>> +  0x80, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
>> +!else
>>    # Blockmap[0]: 0x20 Blocks * 0x1000 Bytes / Block
>>    0x20, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
>> +!endif
>>    # Blockmap[1]: End
>>    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>>    ## This is the VARIABLE_STORE_HEADER
>> @@ -44,18 +67,33 @@
>>    #     { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
>>    0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
>>    0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # Size: 0x3e000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
>> +  #          0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3dfb8
>> +  # This can speed up the Variable Dispatch a bit.
>> +  0xB8, 0xDF, 0x03, 0x00,
>> +!else
>>    # Size: 0xe000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
>>    #         0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xdfb8
>>    # This can speed up the Variable Dispatch a bit.
>>    0xB8, 0xDF, 0x00, 0x00,
>> +!endif
>>    # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
>>    0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  }
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x0003e000|0x00001000
>> +!else
>>  0x0000e000|0x00001000
>> +!endif
>>  #NV_EVENT_LOG
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x0003f000|0x00001000
>> +!else
>>  0x0000f000|0x00001000
>> +!endif
>>  #NV_FTW_WORKING
>>  DATA = {
>>    # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid         =
>> @@ -68,5 +106,9 @@
>>    0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  }
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x00040000|0x00040000
>> +!else
>>  0x00010000|0x00010000
>> +!endif
>>  #NV_FTW_SPARE
>> -- 
>> 2.9.3
>>
>>