Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Yao, Jiewen]

Thanks for the patch.

Is this the only missing configuration data?
Or do you have more on the way?



> -----Original Message-----
> From: Sun, CepingX <cepingx.sun@intel.com>
> Sent: Wednesday, March 13, 2024 7:52 AM
> To: devel@edk2.groups.io
> Cc: Sun, CepingX <cepingx.sun@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M
> <min.m.xu@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Reshetova, Elena
> <elena.reshetova@intel.com>
> Subject: [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-
> menu-wait
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4415
> 
> Refer to the section 8.3.4 of tdx-virtual-firmware-design-guide spec,
> OVMF would uses FW_CFG_IO_SELECTOR(0x510) and FW_CFG_IO_DATA(0x511)
> to get configuration data from QEMU. From the security perspective,
> if TDVF uses this method, configuration data must be measured into
> RTMR[0].
> 
> Currently, the etc/boot-menu-wait is using in TDVF, it required to be
> measured into RTMR[0].
> 
> This is the first patch and will continue to be updated to measure
> additional configuration data.
> 
> Refernce:
> spec: https://cdrdv2.intel.com/v1/dl/getContent/733585
> 
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Ceping Sun <cepingx.sun@intel.com>
> ---
>  .../QemuBootOrderLib/QemuBootOrderLib.c       | 21 ++++++++++++++++++-
>  .../QemuBootOrderLib/QemuBootOrderLib.inf     |  1 +
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> index 2fe6ab30c032..63a290712002 100644
> --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> @@ -20,6 +20,8 @@
>  #include <Library/BaseMemoryLib.h>
>  #include <Guid/GlobalVariable.h>
>  #include <Guid/VirtioMmioTransport.h>
> +#include <IndustryStandard/UefiTcgPlatform.h>
> +#include <Library/TpmMeasurementLib.h>
> 
>  #include "ExtraRootBusMap.h"
> 
> @@ -41,6 +43,9 @@
>  #define REQUIRED_MMIO_OFW_NODES  1
>  #define EXAMINED_OFW_NODES       6
> 
> +#define EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA  "QEMU
> BOOTMENU WAIT TIME"
> +#define QEMU_BOOTMENU_WAIT_DATA_LEN
> (sizeof(EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA) - 1)
> +
>  /**
>    Simple character classification routines, corresponding to POSIX class names
>    and ASCII encoding.
> @@ -2418,5 +2423,19 @@ GetFrontPageTimeoutFromQemu (
>    // seconds, round N up.
>    //
>    QemuFwCfgSelectItem (BootMenuWaitItem);
> -  return (UINT16)((QemuFwCfgRead16 () + 999) / 1000);
> +  Timeout = QemuFwCfgRead16 ();
> +  //
> +  // Measure the Timeout which is downloaded from QEMU.
> +  // It has to be done before it is consumed.
> +  //
> +  TpmMeasureAndLogData (
> +    1,
> +    EV_PLATFORM_CONFIG_FLAGS,
> +    EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA,
> +    QEMU_BOOTMENU_WAIT_DATA_LEN,
> +    (VOID *)(UINTN)&Timeout,
> +    BootMenuWaitSize
> +    );
> +
> +  return (UINT16)((Timeout + 999) / 1000);
>  }
> diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> index 6e320e3e8514..0231c9d5c5b8 100644
> --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> @@ -45,6 +45,7 @@
>    DevicePathLib
>    BaseMemoryLib
>    OrderedCollectionLib
> +  TpmMeasurementLib
> 
>  [Guids]
>    gEfiGlobalVariableGuid
> --
> 2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116670): https://edk2.groups.io/g/devel/message/116670
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Wed, Mar 13, 2024 at 07:51:46AM +0800, Ceping Sun wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4415
> 
> Refer to the section 8.3.4 of tdx-virtual-firmware-design-guide spec,
> OVMF would uses FW_CFG_IO_SELECTOR(0x510) and FW_CFG_IO_DATA(0x511)
> to get configuration data from QEMU. From the security perspective,
> if TDVF uses this method, configuration data must be measured into
> RTMR[0].
> 
> Currently, the etc/boot-menu-wait is using in TDVF, it required to be
> measured into RTMR[0].

That config item doesn't change the control flow.
Do we have to measure it?

> This is the first patch and will continue to be updated to measure
> additional configuration data.

What else is in the pipeline?  At least ACPI and smbios tables I assume?

I'd like to have a more complete picture first.  Also I think it makes
sense to have a single patch series implementing all of it instead of
merging it piece by piece, to avoid having multiple edk2 releases where
the measurements are changing.

Note that the current code (looking at a non-tdx build) reads several
fw_cfg items multiple times.  Entries 0 and 1 (used for probing fw_cfg
presence), 0x19 (file directory) are read most frequently.  etc/e820 is
scanned multiple times too; tvdf in tdx mode wouldn't use it though.

If we are going to measure the fw_cfg bits used by ovmf / tdvf I think
we have to:

  (1) Make sure we read + measure the data once.
  (2) Make sure we measure the fw_cfg entries in a deterministic
      order so the measurements are stable.
  (3) Cache the measured data somewhere if needed multiple times
      (or simply cache unconditionally).

We probably wouldn't measure all fw_cfg entries.  The ones used by
direct kernel boot can be skipped for example.  The kernel image will
be measured anyway before it is launched.

> +#define EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA  "QEMU BOOTMENU WAIT TIME"

"QEMU FW CFG" ?

I think it makes sense to have one name and one struct for all qemu
fw_cfg items.  Or maybe two, one for the file-name based entries and
one for the others.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116677): https://edk2.groups.io/g/devel/message/116677
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[sunceping]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4415

Refer to the section 8.3.4 of tdx-virtual-firmware-design-guide spec,
OVMF would uses FW_CFG_IO_SELECTOR(0x510) and FW_CFG_IO_DATA(0x511)
to get configuration data from QEMU. From the security perspective,
if TDVF uses this method, configuration data must be measured into
RTMR[0].

Currently, the etc/boot-menu-wait is using in TDVF, it required to be
measured into RTMR[0].

This is the first patch and will continue to be updated to measure
additional configuration data.

Refernce:
spec: https://cdrdv2.intel.com/v1/dl/getContent/733585

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Ceping Sun <cepingx.sun@intel.com>
---
 .../QemuBootOrderLib/QemuBootOrderLib.c       | 21 ++++++++++++++++++-
 .../QemuBootOrderLib/QemuBootOrderLib.inf     |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
index 2fe6ab30c032..63a290712002 100644
--- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
+++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
@@ -20,6 +20,8 @@
 #include <Library/BaseMemoryLib.h>
 #include <Guid/GlobalVariable.h>
 #include <Guid/VirtioMmioTransport.h>
+#include <IndustryStandard/UefiTcgPlatform.h>
+#include <Library/TpmMeasurementLib.h>
 
 #include "ExtraRootBusMap.h"
 
@@ -41,6 +43,9 @@
 #define REQUIRED_MMIO_OFW_NODES  1
 #define EXAMINED_OFW_NODES       6
 
+#define EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA  "QEMU BOOTMENU WAIT TIME"
+#define QEMU_BOOTMENU_WAIT_DATA_LEN                    (sizeof(EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA) - 1)
+
 /**
   Simple character classification routines, corresponding to POSIX class names
   and ASCII encoding.
@@ -2418,5 +2423,19 @@ GetFrontPageTimeoutFromQemu (
   // seconds, round N up.
   //
   QemuFwCfgSelectItem (BootMenuWaitItem);
-  return (UINT16)((QemuFwCfgRead16 () + 999) / 1000);
+  Timeout = QemuFwCfgRead16 ();
+  //
+  // Measure the Timeout which is downloaded from QEMU.
+  // It has to be done before it is consumed.
+  //
+  TpmMeasureAndLogData (
+    1,
+    EV_PLATFORM_CONFIG_FLAGS,
+    EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA,
+    QEMU_BOOTMENU_WAIT_DATA_LEN,
+    (VOID *)(UINTN)&Timeout,
+    BootMenuWaitSize
+    );
+
+  return (UINT16)((Timeout + 999) / 1000);
 }
diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
index 6e320e3e8514..0231c9d5c5b8 100644
--- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
+++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
@@ -45,6 +45,7 @@
   DevicePathLib
   BaseMemoryLib
   OrderedCollectionLib
+  TpmMeasurementLib
 
 [Guids]
   gEfiGlobalVariableGuid
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116669): https://edk2.groups.io/g/devel/message/116669
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[sunceping]

On Tuesday, March 12, 2024 3:58 PM Yao, Jiewen wrote:
> Subject: RE: [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the 
> etc/boot-menu-wait
> 
> Thanks for the patch.
> 
> Is this the only missing configuration data?
> Or do you have more on the way?
> 
This is not the only missing configuration data.
There are other configurations need to be measured.
We have a draft PR(https://github.com/tianocore/edk2/pull/5440)
 to measure the below items:
etc/system-states
opt/ovmf/X-PciMmio64Mb
etc/reserved-memory-end
etc/boot-menu-wait
etc./extra-pci-roots

According to Hoffmann's comments,  
we would prepare a single patch series with all measurements in next version.

Thanks
Ceping
> 
> > -----Original Message-----
> > From: Sun, CepingX <cepingx.sun@intel.com>
> > Sent: Wednesday, March 13, 2024 7:52 AM
> > To: devel@edk2.groups.io
> > Cc: Sun, CepingX <cepingx.sun@intel.com>; Aktas, Erdem
> > <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min
> M
> > <min.m.xu@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Reshetova,
> > Elena <elena.reshetova@intel.com>
> > Subject: [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the
> > etc/boot- menu-wait
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4415
> >
> > Refer to the section 8.3.4 of tdx-virtual-firmware-design-guide spec,
> > OVMF would uses FW_CFG_IO_SELECTOR(0x510) and
> FW_CFG_IO_DATA(0x511) to
> > get configuration data from QEMU. From the security perspective, if
> > TDVF uses this method, configuration data must be measured into
> > RTMR[0].
> >
> > Currently, the etc/boot-menu-wait is using in TDVF, it required to be
> > measured into RTMR[0].
> >
> > This is the first patch and will continue to be updated to measure
> > additional configuration data.
> >
> > Refernce:
> > spec: https://cdrdv2.intel.com/v1/dl/getContent/733585
> >
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Min Xu <min.m.xu@intel.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Ceping Sun <cepingx.sun@intel.com>
> > ---
> >  .../QemuBootOrderLib/QemuBootOrderLib.c       | 21
> ++++++++++++++++++-
> >  .../QemuBootOrderLib/QemuBootOrderLib.inf     |  1 +
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> > b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> > index 2fe6ab30c032..63a290712002 100644
> > --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> > +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> > @@ -20,6 +20,8 @@
> >  #include <Library/BaseMemoryLib.h>
> >  #include <Guid/GlobalVariable.h>
> >  #include <Guid/VirtioMmioTransport.h>
> > +#include <IndustryStandard/UefiTcgPlatform.h>
> > +#include <Library/TpmMeasurementLib.h>
> >
> >  #include "ExtraRootBusMap.h"
> >
> > @@ -41,6 +43,9 @@
> >  #define REQUIRED_MMIO_OFW_NODES  1
> >  #define EXAMINED_OFW_NODES       6
> >
> > +#define EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA
> "QEMU
> > BOOTMENU WAIT TIME"
> > +#define QEMU_BOOTMENU_WAIT_DATA_LEN
> > (sizeof(EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA) - 1)
> > +
> >  /**
> >    Simple character classification routines, corresponding to POSIX class
> names
> >    and ASCII encoding.
> > @@ -2418,5 +2423,19 @@ GetFrontPageTimeoutFromQemu (
> >    // seconds, round N up.
> >    //
> >    QemuFwCfgSelectItem (BootMenuWaitItem);
> > -  return (UINT16)((QemuFwCfgRead16 () + 999) / 1000);
> > +  Timeout = QemuFwCfgRead16 ();
> > +  //
> > +  // Measure the Timeout which is downloaded from QEMU.
> > +  // It has to be done before it is consumed.
> > +  //
> > +  TpmMeasureAndLogData (
> > +    1,
> > +    EV_PLATFORM_CONFIG_FLAGS,
> > +    EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA,
> > +    QEMU_BOOTMENU_WAIT_DATA_LEN,
> > +    (VOID *)(UINTN)&Timeout,
> > +    BootMenuWaitSize
> > +    );
> > +
> > +  return (UINT16)((Timeout + 999) / 1000);
> >  }
> > diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> > b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> > index 6e320e3e8514..0231c9d5c5b8 100644
> > --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> > +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> > @@ -45,6 +45,7 @@
> >    DevicePathLib
> >    BaseMemoryLib
> >    OrderedCollectionLib
> > +  TpmMeasurementLib
> >
> >  [Guids]
> >    gEfiGlobalVariableGuid
> > --
> > 2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116710): https://edk2.groups.io/g/devel/message/116710
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[sunceping]

On Tuesday, March 12, 2024 7:04 PM Gerd Hoffmann wrote:
> On Wed, Mar 13, 2024 at 07:51:46AM +0800, Ceping Sun wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4415
> >
> > Refer to the section 8.3.4 of tdx-virtual-firmware-design-guide 
> > spec, OVMF would uses FW_CFG_IO_SELECTOR(0x510) and
> FW_CFG_IO_DATA(0x511) to
> > get configuration data from QEMU. From the security perspective, if 
> > TDVF uses this method, configuration data must be measured into 
> > RTMR[0].
> >
> > Currently, the etc/boot-menu-wait is using in TDVF, it required to 
> > be measured into RTMR[0].
> 
> That config item doesn't change the control flow.
> Do we have to measure it?
> 
 For TD-Guest, VMM is out of TCB, the configuration is untrusted data.
 From the security perspective, it must be measured into RTMR[0]

> > This is the first patch and will continue to be updated to measure 
> > additional configuration data.
> 
> What else is in the pipeline?  At least ACPI and smbios tables I assume?
> 
The ACPI tables from QEMU has been measured in edk2 .
There are detail message : https://edk2.groups.io/g/devel/message/99441

For smbios tables, we would double check it and update in next version.

> I'd like to have a more complete picture first.  Also I think it makes 
> sense to have a single patch series implementing all of it instead of 
> merging it piece by piece, to avoid having multiple edk2 releases 
> where the measurements are changing.
Yes , that's good idea. 
We would prepare the patch series in next version.

> 
> Note that the current code (looking at a non-tdx build) reads several 
> fw_cfg items multiple times.  Entries 0 and 1 (used for probing fw_cfg 
> presence), 0x19 (file directory) are read most frequently.  etc/e820 
> is scanned multiple times too; tvdf in tdx mode wouldn't use it though.
For etc/e820 , it is used in TD-Guest,  PlatformInfoHob->LowMemory would be updated with the low memory size now.

> If we are going to measure the fw_cfg bits used by ovmf / tdvf I think 
> we have
> to:
> 
>   (1) Make sure we read + measure the data once.
Yes,  agree.

>   (2) Make sure we measure the fw_cfg entries in a deterministic
>       order so the measurements are stable.
Yes,  agree.

>   (3) Cache the measured data somewhere if needed multiple times
>       (or simply cache unconditionally).
> 
Yes, agree.
Cache the measured data into HOB in the PEI phase 
and cache the measured data into the global variables in the DXE phase.
How about this?

> We probably wouldn't measure all fw_cfg entries.  The ones used by 
> direct kernel boot can be skipped for example.  The kernel image will 
> be measured anyway before it is launched.
Yes,  agree.

> 
> > +#define EV_POSTCODE_INFO_QEMU_BOOTMENU_WAIT_TIME_DATA
> "QEMU BOOTMENU WAIT TIME"
> 
> "QEMU FW CFG" ?
> 
> I think it makes sense to have one name and one struct for all qemu 
> fw_cfg items.  Or maybe two, one for the file-name based entries and 
> one for the others.
Yes,  we would update in next version.

Thanks
Ceping


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116711): https://edk2.groups.io/g/devel/message/116711
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

  Hi,

> >   (3) Cache the measured data somewhere if needed multiple times
> >       (or simply cache unconditionally).
> > 
> Yes, agree.
> Cache the measured data into HOB in the PEI phase 
> and cache the measured data into the global variables in the DXE phase.
> How about this?

Load, measure and cache all fw_cfg entries we care about early in the
PEI phase (or SEC phase for pei-less builds), so we can
 (a) easily have a fixed order, and
 (b) store them all in HOBs?

Which implies SEC/PEI must read all relevant fw_cfg entries, even in
case they are used only later in the DXE phase.

Advantage is we have a single cache which can be used in all firmware
phases.  When using global variables in DXE we still can end up reading
entries multiple times, either because entries are needed by both PEI
and DXE, or because multiple DXE modules need them (global variables are
per module).

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116734): https://edk2.groups.io/g/devel/message/116734
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[sunceping]

On Thursday, March 14, 2024 5:31 PM Gerd Hoffmann wrote:
> Load, measure and cache all fw_cfg entries we care about early in the PEI phase
> (or SEC phase for pei-less builds), so we can
>  (a) easily have a fixed order, and
>  (b) store them all in HOBs?
> 
> Which implies SEC/PEI must read all relevant fw_cfg entries, even in case they
> are used only later in the DXE phase.
> 
> Advantage is we have a single cache which can be used in all firmware phases.
> When using global variables in DXE we still can end up reading entries multiple
> times, either because entries are needed by both PEI and DXE, or because
> multiple DXE modules need them (global variables are per module).
> 
We have some concerns that in above solution the size of a single cache is large
( because all fw_cfg data need to be read from qemu). 
Further more this is not a lazy mode which means some fw_cfg data 
may be read but it is not to be consumed later. 

We propose below solution :

Add a new API in QemuFwCfgLib,
RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value, FW_CFG_GET_DATA_FLAG flag).	

Input:
@fw_cfg_name: The fw_cfg file name .
@size: Number of bytes in the file.
@value: Value of fw_cfg item read. 
@flag: Bitmap providing additional information(cache?  measurement ?)
             #define  FW_CFG_GET_DATA_CACHE 0x00000001
             #define  FW_CFG_GET_DATA_MEASUREMENT 0x00000002

Output:
@return: RETURN_NOT_FOUND: The fw_cfg file is not found
	   RETURN_UNSUPPORTED: The fw_cfg is unavailable.
                 RETURN_BUFFER_TOO_SMALL: The required buffer size is in @size.
                 RETURN_SUCCESS:  The fw_cfg file is found and load the data successfully.

Description:
 This function finds fw_cfg item and reads its data into value.  And it also provides callers with
 options to cache and/or measurement. 

Pros:
Caller can decide how to get the fw_cfg data from QEMU (cache? Measurement?)
 And it can reduce the code complexity because it pack 3 fw_cfg call 
(QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) into one call. 

What's your thought?


Thanks
Ceping


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116915): https://edk2.groups.io/g/devel/message/116915
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Wed, Mar 20, 2024 at 08:41:41AM +0000, Sun, CepingX wrote:
> 
> On Thursday, March 14, 2024 5:31 PM Gerd Hoffmann wrote:
> > Load, measure and cache all fw_cfg entries we care about early in the PEI phase
> > (or SEC phase for pei-less builds), so we can
> >  (a) easily have a fixed order, and
> >  (b) store them all in HOBs?
> > 
> > Which implies SEC/PEI must read all relevant fw_cfg entries, even in case they
> > are used only later in the DXE phase.
> > 
> > Advantage is we have a single cache which can be used in all firmware phases.
> > When using global variables in DXE we still can end up reading entries multiple
> > times, either because entries are needed by both PEI and DXE, or because
> > multiple DXE modules need them (global variables are per module).
> > 
> We have some concerns that in above solution the size of a single cache is large
> ( because all fw_cfg data need to be read from qemu). 

We don't need to read + cache all fw_cfg data.  We only need to cache
the entries which (a) must be measured, and (b) will not be measured in
some other way.

The big entries I'm aware of are:
 (1) kernel for direct boot (no fw_cfg measurement needed, binary
     will be measured before launch).
 (2) ACPI tables (no fw_cfg measurement needed, acpi tables are
     measured when installed).
 (3) smbios tables (to be decided, could be handled similar to ACPI).

Anything else where you have size concerns?

> Further more this is not a lazy mode which means some fw_cfg data 
> may be read but it is not to be consumed later. 

The problem with lazy mode is that the measurement order is not fixed.

> We propose below solution :
> 
> Add a new API in QemuFwCfgLib,
> RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value, FW_CFG_GET_DATA_FLAG flag).	

I'd suggest to *not* touch the existing interfaces for reading entries.
Instead change the existing functions to first check the cache, in case
there is no cache entry go read from fw_cfg.

Add a new interface for adding fw_cfg entries to the cache, with
optional measurement.

Populate the cache with all entries which need fw_cfg measurement.

Additionally cache entries which are used frequently
(QemuFwCfgItemSignature + QemuFwCfgItemInterfaceVersion +
QemuFwCfgItemFileDir are candidates).

Note that QemuFwCfgItemFileDir must be cached for security reasons (so
the hypervisor can't present different versions of the file list) even
though I don't think we need to measure it.

> Pros:
> Caller can decide how to get the fw_cfg data from QEMU (cache? Measurement?)

Cache and measurement are not independent options.  Measurement without
caching doesn't make sense.

>  And it can reduce the code complexity because it pack 3 fw_cfg call 
> (QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) into one call. 

There are a few places in the code which expect they can read fw_cfg
files in multiple chunks (i.e. call QemuFwCfgReadBytes multiple times).

Also note that not all fw_cfg entries are (pseudo-)files.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116932): https://edk2.groups.io/g/devel/message/116932
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[sunceping]

On Wednesday, March 20, 2024 6:05 PM Gerd Hoffmann wrote:
> 
> We don't need to read + cache all fw_cfg data.  We only need to cache the
> entries which (a) must be measured, and (b) will not be measured in some
> other way.
> 
I am afraid that it is difficult to determine which fw_cfg items 
are need to be cached and measured, since there are some fw_cfg items are
optional with special qemu cmd.
Example : 
fw_cfg item: opt/ovmf/X-PciMmio64Mb  
depends on qemu command: "-fw_cfg name=opt/ovmf/X-PciMmio64Mb,string=N"

> The big entries I'm aware of are:
>  (1) kernel for direct boot (no fw_cfg measurement needed, binary
>      will be measured before launch).
>  (2) ACPI tables (no fw_cfg measurement needed, acpi tables are
>      measured when installed).
>  (3) smbios tables (to be decided, could be handled similar to ACPI).
> 
> Anything else where you have size concerns?
So far I don't find other big entries.

> 
> > Further more this is not a lazy mode which means some fw_cfg data may
> > be read but it is not to be consumed later.
> 
> The problem with lazy mode is that the measurement order is not fixed.
We'd better define what's the "fixed measurement order".
Think about such scenario:
There are 5 fw_cfg items ("A-B-C-D-E"), and C is an optional one which depends on special qemu cmd
 (example : etc/boot-menu-wait depends on qemu command: " -boot menu=on,splash-time=N").
	
So, there are two measurement orders in different qemu command:
1, "A-B-C-D-E" (with qemu command "-boot menu=on,splash-time=N ")
2, "A-B-D-E" (without qemu command "-boot menu=on,splash-time=N ") 

Is the "A-B-C-D-E" or "A-B-D-E"  fixed order ? 

What's your thoughts ?

> 
> > We propose below solution :
> >
> > Add a new API in QemuFwCfgLib,
> > RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value,
> FW_CFG_GET_DATA_FLAG flag).
> 
> I'd suggest to *not* touch the existing interfaces for reading entries.
> Instead change the existing functions to first check the cache, in case there is
> no cache entry go read from fw_cfg.
Actually we don't touch the existing interface for reading entries.
The API is newly added.

>
> Add a new interface for adding fw_cfg entries to the cache, with optional
> measurement.
Do you mean to add a new API (example : QemuFwCfgReadBytes2(*size, *buffer, Flag))
 with optional measurement to cache fw_cfg data?

> 
> >  And it can reduce the code complexity because it pack 3 fw_cfg call
> > (QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) into one
> call.
> 
> There are a few places in the code which expect they can read fw_cfg files in
> multiple chunks (i.e. call QemuFwCfgReadBytes multiple times).
> 
Actually we can handle the case with the new API.
For example, etc/e820 is read in chunks. 
Current code like below:
  QemuFwCfgSelectItem (FwCfgItem);
  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
    Callback (&E820Entry, PlatformInfoHob);
  }

We can update it like below:
  QemuFwCfgGetData("etc/e820", &FwCfgSize,  Buffer, Flag);
  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { 
    EFI_E820_ENTRY64 *E820Entry = (EFI_E820_ENTRY64 *)(Buffer + Processed);
    Callback (E820Entry, PlatformInfoHob);
  }

> Also note that not all fw_cfg entries are (pseudo-)files.
I am not sure what you mean about (pseudo-)files.

Could you give some instructions?

Base on your comments,  does this mean there are other fw_cfg data 
which are read from QEMU via different method (not QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) ?

If it is the case, could you please give an example? 

Thanks 
Ceping


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116950): https://edk2.groups.io/g/devel/message/116950
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Thu, Mar 21, 2024 at 08:39:13AM +0000, sunceping wrote:
> On Wednesday, March 20, 2024 6:05 PM Gerd Hoffmann wrote:
> > 
> > We don't need to read + cache all fw_cfg data.  We only need to cache the
> > entries which (a) must be measured, and (b) will not be measured in some
> > other way.
> > 
> I am afraid that it is difficult to determine which fw_cfg items 
> are need to be cached and measured, since there are some fw_cfg items are
> optional with special qemu cmd.
> Example : 
> fw_cfg item: opt/ovmf/X-PciMmio64Mb  
> depends on qemu command: "-fw_cfg name=opt/ovmf/X-PciMmio64Mb,string=N"

Well, just try to read them.  If present they can just be measured.
If not present we can either skip them, or measure with an empty data
field to indicate it is not present.

> > > Further more this is not a lazy mode which means some fw_cfg data may
> > > be read but it is not to be consumed later.
> > 
> > The problem with lazy mode is that the measurement order is not fixed.
> We'd better define what's the "fixed measurement order".
> Think about such scenario:
> There are 5 fw_cfg items ("A-B-C-D-E"), and C is an optional one which depends on special qemu cmd
>  (example : etc/boot-menu-wait depends on qemu command: " -boot menu=on,splash-time=N").
> 	
> So, there are two measurement orders in different qemu command:
> 1, "A-B-C-D-E" (with qemu command "-boot menu=on,splash-time=N ")
> 2, "A-B-D-E" (without qemu command "-boot menu=on,splash-time=N ") 
> 
> Is the "A-B-C-D-E" or "A-B-D-E"  fixed order ? 
> 
> What's your thoughts ?

See above for optional items.

I'm more concerned about reordering.  We have a number of DXE modules
reading fw_cfg entries.  The order the modules are scheduled depends
multiple factors (order in the firmware volume, depex), so that order
may change.

With lazy loading and measurement changes in the initialization order
would also change the measurements even if the content of the fw_cfg
items files is identical, because the items are measured in B-A instead
of A-B order.

> > > We propose below solution :
> > >
> > > Add a new API in QemuFwCfgLib,
> > > RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value,
> > FW_CFG_GET_DATA_FLAG flag).
> > 
> > I'd suggest to *not* touch the existing interfaces for reading entries.
> > Instead change the existing functions to first check the cache, in case there is
> > no cache entry go read from fw_cfg.
> Actually we don't touch the existing interface for reading entries.
> The API is newly added.

But then you have to find and update all callsites (or at least the
ones where we care about measurement).

> > Add a new interface for adding fw_cfg entries to the cache, with optional
> > measurement.
> Do you mean to add a new API (example : QemuFwCfgReadBytes2(*size, *buffer, Flag))
>  with optional measurement to cache fw_cfg data?

More like this:

QemuFwCfgCacheAdd(int Item, int Size, bool Measure);

> We can update it like below:
>   QemuFwCfgGetData("etc/e820", &FwCfgSize,  Buffer, Flag);
>   for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { 
>     EFI_E820_ENTRY64 *E820Entry = (EFI_E820_ENTRY64 *)(Buffer + Processed);
>     Callback (E820Entry, PlatformInfoHob);
>   }

Well, not that easy.  Allocating memory is not always possible in early
firmware phases, which is the reason why the code works with the stack
and chunkwise reads.

We might offer an QemuFwCfgCacheGet() API though.  When storing cached
items in HOBs we can hand out a pointer to the data in the HOB, and
callers access the complete fw_cfg item then without having to allocate
memory.

> > Also note that not all fw_cfg entries are (pseudo-)files.
> I am not sure what you mean about (pseudo-)files.

Well, even though fw_cfg uses names which look like file paths for
entries it's not a real filesystem, it's just a naming convention,
that's why named them pseudo files.

Some older fw_cfg items don't use this file-naming scheme, they have a
fixed number instead (see QemuFwCfgItem*).

> Base on your comments,  does this mean there are other fw_cfg data 
> which are read from QEMU via different method (not QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) ?

It all comes down to QemuFwCfgSelectItem + QemuFwCfgReadBytes.  There
are some convenience functions to read (for example) integers, they
internally call QemuFwCfgReadBytes though.  QemuFwCfgFindFile also
simply calls QemuFwCfgSelectItem + QemuFwCfgReadBytes.

So making QemuFwCfgSelectItem + QemuFwCfgReadBytes aware of the cache
should be all you need.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116959): https://edk2.groups.io/g/devel/message/116959
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[sunceping]

On Thursday, March 21, 2024 8:25 PM Gerd Hoffmann wrote:
> Well, just try to read them.  If present they can just be measured.
> If not present we can either skip them, or measure with an empty data 
> field to indicate it is not present.
My understanding :
If the fw_cfg is present,  it must be measured and consumed later.
Is this correct?

> > > > We propose below solution :
> > > >
> > > > Add a new API in QemuFwCfgLib,
> > > > RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value,
> > > FW_CFG_GET_DATA_FLAG flag).
> > >
> > > I'd suggest to *not* touch the existing interfaces for reading entries.
> > > Instead change the existing functions to first check the cache, in 
> > > case there is no cache entry go read from fw_cfg.
> > Actually we don't touch the existing interface for reading entries.
> > The API is newly added.
> 
> But then you have to find and update all callsites (or at least the 
> ones where we care about measurement).
In your solution,  if we cache all items that need to be measured,
we would have to add a new API (example: QemuFwCfgGetDataFromCache ())  to get the data from cache.

Then we also have to find and update all callsites.

Thanks
Ceping



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117028): https://edk2.groups.io/g/devel/message/117028
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Fri, Mar 22, 2024 at 08:29:28AM +0000, Sun, CepingX wrote:
> On Thursday, March 21, 2024 8:25 PM Gerd Hoffmann wrote:
> > Well, just try to read them.  If present they can just be measured.
> > If not present we can either skip them, or measure with an empty data 
> > field to indicate it is not present.
> My understanding :
> If the fw_cfg is present,  it must be measured and consumed later.
> Is this correct?

I think that would be the best strategy.  In SEC or early PEI read the
items, cache them in HOBs, optionally measure them.

When consumed just fetch them from the cache.  For entries we are
measuring caching is mandatory (to make sure we are actually using
the same data we have measured).

> > But then you have to find and update all callsites (or at least the 
> > ones where we care about measurement).
> In your solution,  if we cache all items that need to be measured,
> we would have to add a new API (example: QemuFwCfgGetDataFromCache ())  to get the data from cache.

No, we only need to update QemuFwCfgSelectItem + QemuFwCfgReadBytes to
support reading from the cache.

QemuFwCfgGetDataFromCache() can be added as additional API, and
callsites have the option to either switch over, or continue to
use the existing API.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117032): https://edk2.groups.io/g/devel/message/117032
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[sunceping]

On Friday, March 22, 2024 5:06 PM Gerd Hoffmann wrote:
> 
> > > But then you have to find and update all callsites (or at least the
> > > ones where we care about measurement).
> > In your solution,  if we cache all items that need to be measured, we
> > would have to add a new API (example: QemuFwCfgGetDataFromCache ())  to
> get the data from cache.
> 
> No, we only need to update QemuFwCfgSelectItem + QemuFwCfgReadBytes to
> support reading from the cache.
Do you mean the existing API (QemuFwCfgSelectItem + QemuFwCfgReadBytes) need to be changed to support reading from the cache?

If that is the case,  there are some concerns as below:
1:  One or more new parameters (of QemuFwCfgReadBytes())  need to be added to search 
the item in cache, which is equivalent to adding a new API.

2:  The current QemuFwCfgReadBytes can read in chunks from qemu (example etc/e820) 
If we call QemuFwCfgReadBytes to get data from cache , we also need to support reading in chunks.
Then we have to add more parameters ( to set offset). This will make the API more and more complicated.

> QemuFwCfgGetDataFromCache() can be added as additional API, and callsites have the option to either switch over, or continue to use the existing API.
Base above concerns, we think QemuFwCfgGetDataFromCache is must.

Thanks
Ceping




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117105): https://edk2.groups.io/g/devel/message/117105
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Tue, Mar 26, 2024 at 09:08:59AM +0000, Sun, CepingX wrote:
> On Friday, March 22, 2024 5:06 PM Gerd Hoffmann wrote:
> > 
> > No, we only need to update QemuFwCfgSelectItem + QemuFwCfgReadBytes to
> > support reading from the cache.
> Do you mean the existing API (QemuFwCfgSelectItem + QemuFwCfgReadBytes) need to be changed to support reading from the cache?
> 
> If that is the case,  there are some concerns as below:
> 1:  One or more new parameters (of QemuFwCfgReadBytes())  need to be added to search 
> the item in cache, which is equivalent to adding a new API.

No.

Yes, you need to maintain some extra state, so you know which item was
selected most recently and how many bytes have been read already.

It's not needed to task the caller with that though.  Alternatively you
can add fields to EFI_HOB_PLATFORM_INFO, or create a new HOB for that,
and store the state there.  That way QemuFwCfgReadBytes works properly
with the cache without the caller passing in the state.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117129): https://edk2.groups.io/g/devel/message/117129
Mute This Topic: https://groups.io/mt/104880546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-