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(-)
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(-)
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77174): https://edk2.groups.io/g/devel/message/77174
Mute This Topic: https://groups.io/mt/83841915/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77271): https://edk2.groups.io/g/devel/message/77271 Mute This Topic: https://groups.io/mt/83841915/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77272): https://edk2.groups.io/g/devel/message/77272 Mute This Topic: https://groups.io/mt/83841915/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 06/29/21 15:03, Dov Murik wrote: > 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? You could try a UEFI memmap comparison, but the cmdline is a pool allocation, not a page allocation, so I don't think the difference is noticeable in the UEFI memmap (the allocation is really small, so it is likely satisfied from one of the preallocated "bins"). However, in DEBUG and NOOPT builds of OVMF, FreePool() should actually wipe (part of) the freed area (with the PcdDebugClearMemoryValue=0xAF byte value), and then a double-free would trigger an assertion failure (signature missing), if I remember correctly. See the CoreFreePoolI() function in "MdeModulePkg/Core/Dxe/Mem/Pool.c", in particular the signature checks on top, and later DEBUG_CLEAR_MEMORY(). Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77273): https://edk2.groups.io/g/devel/message/77273 Mute This Topic: https://groups.io/mt/83841915/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
When QemuLoadKernelImage() ends successfully, the command-line blob is
not freed, even though it is not used elsewhere (its content is already
copied to KernelLoadedImage->LoadOptions). The memory leak bug was
introduced in commit ddd2be6b0026 ("OvmfPkg: provide a generic
implementation of QemuLoadImageLib", 2020-03-05).
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>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
index 114db7e8441f..8a29976ae172 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
@@ -193,14 +193,16 @@ QemuLoadKernelImage (
}
*ImageHandle = KernelImageHandle;
- return EFI_SUCCESS;
+ Status = EFI_SUCCESS;
FreeCommandLine:
if (CommandLineSize > 0) {
FreePool (CommandLine);
}
UnloadImage:
- gBS->UnloadImage (KernelImageHandle);
+ if (EFI_ERROR (Status)) {
+ gBS->UnloadImage (KernelImageHandle);
+ }
return Status;
}
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77172): https://edk2.groups.io/g/devel/message/77172
Mute This Topic: https://groups.io/mt/83841912/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> When QemuLoadKernelImage() ends successfully, the command-line blob is
> not freed, even though it is not used elsewhere (its content is already
> copied to KernelLoadedImage->LoadOptions). The memory leak bug was
> introduced in commit ddd2be6b0026 ("OvmfPkg: provide a generic
> implementation of QemuLoadImageLib", 2020-03-05).
>
> 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>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> index 114db7e8441f..8a29976ae172 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> @@ -193,14 +193,16 @@ QemuLoadKernelImage (
> }
>
> *ImageHandle = KernelImageHandle;
> - return EFI_SUCCESS;
> + Status = EFI_SUCCESS;
>
> FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> UnloadImage:
> - gBS->UnloadImage (KernelImageHandle);
> + if (EFI_ERROR (Status)) {
> + gBS->UnloadImage (KernelImageHandle);
> + }
>
> return Status;
> }
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77257): https://edk2.groups.io/g/devel/message/77257
Mute This Topic: https://groups.io/mt/83841912/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> When QemuLoadKernelImage() ends successfully, the command-line blob is
> not freed, even though it is not used elsewhere (its content is already
> copied to KernelLoadedImage->LoadOptions). The memory leak bug was
> introduced in commit ddd2be6b0026 ("OvmfPkg: provide a generic
> implementation of QemuLoadImageLib", 2020-03-05).
>
> 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>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> index 114db7e8441f..8a29976ae172 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> @@ -193,14 +193,16 @@ QemuLoadKernelImage (
> }
>
> *ImageHandle = KernelImageHandle;
> - return EFI_SUCCESS;
> + Status = EFI_SUCCESS;
>
> FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> UnloadImage:
> - gBS->UnloadImage (KernelImageHandle);
> + if (EFI_ERROR (Status)) {
> + gBS->UnloadImage (KernelImageHandle);
> + }
>
> return Status;
> }
>
using an aarch64 guest,
Tested-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77262): https://edk2.groups.io/g/devel/message/77262
Mute This Topic: https://groups.io/mt/83841912/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
When QemuLoadKernelImage() ends successfully, the command-line blob is
not freed, even though it is not used elsewhere (its content is already
copied to KernelLoadedImage->LoadOptions). The memory leak bug was
introduced in commit 7c47d89003a6 ("OvmfPkg: implement QEMU loader
library for X86 with legacy fallback", 2020-03-05).
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>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
index 1177582ab051..6b1e7e649014 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
@@ -446,14 +446,16 @@ QemuLoadKernelImage (
}
*ImageHandle = KernelImageHandle;
- return EFI_SUCCESS;
+ Status = EFI_SUCCESS;
FreeCommandLine:
if (CommandLineSize > 0) {
FreePool (CommandLine);
}
UnloadImage:
- gBS->UnloadImage (KernelImageHandle);
+ if (EFI_ERROR (Status)) {
+ gBS->UnloadImage (KernelImageHandle);
+ }
return Status;
}
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77173): https://edk2.groups.io/g/devel/message/77173
Mute This Topic: https://groups.io/mt/83841914/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> When QemuLoadKernelImage() ends successfully, the command-line blob is
> not freed, even though it is not used elsewhere (its content is already
> copied to KernelLoadedImage->LoadOptions). The memory leak bug was
> introduced in commit 7c47d89003a6 ("OvmfPkg: implement QEMU loader
> library for X86 with legacy fallback", 2020-03-05).
>
> 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>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> index 1177582ab051..6b1e7e649014 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> @@ -446,14 +446,16 @@ QemuLoadKernelImage (
> }
>
> *ImageHandle = KernelImageHandle;
> - return EFI_SUCCESS;
> + Status = EFI_SUCCESS;
>
> FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> UnloadImage:
> - gBS->UnloadImage (KernelImageHandle);
> + if (EFI_ERROR (Status)) {
> + gBS->UnloadImage (KernelImageHandle);
> + }
>
> return Status;
> }
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77258): https://edk2.groups.io/g/devel/message/77258
Mute This Topic: https://groups.io/mt/83841914/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> When QemuLoadKernelImage() ends successfully, the command-line blob is
> not freed, even though it is not used elsewhere (its content is already
> copied to KernelLoadedImage->LoadOptions). The memory leak bug was
> introduced in commit 7c47d89003a6 ("OvmfPkg: implement QEMU loader
> library for X86 with legacy fallback", 2020-03-05).
>
> 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>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> index 1177582ab051..6b1e7e649014 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> @@ -446,14 +446,16 @@ QemuLoadKernelImage (
> }
>
> *ImageHandle = KernelImageHandle;
> - return EFI_SUCCESS;
> + Status = EFI_SUCCESS;
>
> FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> UnloadImage:
> - gBS->UnloadImage (KernelImageHandle);
> + if (EFI_ERROR (Status)) {
> + gBS->UnloadImage (KernelImageHandle);
> + }
>
> return Status;
> }
>
Tested-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77265): https://edk2.groups.io/g/devel/message/77265
Mute This Topic: https://groups.io/mt/83841914/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.
Manually fixed conflicts in:
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
Note that besides re-exposing the kernel command line as a file in the
synthetic filesystem, we also revert back to AllocatePool instead of
AllocatePages.
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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index b09ff6a3590d..c7ddd86f5c75 100644
--- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
+++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
@@ -33,6 +33,7 @@
typedef enum {
KernelBlobTypeKernel,
KernelBlobTypeInitrd,
+ KernelBlobTypeCommandLine,
KernelBlobTypeMax
} KERNEL_BLOB_TYPE;
@@ -59,6 +60,11 @@ STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] = {
{
{ QemuFwCfgItemInitrdSize, QemuFwCfgItemInitrdData, },
}
+ }, {
+ L"cmdline",
+ {
+ { QemuFwCfgItemCommandLineSize, QemuFwCfgItemCommandLineData, },
+ }
}
};
@@ -948,7 +954,7 @@ FetchBlob (
//
// Read blob.
//
- Blob->Data = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)Blob->Size));
+ Blob->Data = AllocatePool (Blob->Size);
if (Blob->Data == NULL) {
DEBUG ((DEBUG_ERROR, "%a: failed to allocate %Ld bytes for \"%s\"\n",
__FUNCTION__, (INT64)Blob->Size, Blob->Name));
@@ -1083,8 +1089,7 @@ FreeBlobs:
while (BlobType > 0) {
CurrentBlob = &mKernelBlob[--BlobType];
if (CurrentBlob->Data != NULL) {
- FreePages (CurrentBlob->Data,
- EFI_SIZE_TO_PAGES ((UINTN)CurrentBlob->Size));
+ FreePool (CurrentBlob->Data);
CurrentBlob->Size = 0;
CurrentBlob->Data = NULL;
}
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77176): https://edk2.groups.io/g/devel/message/77176
Mute This Topic: https://groups.io/mt/83841917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.
>
> Manually fixed conflicts in:
> OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>
> Note that besides re-exposing the kernel command line as a file in the
> synthetic filesystem, we also revert back to AllocatePool instead of
> AllocatePages.
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index b09ff6a3590d..c7ddd86f5c75 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -33,6 +33,7 @@
> typedef enum {
> KernelBlobTypeKernel,
> KernelBlobTypeInitrd,
> + KernelBlobTypeCommandLine,
> KernelBlobTypeMax
> } KERNEL_BLOB_TYPE;
>
> @@ -59,6 +60,11 @@ STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] = {
> {
> { QemuFwCfgItemInitrdSize, QemuFwCfgItemInitrdData, },
> }
> + }, {
> + L"cmdline",
> + {
> + { QemuFwCfgItemCommandLineSize, QemuFwCfgItemCommandLineData, },
> + }
> }
> };
>
> @@ -948,7 +954,7 @@ FetchBlob (
> //
> // Read blob.
> //
> - Blob->Data = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)Blob->Size));
> + Blob->Data = AllocatePool (Blob->Size);
> if (Blob->Data == NULL) {
> DEBUG ((DEBUG_ERROR, "%a: failed to allocate %Ld bytes for \"%s\"\n",
> __FUNCTION__, (INT64)Blob->Size, Blob->Name));
> @@ -1083,8 +1089,7 @@ FreeBlobs:
> while (BlobType > 0) {
> CurrentBlob = &mKernelBlob[--BlobType];
> if (CurrentBlob->Data != NULL) {
> - FreePages (CurrentBlob->Data,
> - EFI_SIZE_TO_PAGES ((UINTN)CurrentBlob->Size));
> + FreePool (CurrentBlob->Data);
> CurrentBlob->Size = 0;
> CurrentBlob->Data = NULL;
> }
>
Thanks for the updates.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77259): https://edk2.groups.io/g/devel/message/77259
Mute This Topic: https://groups.io/mt/83841917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.
>
> Manually fixed conflicts in:
> OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>
> Note that besides re-exposing the kernel command line as a file in the
> synthetic filesystem, we also revert back to AllocatePool instead of
> AllocatePages.
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index b09ff6a3590d..c7ddd86f5c75 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -33,6 +33,7 @@
> typedef enum {
> KernelBlobTypeKernel,
> KernelBlobTypeInitrd,
> + KernelBlobTypeCommandLine,
> KernelBlobTypeMax
> } KERNEL_BLOB_TYPE;
>
> @@ -59,6 +60,11 @@ STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] = {
> {
> { QemuFwCfgItemInitrdSize, QemuFwCfgItemInitrdData, },
> }
> + }, {
> + L"cmdline",
> + {
> + { QemuFwCfgItemCommandLineSize, QemuFwCfgItemCommandLineData, },
> + }
> }
> };
>
> @@ -948,7 +954,7 @@ FetchBlob (
> //
> // Read blob.
> //
> - Blob->Data = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)Blob->Size));
> + Blob->Data = AllocatePool (Blob->Size);
> if (Blob->Data == NULL) {
> DEBUG ((DEBUG_ERROR, "%a: failed to allocate %Ld bytes for \"%s\"\n",
> __FUNCTION__, (INT64)Blob->Size, Blob->Name));
> @@ -1083,8 +1089,7 @@ FreeBlobs:
> while (BlobType > 0) {
> CurrentBlob = &mKernelBlob[--BlobType];
> if (CurrentBlob->Data != NULL) {
> - FreePages (CurrentBlob->Data,
> - EFI_SIZE_TO_PAGES ((UINTN)CurrentBlob->Size));
> + FreePool (CurrentBlob->Data);
> CurrentBlob->Size = 0;
> CurrentBlob->Data = NULL;
> }
>
using an aarch64 guest,
Tested-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77263): https://edk2.groups.io/g/devel/message/77263
Mute This Topic: https://groups.io/mt/83841917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Remove the QemuFwCfgLib interface used to read the QEMU cmdline
(-append argument) and the initrd size. Instead, use the synthetic
filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd",
and "cmdline".
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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 3 +-
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 151 ++++++++++++++++++--
2 files changed, 139 insertions(+), 15 deletions(-)
diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
index b262cb926a4d..9c9e35b1c5b9 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
@@ -25,14 +25,15 @@ [Packages]
[LibraryClasses]
DebugLib
+ FileHandleLib
MemoryAllocationLib
PrintLib
- QemuFwCfgLib
UefiBootServicesTableLib
[Protocols]
gEfiDevicePathProtocolGuid
gEfiLoadedImageProtocolGuid
+ gEfiSimpleFileSystemProtocolGuid
[Guids]
gQemuKernelLoaderFsMediaGuid
diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
index 8a29976ae172..66e029397bd6 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
@@ -11,13 +11,14 @@
#include <Base.h>
#include <Guid/QemuKernelLoaderFsMedia.h>
#include <Library/DebugLib.h>
+#include <Library/FileHandleLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PrintLib.h>
-#include <Library/QemuFwCfgLib.h>
#include <Library/QemuLoadImageLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Protocol/DevicePath.h>
#include <Protocol/LoadedImage.h>
+#include <Protocol/SimpleFileSystem.h>
#pragma pack (1)
typedef struct {
@@ -30,6 +31,11 @@ typedef struct {
KERNEL_FILE_DEVPATH FileNode;
EFI_DEVICE_PATH_PROTOCOL EndNode;
} KERNEL_VENMEDIA_FILE_DEVPATH;
+
+typedef struct {
+ VENDOR_DEVICE_PATH VenMediaNode;
+ EFI_DEVICE_PATH_PROTOCOL EndNode;
+} SINGLE_VENMEDIA_NODE_DEVPATH;
#pragma pack ()
STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
@@ -51,6 +57,82 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
}
};
+STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFsDevicePath = {
+ {
+ {
+ MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,
+ { sizeof (VENDOR_DEVICE_PATH) }
+ },
+ QEMU_KERNEL_LOADER_FS_MEDIA_GUID
+ }, {
+ END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
+ { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
+ }
+};
+
+STATIC
+EFI_STATUS
+GetQemuKernelLoaderBlobSize (
+ IN EFI_FILE_HANDLE Root,
+ IN CHAR16 *FileName,
+ OUT UINTN *Size
+ )
+{
+ EFI_STATUS Status;
+ EFI_FILE_HANDLE FileHandle;
+ UINT64 FileSize;
+
+ Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = FileHandleGetSize (FileHandle, &FileSize);
+ if (EFI_ERROR (Status)) {
+ goto CloseFile;
+ }
+ if (FileSize > MAX_UINTN) {
+ Status = EFI_UNSUPPORTED;
+ goto CloseFile;
+ }
+ *Size = (UINTN)FileSize;
+ Status = EFI_SUCCESS;
+CloseFile:
+ FileHandle->Close (FileHandle);
+ return Status;
+}
+
+STATIC
+EFI_STATUS
+ReadWholeQemuKernelLoaderBlob (
+ IN EFI_FILE_HANDLE Root,
+ IN CHAR16 *FileName,
+ IN UINTN Size,
+ OUT VOID *Buffer
+ )
+{
+ EFI_STATUS Status;
+ EFI_FILE_HANDLE FileHandle;
+ UINTN ReadSize;
+
+ Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ ReadSize = Size;
+ Status = FileHandle->Read (FileHandle, &ReadSize, Buffer);
+ if (EFI_ERROR (Status)) {
+ goto CloseFile;
+ }
+ if (ReadSize != Size) {
+ Status = EFI_PROTOCOL_ERROR;
+ goto CloseFile;
+ }
+ Status = EFI_SUCCESS;
+CloseFile:
+ FileHandle->Close (FileHandle);
+ return Status;
+}
+
/**
Download the kernel, the initial ramdisk, and the kernel command line from
QEMU's fw_cfg. The kernel will be instructed via its command line to load
@@ -76,12 +158,16 @@ QemuLoadKernelImage (
OUT EFI_HANDLE *ImageHandle
)
{
- EFI_STATUS Status;
- EFI_HANDLE KernelImageHandle;
- EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
- UINTN CommandLineSize;
- CHAR8 *CommandLine;
- UINTN InitrdSize;
+ EFI_STATUS Status;
+ EFI_HANDLE KernelImageHandle;
+ EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
+ EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
+ EFI_HANDLE FsVolumeHandle;
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol;
+ EFI_FILE_HANDLE Root;
+ UINTN CommandLineSize;
+ CHAR8 *CommandLine;
+ UINTN InitrdSize;
//
// Load the image. This should call back into the QEMU EFI loader file system.
@@ -124,8 +210,38 @@ QemuLoadKernelImage (
);
ASSERT_EFI_ERROR (Status);
- QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
- CommandLineSize = (UINTN)QemuFwCfgRead32 ();
+ //
+ // Open the Qemu Kernel Loader abstract filesystem (volume) which will be
+ // used to query the "initrd" and to read the "cmdline" synthetic files.
+ //
+ DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *)&mQemuKernelLoaderFsDevicePath;
+ Status = gBS->LocateDevicePath (
+ &gEfiSimpleFileSystemProtocolGuid,
+ &DevicePathNode,
+ &FsVolumeHandle
+ );
+ if (EFI_ERROR (Status)) {
+ goto UnloadImage;
+ }
+
+ Status = gBS->HandleProtocol (
+ FsVolumeHandle,
+ &gEfiSimpleFileSystemProtocolGuid,
+ (VOID **)&FsProtocol
+ );
+ if (EFI_ERROR (Status)) {
+ goto UnloadImage;
+ }
+
+ Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
+ if (EFI_ERROR (Status)) {
+ goto UnloadImage;
+ }
+
+ Status = GetQemuKernelLoaderBlobSize (Root, L"cmdline", &CommandLineSize);
+ if (EFI_ERROR (Status)) {
+ goto CloseRoot;
+ }
if (CommandLineSize == 0) {
KernelLoadedImage->LoadOptionsSize = 0;
@@ -133,11 +249,14 @@ QemuLoadKernelImage (
CommandLine = AllocatePool (CommandLineSize);
if (CommandLine == NULL) {
Status = EFI_OUT_OF_RESOURCES;
- goto UnloadImage;
+ goto CloseRoot;
}
- QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
- QemuFwCfgReadBytes (CommandLineSize, CommandLine);
+ Status = ReadWholeQemuKernelLoaderBlob (Root, L"cmdline", CommandLineSize,
+ CommandLine);
+ if (EFI_ERROR (Status)) {
+ goto FreeCommandLine;
+ }
//
// Verify NUL-termination of the command line.
@@ -155,8 +274,10 @@ QemuLoadKernelImage (
KernelLoadedImage->LoadOptionsSize = (UINT32)((CommandLineSize - 1) * 2);
}
- QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
- InitrdSize = (UINTN)QemuFwCfgRead32 ();
+ Status = GetQemuKernelLoaderBlobSize (Root, L"initrd", &InitrdSize);
+ if (EFI_ERROR (Status)) {
+ goto FreeCommandLine;
+ }
if (InitrdSize > 0) {
//
@@ -199,6 +320,8 @@ FreeCommandLine:
if (CommandLineSize > 0) {
FreePool (CommandLine);
}
+CloseRoot:
+ Root->Close (Root);
UnloadImage:
if (EFI_ERROR (Status)) {
gBS->UnloadImage (KernelImageHandle);
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77175): https://edk2.groups.io/g/devel/message/77175
Mute This Topic: https://groups.io/mt/83841916/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> Remove the QemuFwCfgLib interface used to read the QEMU cmdline
> (-append argument) and the initrd size. Instead, use the synthetic
> filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd",
> and "cmdline".
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 3 +-
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 151 ++++++++++++++++++--
> 2 files changed, 139 insertions(+), 15 deletions(-)
>
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> index b262cb926a4d..9c9e35b1c5b9 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> @@ -25,14 +25,15 @@ [Packages]
>
> [LibraryClasses]
> DebugLib
> + FileHandleLib
> MemoryAllocationLib
> PrintLib
> - QemuFwCfgLib
> UefiBootServicesTableLib
>
> [Protocols]
> gEfiDevicePathProtocolGuid
> gEfiLoadedImageProtocolGuid
> + gEfiSimpleFileSystemProtocolGuid
>
> [Guids]
> gQemuKernelLoaderFsMediaGuid
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> index 8a29976ae172..66e029397bd6 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> @@ -11,13 +11,14 @@
> #include <Base.h>
> #include <Guid/QemuKernelLoaderFsMedia.h>
> #include <Library/DebugLib.h>
> +#include <Library/FileHandleLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PrintLib.h>
> -#include <Library/QemuFwCfgLib.h>
> #include <Library/QemuLoadImageLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Protocol/DevicePath.h>
> #include <Protocol/LoadedImage.h>
> +#include <Protocol/SimpleFileSystem.h>
>
> #pragma pack (1)
> typedef struct {
> @@ -30,6 +31,11 @@ typedef struct {
> KERNEL_FILE_DEVPATH FileNode;
> EFI_DEVICE_PATH_PROTOCOL EndNode;
> } KERNEL_VENMEDIA_FILE_DEVPATH;
> +
> +typedef struct {
> + VENDOR_DEVICE_PATH VenMediaNode;
> + EFI_DEVICE_PATH_PROTOCOL EndNode;
> +} SINGLE_VENMEDIA_NODE_DEVPATH;
> #pragma pack ()
>
> STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
> @@ -51,6 +57,82 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
> }
> };
>
> +STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFsDevicePath = {
> + {
> + {
> + MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,
> + { sizeof (VENDOR_DEVICE_PATH) }
> + },
> + QEMU_KERNEL_LOADER_FS_MEDIA_GUID
> + }, {
> + END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> + { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
> + }
> +};
> +
> +STATIC
> +EFI_STATUS
> +GetQemuKernelLoaderBlobSize (
> + IN EFI_FILE_HANDLE Root,
> + IN CHAR16 *FileName,
> + OUT UINTN *Size
> + )
> +{
> + EFI_STATUS Status;
> + EFI_FILE_HANDLE FileHandle;
> + UINT64 FileSize;
> +
> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = FileHandleGetSize (FileHandle, &FileSize);
> + if (EFI_ERROR (Status)) {
> + goto CloseFile;
> + }
> + if (FileSize > MAX_UINTN) {
> + Status = EFI_UNSUPPORTED;
> + goto CloseFile;
> + }
> + *Size = (UINTN)FileSize;
> + Status = EFI_SUCCESS;
> +CloseFile:
> + FileHandle->Close (FileHandle);
> + return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +ReadWholeQemuKernelLoaderBlob (
> + IN EFI_FILE_HANDLE Root,
> + IN CHAR16 *FileName,
> + IN UINTN Size,
> + OUT VOID *Buffer
> + )
> +{
> + EFI_STATUS Status;
> + EFI_FILE_HANDLE FileHandle;
> + UINTN ReadSize;
> +
> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + ReadSize = Size;
> + Status = FileHandle->Read (FileHandle, &ReadSize, Buffer);
> + if (EFI_ERROR (Status)) {
> + goto CloseFile;
> + }
> + if (ReadSize != Size) {
> + Status = EFI_PROTOCOL_ERROR;
> + goto CloseFile;
> + }
> + Status = EFI_SUCCESS;
> +CloseFile:
> + FileHandle->Close (FileHandle);
> + return Status;
> +}
> +
> /**
> Download the kernel, the initial ramdisk, and the kernel command line from
> QEMU's fw_cfg. The kernel will be instructed via its command line to load
> @@ -76,12 +158,16 @@ QemuLoadKernelImage (
> OUT EFI_HANDLE *ImageHandle
> )
> {
> - EFI_STATUS Status;
> - EFI_HANDLE KernelImageHandle;
> - EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
> - UINTN CommandLineSize;
> - CHAR8 *CommandLine;
> - UINTN InitrdSize;
> + EFI_STATUS Status;
> + EFI_HANDLE KernelImageHandle;
> + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
> + EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
> + EFI_HANDLE FsVolumeHandle;
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol;
> + EFI_FILE_HANDLE Root;
> + UINTN CommandLineSize;
> + CHAR8 *CommandLine;
> + UINTN InitrdSize;
>
> //
> // Load the image. This should call back into the QEMU EFI loader file system.
> @@ -124,8 +210,38 @@ QemuLoadKernelImage (
> );
> ASSERT_EFI_ERROR (Status);
>
> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
> - CommandLineSize = (UINTN)QemuFwCfgRead32 ();
> + //
> + // Open the Qemu Kernel Loader abstract filesystem (volume) which will be
> + // used to query the "initrd" and to read the "cmdline" synthetic files.
> + //
> + DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *)&mQemuKernelLoaderFsDevicePath;
> + Status = gBS->LocateDevicePath (
> + &gEfiSimpleFileSystemProtocolGuid,
> + &DevicePathNode,
> + &FsVolumeHandle
> + );
> + if (EFI_ERROR (Status)) {
> + goto UnloadImage;
> + }
> +
> + Status = gBS->HandleProtocol (
> + FsVolumeHandle,
> + &gEfiSimpleFileSystemProtocolGuid,
> + (VOID **)&FsProtocol
> + );
> + if (EFI_ERROR (Status)) {
> + goto UnloadImage;
> + }
> +
> + Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
> + if (EFI_ERROR (Status)) {
> + goto UnloadImage;
> + }
> +
> + Status = GetQemuKernelLoaderBlobSize (Root, L"cmdline", &CommandLineSize);
> + if (EFI_ERROR (Status)) {
> + goto CloseRoot;
> + }
>
> if (CommandLineSize == 0) {
> KernelLoadedImage->LoadOptionsSize = 0;
> @@ -133,11 +249,14 @@ QemuLoadKernelImage (
> CommandLine = AllocatePool (CommandLineSize);
> if (CommandLine == NULL) {
> Status = EFI_OUT_OF_RESOURCES;
> - goto UnloadImage;
> + goto CloseRoot;
> }
>
> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
> - QemuFwCfgReadBytes (CommandLineSize, CommandLine);
> + Status = ReadWholeQemuKernelLoaderBlob (Root, L"cmdline", CommandLineSize,
> + CommandLine);
> + if (EFI_ERROR (Status)) {
> + goto FreeCommandLine;
> + }
>
> //
> // Verify NUL-termination of the command line.
> @@ -155,8 +274,10 @@ QemuLoadKernelImage (
> KernelLoadedImage->LoadOptionsSize = (UINT32)((CommandLineSize - 1) * 2);
> }
>
> - QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
> - InitrdSize = (UINTN)QemuFwCfgRead32 ();
> + Status = GetQemuKernelLoaderBlobSize (Root, L"initrd", &InitrdSize);
> + if (EFI_ERROR (Status)) {
> + goto FreeCommandLine;
> + }
>
> if (InitrdSize > 0) {
> //
> @@ -199,6 +320,8 @@ FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> +CloseRoot:
> + Root->Close (Root);
> UnloadImage:
> if (EFI_ERROR (Status)) {
> gBS->UnloadImage (KernelImageHandle);
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77260): https://edk2.groups.io/g/devel/message/77260
Mute This Topic: https://groups.io/mt/83841916/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote:
> Remove the QemuFwCfgLib interface used to read the QEMU cmdline
> (-append argument) and the initrd size. Instead, use the synthetic
> filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd",
> and "cmdline".
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 3 +-
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 151 ++++++++++++++++++--
> 2 files changed, 139 insertions(+), 15 deletions(-)
>
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> index b262cb926a4d..9c9e35b1c5b9 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> @@ -25,14 +25,15 @@ [Packages]
>
> [LibraryClasses]
> DebugLib
> + FileHandleLib
> MemoryAllocationLib
> PrintLib
> - QemuFwCfgLib
> UefiBootServicesTableLib
>
> [Protocols]
> gEfiDevicePathProtocolGuid
> gEfiLoadedImageProtocolGuid
> + gEfiSimpleFileSystemProtocolGuid
>
> [Guids]
> gQemuKernelLoaderFsMediaGuid
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> index 8a29976ae172..66e029397bd6 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> @@ -11,13 +11,14 @@
> #include <Base.h>
> #include <Guid/QemuKernelLoaderFsMedia.h>
> #include <Library/DebugLib.h>
> +#include <Library/FileHandleLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PrintLib.h>
> -#include <Library/QemuFwCfgLib.h>
> #include <Library/QemuLoadImageLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Protocol/DevicePath.h>
> #include <Protocol/LoadedImage.h>
> +#include <Protocol/SimpleFileSystem.h>
>
> #pragma pack (1)
> typedef struct {
> @@ -30,6 +31,11 @@ typedef struct {
> KERNEL_FILE_DEVPATH FileNode;
> EFI_DEVICE_PATH_PROTOCOL EndNode;
> } KERNEL_VENMEDIA_FILE_DEVPATH;
> +
> +typedef struct {
> + VENDOR_DEVICE_PATH VenMediaNode;
> + EFI_DEVICE_PATH_PROTOCOL EndNode;
> +} SINGLE_VENMEDIA_NODE_DEVPATH;
> #pragma pack ()
>
> STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
> @@ -51,6 +57,82 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
> }
> };
>
> +STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFsDevicePath = {
> + {
> + {
> + MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,
> + { sizeof (VENDOR_DEVICE_PATH) }
> + },
> + QEMU_KERNEL_LOADER_FS_MEDIA_GUID
> + }, {
> + END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> + { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
> + }
> +};
> +
> +STATIC
> +EFI_STATUS
> +GetQemuKernelLoaderBlobSize (
> + IN EFI_FILE_HANDLE Root,
> + IN CHAR16 *FileName,
> + OUT UINTN *Size
> + )
> +{
> + EFI_STATUS Status;
> + EFI_FILE_HANDLE FileHandle;
> + UINT64 FileSize;
> +
> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = FileHandleGetSize (FileHandle, &FileSize);
> + if (EFI_ERROR (Status)) {
> + goto CloseFile;
> + }
> + if (FileSize > MAX_UINTN) {
> + Status = EFI_UNSUPPORTED;
> + goto CloseFile;
> + }
> + *Size = (UINTN)FileSize;
> + Status = EFI_SUCCESS;
> +CloseFile:
> + FileHandle->Close (FileHandle);
> + return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +ReadWholeQemuKernelLoaderBlob (
> + IN EFI_FILE_HANDLE Root,
> + IN CHAR16 *FileName,
> + IN UINTN Size,
> + OUT VOID *Buffer
> + )
> +{
> + EFI_STATUS Status;
> + EFI_FILE_HANDLE FileHandle;
> + UINTN ReadSize;
> +
> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + ReadSize = Size;
> + Status = FileHandle->Read (FileHandle, &ReadSize, Buffer);
> + if (EFI_ERROR (Status)) {
> + goto CloseFile;
> + }
> + if (ReadSize != Size) {
> + Status = EFI_PROTOCOL_ERROR;
> + goto CloseFile;
> + }
> + Status = EFI_SUCCESS;
> +CloseFile:
> + FileHandle->Close (FileHandle);
> + return Status;
> +}
> +
> /**
> Download the kernel, the initial ramdisk, and the kernel command line from
> QEMU's fw_cfg. The kernel will be instructed via its command line to load
> @@ -76,12 +158,16 @@ QemuLoadKernelImage (
> OUT EFI_HANDLE *ImageHandle
> )
> {
> - EFI_STATUS Status;
> - EFI_HANDLE KernelImageHandle;
> - EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
> - UINTN CommandLineSize;
> - CHAR8 *CommandLine;
> - UINTN InitrdSize;
> + EFI_STATUS Status;
> + EFI_HANDLE KernelImageHandle;
> + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
> + EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
> + EFI_HANDLE FsVolumeHandle;
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol;
> + EFI_FILE_HANDLE Root;
> + UINTN CommandLineSize;
> + CHAR8 *CommandLine;
> + UINTN InitrdSize;
>
> //
> // Load the image. This should call back into the QEMU EFI loader file system.
> @@ -124,8 +210,38 @@ QemuLoadKernelImage (
> );
> ASSERT_EFI_ERROR (Status);
>
> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
> - CommandLineSize = (UINTN)QemuFwCfgRead32 ();
> + //
> + // Open the Qemu Kernel Loader abstract filesystem (volume) which will be
> + // used to query the "initrd" and to read the "cmdline" synthetic files.
> + //
> + DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *)&mQemuKernelLoaderFsDevicePath;
> + Status = gBS->LocateDevicePath (
> + &gEfiSimpleFileSystemProtocolGuid,
> + &DevicePathNode,
> + &FsVolumeHandle
> + );
> + if (EFI_ERROR (Status)) {
> + goto UnloadImage;
> + }
> +
> + Status = gBS->HandleProtocol (
> + FsVolumeHandle,
> + &gEfiSimpleFileSystemProtocolGuid,
> + (VOID **)&FsProtocol
> + );
> + if (EFI_ERROR (Status)) {
> + goto UnloadImage;
> + }
> +
> + Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
> + if (EFI_ERROR (Status)) {
> + goto UnloadImage;
> + }
> +
> + Status = GetQemuKernelLoaderBlobSize (Root, L"cmdline", &CommandLineSize);
> + if (EFI_ERROR (Status)) {
> + goto CloseRoot;
> + }
>
> if (CommandLineSize == 0) {
> KernelLoadedImage->LoadOptionsSize = 0;
> @@ -133,11 +249,14 @@ QemuLoadKernelImage (
> CommandLine = AllocatePool (CommandLineSize);
> if (CommandLine == NULL) {
> Status = EFI_OUT_OF_RESOURCES;
> - goto UnloadImage;
> + goto CloseRoot;
> }
>
> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
> - QemuFwCfgReadBytes (CommandLineSize, CommandLine);
> + Status = ReadWholeQemuKernelLoaderBlob (Root, L"cmdline", CommandLineSize,
> + CommandLine);
> + if (EFI_ERROR (Status)) {
> + goto FreeCommandLine;
> + }
>
> //
> // Verify NUL-termination of the command line.
> @@ -155,8 +274,10 @@ QemuLoadKernelImage (
> KernelLoadedImage->LoadOptionsSize = (UINT32)((CommandLineSize - 1) * 2);
> }
>
> - QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
> - InitrdSize = (UINTN)QemuFwCfgRead32 ();
> + Status = GetQemuKernelLoaderBlobSize (Root, L"initrd", &InitrdSize);
> + if (EFI_ERROR (Status)) {
> + goto FreeCommandLine;
> + }
>
> if (InitrdSize > 0) {
> //
> @@ -199,6 +320,8 @@ FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> +CloseRoot:
> + Root->Close (Root);
> UnloadImage:
> if (EFI_ERROR (Status)) {
> gBS->UnloadImage (KernelImageHandle);
>
using an aarch64 guest,
Tested-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77264): https://edk2.groups.io/g/devel/message/77264
Mute This Topic: https://groups.io/mt/83841916/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
ground to add a warning about the incompatibility with boot verification
process.
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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 3 +++
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
index e1615badd2ba..c7ec041cb706 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
@@ -2,6 +2,9 @@
# X86 specific implementation of QemuLoadImageLib library class interface
# with support for loading mixed mode images and non-EFI stub images
#
+# Note that this implementation reads the cmdline (and possibly kernel, setup
+# data, and initrd in the legacy boot mode) from fw_cfg directly.
+#
# Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
index 6b1e7e649014..9f30df29736d 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
@@ -2,6 +2,9 @@
X86 specific implementation of QemuLoadImageLib library class interface
with support for loading mixed mode images and non-EFI stub images
+ Note that this implementation reads the cmdline (and possibly kernel, setup
+ data, and initrd in the legacy boot mode) from fw_cfg directly.
+
Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77177): https://edk2.groups.io/g/devel/message/77177
Mute This Topic: https://groups.io/mt/83841918/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 06/28/21 12:51, Dov Murik wrote: > Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the > ground to add a warning about the incompatibility with boot verification > process. > > 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> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457 > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > --- > OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 3 +++ > OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf > index e1615badd2ba..c7ec041cb706 100644 > --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf > +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf > @@ -2,6 +2,9 @@ > # X86 specific implementation of QemuLoadImageLib library class interface > # with support for loading mixed mode images and non-EFI stub images > # > +# Note that this implementation reads the cmdline (and possibly kernel, setup > +# data, and initrd in the legacy boot mode) from fw_cfg directly. > +# > # Copyright (c) 2020, ARM Ltd. All rights reserved.<BR> > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > index 6b1e7e649014..9f30df29736d 100644 > --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > @@ -2,6 +2,9 @@ > X86 specific implementation of QemuLoadImageLib library class interface > with support for loading mixed mode images and non-EFI stub images > > + Note that this implementation reads the cmdline (and possibly kernel, setup > + data, and initrd in the legacy boot mode) from fw_cfg directly. > + > Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> > Copyright (c) 2020, ARM Ltd. All rights reserved.<BR> > > Thanks for the updates. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77261): https://edk2.groups.io/g/devel/message/77261 Mute This Topic: https://groups.io/mt/83841918/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.