Re: [edk2-devel] [PATCH v3 0/5] OvmfPkg: Use QemuKernelLoaderFs to read cmdline/initrd

["Dov Murik" <dovmurik@linux.ibm.com>]

On 29/06/2021 15:54, Laszlo Ersek wrote:
> On 06/28/21 12:51, Dov Murik wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>>
>> In order to support measured SEV boot with kernel/initrd/cmdline, we'd
>> like to have one place that reads those blobs; in the future we'll add
>> the measurement and verification in that place.
>>
>> We already have a synthetic filesystem (QemuKernelLoaderFs) which holds
>> three files: "kernel", "initrd", and "cmdline".  The kernel is indeed
>> read from this filesystem in LoadImage; but the cmdline (and the length
>> of initrd) are read from QemuFwCfgLib items.
>>
>> This patch series first fixes two identical memory leak bugs in
>> GenericQemuLoadImageLib and X86QemuLoadImageLib; then modifies
>> GenericQemuLoadImageLib to read cmdline (and the initrd size) from the
>> QemuKernelLoaderFs synthetic filesystem, thus removing the dependency on
>> QemuFwCfgLib.
>>
>> Note that X86QemuLoadImageLib is not modified, because it contains a
>> QemuLoadLegacyImage() which reads other items of the QemuFwCfg which are
>> not available in QemuKernelLoaderFs.  Since we don't want to support the
>> legacy boot path in the future measured SEV boot, we leave
>> X86QemuLoadImageLib as-is (except for a comment addition in patch 3) and
>> will force use for GenericQemuLoadImageLib in the measured SEV boot
>> implementation.
>>
>> Relevant discussion threads start in:
>> https://edk2.groups.io/g/devel/message/76069
>>
>> To test this on x86_64, I forced the use of GenericQemuLoadImageLib
>> using the following local patch:
>>
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 0a237a905866..46442b543bcf 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -404,7 +404,7 @@ [LibraryClasses.common.DXE_DRIVER]
>>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>>    MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
>> -  QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
>> +  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf # XXX don't commit this or someone will be mad
>>  !if $(TPM_ENABLE) == TRUE
>>    Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
>>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>>
>>
>> I tested boot with QEMU and OVMF with the following QEMU arguments:
>>
>>   -kernel a
>>   -kernel a -initrd b
>>   -kernel a -cmdline c
>>   -kernel a -initrd b -cmdline c
>>
>> (and also without -kernel)
>>
>>
>> Code is at
>> https://github.com/confidential-containers-demo/edk2/tree/use-synthetic-fs-for-cmdline-v3
>>
>> v3 changes:
>>  - Insert patches 1+2 at the top of the series to fix cmdline leak bugs
>>  - Organize #include and .inf
>>  - Add UINTN overflow check
>>  - Fix error paths and function epilogue to properly release all resources
>>  - Clarity: rename long variables, reword comments
>>
>> v2: https://edk2.groups.io/g/devel/message/76664
>> v2 changes:
>>  - Add comment to header of X86QemuLoadImageLib.inf
>>  - Clearer function names in GenericQemuLoadImageLib.c
>>  - Fix coding style issues
>>
>> v1: https://edk2.groups.io/g/devel/message/76265
>>
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>>
>>
>> Dov Murik (5):
>>   OvmfPkg/GenericQemuLoadImageLib: plug cmdline blob leak on success
>>   OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success
>>   Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command
>>     line"
>>   OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs
>>   OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header
>>
>>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf |   3 +-
>>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf         |   3 +
>>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c   | 157 ++++++++++++++++++--
>>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c           |   9 +-
>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c               |  11 +-
>>  5 files changed, 161 insertions(+), 22 deletions(-)
>>
> 
> Merged as commit range d1fc3d7ef3cb..9421f5ab8d1e, via
> <https://github.com/tianocore/edk2/pull/1770>.
> 
> (The BZ remains open for the upcoming (related) patch sets.)
> 

Thanks a lot Laszlo for the thorough review rounds.  I'll prepare the
next phase.

Out of curiousity, I wonder regarding the leak fixes -- is there a way
to see that the fix works? Is there some accounting of used pages that
we can check that decreases after the fix?

Thanks,
-Dov