In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
of the kernel/initrd/cmdline from the QEMU fw_cfg interface. Insert a
call to VerifyBlob after fetching to allow BlobVerifierLib
implementations to add a verification step for these blobs.
This will allow confidential computing OVMF builds to add verification
mechanisms for these blobs that originate from an untrusted source
(QEMU).
The null implementation of BlobVerifierLib does nothing in VerifyBlob,
and therefore no functional change is expected.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index c7ddd86f5c75..b43330d23b80 100644
--- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
+++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
@@ -17,6 +17,7 @@
#include <Guid/QemuKernelLoaderFsMedia.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
+#include <Library/BlobVerifierLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/MemoryAllocationLib.h>
@@ -1039,6 +1040,14 @@ QemuKernelLoaderFsDxeEntrypoint (
if (EFI_ERROR (Status)) {
goto FreeBlobs;
}
+ Status = VerifyBlob (
+ CurrentBlob->Name,
+ CurrentBlob->Data,
+ CurrentBlob->Size
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBlobs;
+ }
mTotalBlobBytes += CurrentBlob->Size;
}
KernelBlob = &mKernelBlob[KernelBlobTypeKernel];
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77509): https://edk2.groups.io/g/devel/message/77509
Mute This Topic: https://groups.io/mt/84016359/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 7/6/21 3:54 AM, Dov Murik wrote: > In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content > of the kernel/initrd/cmdline from the QEMU fw_cfg interface. Insert a > call to VerifyBlob after fetching to allow BlobVerifierLib > implementations to add a verification step for these blobs. > > This will allow confidential computing OVMF builds to add verification > mechanisms for these blobs that originate from an untrusted source > (QEMU). > > The null implementation of BlobVerifierLib does nothing in VerifyBlob, > and therefore no functional change is expected. > > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Ashish Kalra <ashish.kalra@amd.com> > Cc: Brijesh Singh <brijesh.singh@amd.com> > Cc: Erdem Aktas <erdemaktas@google.com> > Cc: James Bottomley <jejb@linux.ibm.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Min Xu <min.m.xu@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457 > Co-developed-by: James Bottomley <jejb@linux.ibm.com> > Signed-off-by: James Bottomley <jejb@linux.ibm.com> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> The patch itself is okay. Just curious, do we also need to add a verification for the QEMU FW cfg file ? -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77862): https://edk2.groups.io/g/devel/message/77862 Mute This Topic: https://groups.io/mt/84016359/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 18/07/2021 18:47, Brijesh Singh wrote: > > On 7/6/21 3:54 AM, Dov Murik wrote: >> In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content >> of the kernel/initrd/cmdline from the QEMU fw_cfg interface. Insert a >> call to VerifyBlob after fetching to allow BlobVerifierLib >> implementations to add a verification step for these blobs. >> >> This will allow confidential computing OVMF builds to add verification >> mechanisms for these blobs that originate from an untrusted source >> (QEMU). >> >> The null implementation of BlobVerifierLib does nothing in VerifyBlob, >> and therefore no functional change is expected. >> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Ashish Kalra <ashish.kalra@amd.com> >> Cc: Brijesh Singh <brijesh.singh@amd.com> >> Cc: Erdem Aktas <erdemaktas@google.com> >> Cc: James Bottomley <jejb@linux.ibm.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Min Xu <min.m.xu@intel.com> >> Cc: Tom Lendacky <thomas.lendacky@amd.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457 >> Co-developed-by: James Bottomley <jejb@linux.ibm.com> >> Signed-off-by: James Bottomley <jejb@linux.ibm.com> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > > The patch itself is okay. Just curious, do we also need to add a > verification for the QEMU FW cfg file ? > I don't really understand. This patch adds the VerifyBlob() call on blobs that were read by FetchBlob(), which in turn reads the contents of kernel/initrd/cmdline from QEMU FW cfg (using QemuFwCfgReadBytes for example). We currently *don't* add verification for all other FW cfg settings, like number of CPUs, E820 memory entries, ... similar to what we (don't) do in SEV boot with encrypted root image (in which only OVMF is measured). What else do you think we should verify? -Dov -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77892): https://edk2.groups.io/g/devel/message/77892 Mute This Topic: https://groups.io/mt/84016359/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 7/19/21 7:22 AM, Dov Murik wrote: >> The patch itself is okay. Just curious, do we also need to add a >> verification for the QEMU FW cfg file ? >> > > I don't really understand. This patch adds the VerifyBlob() call on > blobs that were read by FetchBlob(), which in turn reads the contents of > kernel/initrd/cmdline from QEMU FW cfg (using QemuFwCfgReadBytes for > example). > > We currently *don't* add verification for all other FW cfg settings, > like number of CPUs, E820 memory entries, ... similar to what we (don't) > do in SEV boot with encrypted root image (in which only OVMF is measured). > > What else do you think we should verify? > As I understand that your series is attempting to add more security checks in the SEV boot sequence; i.e. after this series is merged, we can verify the kernel,cmdline and initrd passed through qemu. But there are several other configuration parameters (such as e820, acpi) that gets passed by the qemu and consumed by the ovmf. Are you considering to add the checks to cover those blobs in the future series? To me it seems that the framework built here can be extended to cover those as well. Reviewed-by: Brijesh Singh <brijesh.singh@amd.com> thanks! -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77900): https://edk2.groups.io/g/devel/message/77900 Mute This Topic: https://groups.io/mt/84016359/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 19/07/2021 18:19, Brijesh Singh wrote: > > > On 7/19/21 7:22 AM, Dov Murik wrote: >>> The patch itself is okay. Just curious, do we also need to add a >>> verification for the QEMU FW cfg file ? >>> >> >> I don't really understand. This patch adds the VerifyBlob() call on >> blobs that were read by FetchBlob(), which in turn reads the contents of >> kernel/initrd/cmdline from QEMU FW cfg (using QemuFwCfgReadBytes for >> example). >> >> We currently *don't* add verification for all other FW cfg settings, >> like number of CPUs, E820 memory entries, ... similar to what we (don't) >> do in SEV boot with encrypted root image (in which only OVMF is >> measured). >> >> What else do you think we should verify? >> > > As I understand that your series is attempting to add more security > checks in the SEV boot sequence; i.e. after this series is merged, we > can verify the kernel,cmdline and initrd passed through qemu. But there > are several other configuration parameters (such as e820, acpi) that > gets passed by the qemu and consumed by the ovmf. Are you considering to > add the checks to cover those blobs in the future series? To me it seems > that the framework built here can be extended to cover those as well. > You're right -- it can be extended. Currently that's not the plan; the Guest Owner should be able to verify the measurement, which, with this patch series, is a combination of the OVMF, kernel, initrd, and cmdline. Adding the other QEMU FW CFG values will make that even harder for the Guest Owner. Also, the measurement will be different if, for example, the guest is launched with 8GB memory instead of 4GB, or with 8 vcpus instead of 4 vcpus. If there's an obvious attack possible via one of those fw_cfg settings, we can think how to extend the measurement to cover the problematic settings (or not support them at all, if possible). > Reviewed-by: Brijesh Singh <brijesh.singh@amd.com> > Thanks! -Dov -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77921): https://edk2.groups.io/g/devel/message/77921 Mute This Topic: https://groups.io/mt/84016359/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 7/6/21 3:54 AM, Dov Murik wrote:
> In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
> of the kernel/initrd/cmdline from the QEMU fw_cfg interface. Insert a
> call to VerifyBlob after fetching to allow BlobVerifierLib
> implementations to add a verification step for these blobs.
>
> This will allow confidential computing OVMF builds to add verification
> mechanisms for these blobs that originate from an untrusted source
> (QEMU).
>
> The null implementation of BlobVerifierLib does nothing in VerifyBlob,
> and therefore no functional change is expected.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index c7ddd86f5c75..b43330d23b80 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -17,6 +17,7 @@
> #include <Guid/QemuKernelLoaderFsMedia.h>
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> +#include <Library/BlobVerifierLib.h>
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> #include <Library/MemoryAllocationLib.h>
> @@ -1039,6 +1040,14 @@ QemuKernelLoaderFsDxeEntrypoint (
> if (EFI_ERROR (Status)) {
> goto FreeBlobs;
> }
> + Status = VerifyBlob (
> + CurrentBlob->Name,
> + CurrentBlob->Data,
> + CurrentBlob->Size
> + );
Just a nit, but the ");" should be under the "C" in CurrentBlob.
Thanks,
Tom
> + if (EFI_ERROR (Status)) {
> + goto FreeBlobs;
> + }
> mTotalBlobBytes += CurrentBlob->Size;
> }
> KernelBlob = &mKernelBlob[KernelBlobTypeKernel];
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77906): https://edk2.groups.io/g/devel/message/77906
Mute This Topic: https://groups.io/mt/84016359/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 19/07/2021 18:57, Tom Lendacky wrote:
> On 7/6/21 3:54 AM, Dov Murik wrote:
>> In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
>> of the kernel/initrd/cmdline from the QEMU fw_cfg interface. Insert a
>> call to VerifyBlob after fetching to allow BlobVerifierLib
>> implementations to add a verification step for these blobs.
>>
>> This will allow confidential computing OVMF builds to add verification
>> mechanisms for these blobs that originate from an untrusted source
>> (QEMU).
>>
>> The null implementation of BlobVerifierLib does nothing in VerifyBlob,
>> and therefore no functional change is expected.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>> OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>> index c7ddd86f5c75..b43330d23b80 100644
>> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>> @@ -17,6 +17,7 @@
>> #include <Guid/QemuKernelLoaderFsMedia.h>
>> #include <Library/BaseLib.h>
>> #include <Library/BaseMemoryLib.h>
>> +#include <Library/BlobVerifierLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/DevicePathLib.h>
>> #include <Library/MemoryAllocationLib.h>
>> @@ -1039,6 +1040,14 @@ QemuKernelLoaderFsDxeEntrypoint (
>> if (EFI_ERROR (Status)) {
>> goto FreeBlobs;
>> }
>> + Status = VerifyBlob (
>> + CurrentBlob->Name,
>> + CurrentBlob->Data,
>> + CurrentBlob->Size
>> + );
>
> Just a nit, but the ");" should be under the "C" in CurrentBlob.
It's a sad winking face... I'll fix.
Thanks,
-Dov
>
> Thanks,
> Tom
>
>> + if (EFI_ERROR (Status)) {
>> + goto FreeBlobs;
>> + }
>> mTotalBlobBytes += CurrentBlob->Size;
>> }
>> KernelBlob = &mKernelBlob[KernelBlobTypeKernel];
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77918): https://edk2.groups.io/g/devel/message/77918
Mute This Topic: https://groups.io/mt/84016359/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.