[PULL 0/5] Firmware 20241114 patches

[PULL 0/5] Firmware 20241114 patches

[Gerd Hoffmann]

The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:

  Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)

are available in the Git repository at:

  https://gitlab.com/kraxel/qemu.git tags/firmware-20241114-pull-request

for you to fetch changes up to 5916a3b20fdbfbfc2f987f1121e945100c8c3cd2:

  x86/loader: add -shim option (2024-11-14 11:55:39 +0100)

----------------------------------------------------------------
loader: fix efi binary loading via -kernel
loader: support secure boot verification with direct kernel boot

----------------------------------------------------------------

Gerd Hoffmann (5):
  vl: fix qemu_validate_options() indention
  x86/loader: only patch linux kernels
  x86/loader: read complete kernel
  x86/loader: expose unpatched kernel
  x86/loader: add -shim option

 include/hw/boards.h  |  1 +
 hw/core/machine.c    | 20 ++++++++++++++++++++
 hw/i386/x86-common.c | 32 ++++++++++++++++++++++++++------
 system/vl.c          | 25 +++++++++++++++++--------
 qemu-options.hx      |  7 +++++++
 5 files changed, 71 insertions(+), 14 deletions(-)

-- 
2.47.0

[PULL 1/5] vl: fix qemu_validate_options() indention

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-ID: <20240905141211.1253307-2-kraxel@redhat.com>
---
 system/vl.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index d217b3d64de7..3bb8f2db9ac4 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2427,15 +2427,15 @@ static void qemu_validate_options(const QDict *machine_opts)
     const char *kernel_cmdline = qdict_get_try_str(machine_opts, "append");
 
     if (kernel_filename == NULL) {
-         if (kernel_cmdline != NULL) {
-              error_report("-append only allowed with -kernel option");
-              exit(1);
-          }
+        if (kernel_cmdline != NULL) {
+            error_report("-append only allowed with -kernel option");
+            exit(1);
+        }
 
-          if (initrd_filename != NULL) {
-              error_report("-initrd only allowed with -kernel option");
-              exit(1);
-          }
+        if (initrd_filename != NULL) {
+            error_report("-initrd only allowed with -kernel option");
+            exit(1);
+        }
     }
 
     if (loadvm && incoming) {
-- 
2.47.0

[PULL 2/5] x86/loader: only patch linux kernels

[Gerd Hoffmann]

If the binary loaded via -kernel is *not* a linux kernel (in which
case protocol == 0), do not patch the linux kernel header fields.

It's (a) pointless and (b) might break binaries by random patching
and (c) changes the binary hash which in turn breaks secure boot
verification.

Background: OVMF happily loads and runs not only linux kernels but
any efi binary via direct kernel boot.

Note: Breaking the secure boot verification is a problem for linux
kernels too, but fixed that is left for another day ...

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-ID: <20240905141211.1253307-3-kraxel@redhat.com>
---
 hw/i386/x86-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index bc360a9ea44b..ee047308331a 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -943,7 +943,7 @@ void x86_load_linux(X86MachineState *x86ms,
      * kernel on the other side of the fw_cfg interface matches the hash of the
      * file the user passed in.
      */
-    if (!sev_enabled()) {
+    if (!sev_enabled() && protocol > 0) {
         memcpy(setup, header, MIN(sizeof(header), setup_size));
     }
 
-- 
2.47.0

[PULL 3/5] x86/loader: read complete kernel

[Gerd Hoffmann]

Load the complete kernel (including setup) into memory.  Excluding the
setup is handled later when adding the FW_CFG_KERNEL_SIZE and
FW_CFG_KERNEL_DATA entries.

This is a preparation for the next patch which adds a new fw_cfg file
containing the complete, unpatched kernel.  No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-ID: <20240905141211.1253307-4-kraxel@redhat.com>
---
 hw/i386/x86-common.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index ee047308331a..d99bef983e37 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -893,7 +893,6 @@ void x86_load_linux(X86MachineState *x86ms,
         fprintf(stderr, "qemu: invalid kernel header\n");
         exit(1);
     }
-    kernel_size -= setup_size;
 
     setup  = g_malloc(setup_size);
     kernel = g_malloc(kernel_size);
@@ -902,6 +901,7 @@ void x86_load_linux(X86MachineState *x86ms,
         fprintf(stderr, "fread() failed\n");
         exit(1);
     }
+    fseek(f, 0, SEEK_SET);
     if (fread(kernel, 1, kernel_size, f) != kernel_size) {
         fprintf(stderr, "fread() failed\n");
         exit(1);
@@ -948,10 +948,11 @@ void x86_load_linux(X86MachineState *x86ms,
     }
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
-    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
-    sev_load_ctx.kernel_data = (char *)kernel;
-    sev_load_ctx.kernel_size = kernel_size;
+    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size - setup_size);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA,
+                     kernel + setup_size, kernel_size - setup_size);
+    sev_load_ctx.kernel_data = (char *)kernel + setup_size;
+    sev_load_ctx.kernel_size = kernel_size - setup_size;
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
-- 
2.47.0

[PULL 4/5] x86/loader: expose unpatched kernel

[Gerd Hoffmann]

Add a new "etc/boot/kernel" fw_cfg file, containing the kernel without
the setup header patches.  Intended use is booting in UEFI with secure
boot enabled, where the setup header patching breaks secure boot
verification.

Needs OVMF changes too to be actually useful.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-ID: <20240905141211.1253307-5-kraxel@redhat.com>
---
 hw/i386/x86-common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index d99bef983e37..ac91a3464603 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -960,6 +960,9 @@ void x86_load_linux(X86MachineState *x86ms,
     sev_load_ctx.setup_data = (char *)setup;
     sev_load_ctx.setup_size = setup_size;
 
+    /* kernel without setup header patches */
+    fw_cfg_add_file(fw_cfg, "etc/boot/kernel", kernel, kernel_size);
+
     if (sev_enabled()) {
         sev_add_kernel_loader_hashes(&sev_load_ctx, &error_fatal);
     }
-- 
2.47.0

[PULL 5/5] x86/loader: add -shim option

[Gerd Hoffmann]

Add new -shim command line option, wire up for the x86 loader.
When specified load shim into the new "etc/boot/shim" fw_cfg file.

Needs OVMF changes too to be actually useful.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-ID: <20240905141211.1253307-6-kraxel@redhat.com>
---
 include/hw/boards.h  |  1 +
 hw/core/machine.c    | 20 ++++++++++++++++++++
 hw/i386/x86-common.c | 16 ++++++++++++++++
 system/vl.c          |  9 +++++++++
 qemu-options.hx      |  7 +++++++
 5 files changed, 53 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 36fbb9b59df8..a013e769b7bb 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -431,6 +431,7 @@ struct MachineState {
     BootConfiguration boot_config;
     char *kernel_filename;
     char *kernel_cmdline;
+    char *shim_filename;
     char *initrd_filename;
     const char *cpu_type;
     AccelState *accelerator;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index a35c4a8faecb..0d837e4e6924 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -302,6 +302,21 @@ static void machine_set_kernel(Object *obj, const char *value, Error **errp)
     ms->kernel_filename = g_strdup(value);
 }
 
+static char *machine_get_shim(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return g_strdup(ms->shim_filename);
+}
+
+static void machine_set_shim(Object *obj, const char *value, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    g_free(ms->shim_filename);
+    ms->shim_filename = g_strdup(value);
+}
+
 static char *machine_get_initrd(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -1071,6 +1086,11 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "kernel",
         "Linux kernel image file");
 
+    object_class_property_add_str(oc, "shim",
+        machine_get_shim, machine_set_shim);
+    object_class_property_set_description(oc, "shim",
+        "shim.efi file");
+
     object_class_property_add_str(oc, "initrd",
         machine_get_initrd, machine_set_initrd);
     object_class_property_set_description(oc, "initrd",
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index ac91a3464603..a1a90f5f6e8e 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -963,6 +963,22 @@ void x86_load_linux(X86MachineState *x86ms,
     /* kernel without setup header patches */
     fw_cfg_add_file(fw_cfg, "etc/boot/kernel", kernel, kernel_size);
 
+    if (machine->shim_filename) {
+        GMappedFile *mapped_file;
+        GError *gerr = NULL;
+
+        mapped_file = g_mapped_file_new(machine->shim_filename, false, &gerr);
+        if (!mapped_file) {
+            fprintf(stderr, "qemu: error reading shim %s: %s\n",
+                    machine->shim_filename, gerr->message);
+            exit(1);
+        }
+
+        fw_cfg_add_file(fw_cfg, "etc/boot/shim",
+                        g_mapped_file_get_contents(mapped_file),
+                        g_mapped_file_get_length(mapped_file));
+    }
+
     if (sev_enabled()) {
         sev_add_kernel_loader_hashes(&sev_load_ctx, &error_fatal);
     }
diff --git a/system/vl.c b/system/vl.c
index 3bb8f2db9ac4..91926e09c735 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2423,6 +2423,7 @@ static void configure_accelerators(const char *progname)
 static void qemu_validate_options(const QDict *machine_opts)
 {
     const char *kernel_filename = qdict_get_try_str(machine_opts, "kernel");
+    const char *shim_filename = qdict_get_try_str(machine_opts, "shim");
     const char *initrd_filename = qdict_get_try_str(machine_opts, "initrd");
     const char *kernel_cmdline = qdict_get_try_str(machine_opts, "append");
 
@@ -2432,6 +2433,11 @@ static void qemu_validate_options(const QDict *machine_opts)
             exit(1);
         }
 
+        if (shim_filename != NULL) {
+            error_report("-shim only allowed with -kernel option");
+            exit(1);
+        }
+
         if (initrd_filename != NULL) {
             error_report("-initrd only allowed with -kernel option");
             exit(1);
@@ -2914,6 +2920,9 @@ void qemu_init(int argc, char **argv)
             case QEMU_OPTION_kernel:
                 qdict_put_str(machine_opts_dict, "kernel", optarg);
                 break;
+            case QEMU_OPTION_shim:
+                qdict_put_str(machine_opts_dict, "shim", optarg);
+                break;
             case QEMU_OPTION_initrd:
                 qdict_put_str(machine_opts_dict, "initrd", optarg);
                 break;
diff --git a/qemu-options.hx b/qemu-options.hx
index dacc9790a4b8..cc694d3b890c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4145,6 +4145,13 @@ SRST
     or in multiboot format.
 ERST
 
+DEF("shim", HAS_ARG, QEMU_OPTION_shim, \
+    "-shim shim.efi use 'shim.efi' to boot the kernel\n", QEMU_ARCH_ALL)
+SRST
+``-shim shim.efi``
+    Use 'shim.efi' to boot the kernel
+ERST
+
 DEF("append", HAS_ARG, QEMU_OPTION_append, \
     "-append cmdline use 'cmdline' as kernel command line\n", QEMU_ARCH_ALL)
 SRST
-- 
2.47.0

Re: [PULL 0/5] Firmware 20241114 patches

[Daniel P. Berrangé]

On Thu, Nov 14, 2024 at 12:00:56PM +0100, Gerd Hoffmann wrote:
> The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
> 
>   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
> 
> are available in the Git repository at:
> 
>   https://gitlab.com/kraxel/qemu.git tags/firmware-20241114-pull-request
> 
> for you to fetch changes up to 5916a3b20fdbfbfc2f987f1121e945100c8c3cd2:
> 
>   x86/loader: add -shim option (2024-11-14 11:55:39 +0100)
> 
> ----------------------------------------------------------------
> loader: fix efi binary loading via -kernel
> loader: support secure boot verification with direct kernel boot

Hard feature freeze was two days ago, so I would have thought
the new secure boot feature should wait until 10.0 cycle ?

Their commits say they depend on new OVMF features and we've
not updated the OVMF binaries in this cycle, so do we even
have the OVMF feature needed for this to work[1] ?

With regards,
Daniel

[1] admittedly not an issue for distros packaging ovmf separately
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 0/5] Firmware 20241114 patches

[Gerd Hoffmann]

On Thu, Nov 14, 2024 at 11:10:11AM +0000, Daniel P. Berrangé wrote:
> On Thu, Nov 14, 2024 at 12:00:56PM +0100, Gerd Hoffmann wrote:
> > The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
> > 
> >   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
> > 
> > are available in the Git repository at:
> > 
> >   https://gitlab.com/kraxel/qemu.git tags/firmware-20241114-pull-request
> > 
> > for you to fetch changes up to 5916a3b20fdbfbfc2f987f1121e945100c8c3cd2:
> > 
> >   x86/loader: add -shim option (2024-11-14 11:55:39 +0100)
> > 
> > ----------------------------------------------------------------
> > loader: fix efi binary loading via -kernel
> > loader: support secure boot verification with direct kernel boot
> 
> Hard feature freeze was two days ago, so I would have thought
> the new secure boot feature should wait until 10.0 cycle ?

Patches have been posted back in September.  This PR is a bit late
because I was offline in October, and also because I didn't realize we
are in freeze already due to being active mostly in edk2 these days.

So, if an exception is out if question I'll have to wait until 10.0
opens I guess ...

> Their commits say they depend on new OVMF features and we've
> not updated the OVMF binaries in this cycle, so do we even
> have the OVMF feature needed for this to work[1] ?

Nope.  I have a branch ready.  The plan is to submit that once the qemu
changes are accepted.  edk2 is in freeze too, so this will not make the
edk2 2024-11 stable tag.  If all goes well it'll land in 2025-02, which
we should be able to put into qemu 10.0

take care,
  Gerd

Re: [PULL 0/5] Firmware 20241114 patches

[Peter Maydell]

On Thu, 14 Nov 2024 at 11:33, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Nov 14, 2024 at 11:10:11AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Nov 14, 2024 at 12:00:56PM +0100, Gerd Hoffmann wrote:
> > > The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
> > >
> > >   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > >   https://gitlab.com/kraxel/qemu.git tags/firmware-20241114-pull-request
> > >
> > > for you to fetch changes up to 5916a3b20fdbfbfc2f987f1121e945100c8c3cd2:
> > >
> > >   x86/loader: add -shim option (2024-11-14 11:55:39 +0100)
> > >
> > > ----------------------------------------------------------------
> > > loader: fix efi binary loading via -kernel
> > > loader: support secure boot verification with direct kernel boot
> >
> > Hard feature freeze was two days ago, so I would have thought
> > the new secure boot feature should wait until 10.0 cycle ?
>
> Patches have been posted back in September.  This PR is a bit late
> because I was offline in October, and also because I didn't realize we
> are in freeze already due to being active mostly in edk2 these days.
>
> So, if an exception is out if question I'll have to wait until 10.0
> opens I guess ...
>
> > Their commits say they depend on new OVMF features and we've
> > not updated the OVMF binaries in this cycle, so do we even
> > have the OVMF feature needed for this to work[1] ?
>
> Nope.  I have a branch ready.  The plan is to submit that once the qemu
> changes are accepted.  edk2 is in freeze too, so this will not make the
> edk2 2024-11 stable tag.  If all goes well it'll land in 2025-02, which
> we should be able to put into qemu 10.0

If we aren't landing the firmware side until QEMU 10.0 either
then I think I agree with Daniel that the QEMU-side new
feature work also should wait until 10.0.

(I plan to try to be quite conservative with the release schedule
this cycle, because we don't have a lot of time between when
it's supposed to complete and the Christmas holidays, so we
can't afford to overrun the nominal release date by more than
a week.)

thanks
-- PMM