[PATCH 0/5] x86/loader: secure boot support for direct kernel load

[PATCH 0/5] x86/loader: secure boot support for direct kernel load

[Gerd Hoffmann]

This series allows to boot linux kernels and other efi binaries via
direct kernel load with secure boot enabled.

The series adds two new fw_cfg files: 'etc/boot/kernel' contains the
kernel without modifications (no setup header patching), and
'etc/boot/shim' contains shim.

The path to the shim binary can be passed to qemu using the new '-shim'
command line switch.

This needs a companion patch series for tianocore which will put the new
fw_cfg files into use, a draft of that series can be found here:

https://github.com/kraxel/edk2/commits/devel/direct-secure-boot/

With everything in place it is possible to use direct kernel load with
secure boot enabled.

take care,
  Gerd

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.c       | 32 ++++++++++++++++++++++++++------
 system/vl.c         | 25 +++++++++++++++++--------
 qemu-options.hx     |  7 +++++++
 5 files changed, 71 insertions(+), 14 deletions(-)

-- 
2.44.0

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

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <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 c64422298245..0c6201c5bdc5 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2411,15 +2411,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.44.0

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

[Philippe Mathieu-Daudé]

On 11/4/24 10:48, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   system/vl.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 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>
---
 hw/i386/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index ffbda48917fd..765899eebe43 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1108,7 +1108,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.44.0

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

[Michael Tokarev]

11.04.2024 12:48, Gerd Hoffmann wrote:
> 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 ...

Shouldn't this one be picked up for -stable?

Thanks,

/mjt

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/i386/x86.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index ffbda48917fd..765899eebe43 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1108,7 +1108,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));
>       }
>   


-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

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

[Gerd Hoffmann]

On Tue, Dec 17, 2024 at 02:09:30PM +0300, Michael Tokarev wrote:
> 11.04.2024 12:48, Gerd Hoffmann wrote:
> > 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 ...
> 
> Shouldn't this one be picked up for -stable?

yes, please.

thanks,
  Gerd

[PATCH 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>
---
 hw/i386/x86.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 765899eebe43..6f75948b3021 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1058,7 +1058,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);
@@ -1067,6 +1066,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);
@@ -1113,10 +1113,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.44.0

[PATCH 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>
---
 hw/i386/x86.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 6f75948b3021..6724e408e576 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1125,6 +1125,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.44.0

Re: [PATCH 4/5] x86/loader: expose unpatched kernel

[Michael S. Tsirkin]

On Thu, Apr 11, 2024 at 11:48:28AM +0200, Gerd Hoffmann wrote:
> 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>


So given we have this, do we still need patch 2?


> ---
>  hw/i386/x86.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 6f75948b3021..6724e408e576 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1125,6 +1125,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.44.0

Re: [PATCH 4/5] x86/loader: expose unpatched kernel

[Gerd Hoffmann]

On Sun, Jun 02, 2024 at 09:26:09AM GMT, Michael S. Tsirkin wrote:
> On Thu, Apr 11, 2024 at 11:48:28AM +0200, Gerd Hoffmann wrote:
> > 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>
> 
> So given we have this, do we still need patch 2?

With this merged to qemu plus related edk2 patches merged too OVMF will
stop using the patched linux kernel setup header fw_cfg file.  So, patch
#2 will not be essential for direct kernel boot to work correctly with
UEFI.

Nevertheless I'd consider patch #2 a clear bugfix.  Trying to patch
linux kernel setup header fields in binaries which are /not/ a linux
kernel doesn't make any sense.

take care,
  Gerd

[PATCH 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>
---
 include/hw/boards.h |  1 +
 hw/core/machine.c   | 20 ++++++++++++++++++++
 hw/i386/x86.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 8b8f6d5c00d3..37da417cb029 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -399,6 +399,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 37ede0e7d4fd..f27f6ae8e199 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -313,6 +313,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);
@@ -988,6 +1003,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.c b/hw/i386/x86.c
index 6724e408e576..3e95f196fb40 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1128,6 +1128,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 0c6201c5bdc5..4df42ba8c7a6 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2407,6 +2407,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");
 
@@ -2416,6 +2417,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);
@@ -2908,6 +2914,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 8ce85d45598d..b5151857afe5 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4002,6 +4002,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.44.0