[v5,1/4] x86: return modified setup_data only if read as memory, not as file

Commit Message

Jason A. Donenfeld Sept. 21, 2022, 9:31 a.m. UTC

If setup_data is being read into a specific memory location, then
generally the setup_data address parameter is read first, so that the
caller knows where to read it into. In that case, we should return
setup_data containing the absolute addresses that are hard coded and
determined a priori. This is the case when kernels are loaded by BIOS,
for example. In contrast, when setup_data is read as a file, then we
shouldn't modify setup_data, since the absolute address will be wrong by
definition. This is the case when OVMF loads the image.

This allows setup_data to be used like normal, without crashing when EFI
tries to use it.

(As a small development note, strangely, fw_cfg_add_file_callback() was
exported but fw_cfg_add_bytes_callback() wasn't, so this makes that
consistent.)

Cc: Gerd Hoffmann <[e-mail]>
Cc: Laurent Vivier <[e-mail]>
Cc: Michael S. Tsirkin <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Peter Maydell <[e-mail]>
Cc: Philippe Mathieu-Daudé <[e-mail]>
Cc: Richard Henderson <[e-mail]>
Suggested-by: Ard Biesheuvel <[e-mail]>
Reviewed-by: Ard Biesheuvel <[e-mail]>
Signed-off-by: Jason A. Donenfeld <[e-mail]>
---
 hw/i386/x86.c             | 46 ++++++++++++++++++++++++++++++---------
 hw/nvram/fw_cfg.c         | 12 +++++-----
 include/hw/nvram/fw_cfg.h | 22 +++++++++++++++++++
 3 files changed, 64 insertions(+), 16 deletions(-)

Comments

Paolo Bonzini Sept. 22, 2022, 7:26 a.m. UTC | #1

On Wed, 21 Sep 2022 11:31:31 +0200, Jason A. Donenfeld wrote:
> If setup_data is being read into a specific memory location, then
> generally the setup_data address parameter is read first, so that the
> caller knows where to read it into. In that case, we should return
> setup_data containing the absolute addresses that are hard coded and
> determined a priori. This is the case when kernels are loaded by BIOS,
> for example. In contrast, when setup_data is read as a file, then we
> shouldn't modify setup_data, since the absolute address will be wrong by
> definition. This is the case when OVMF loads the image.
> 
> [...]

Applied, thanks!

[1/4] x86: return modified setup_data only if read as memory, not as file
      commit: 7e660efb4d2af17582588bcdc4af7d28040feda6
[2/4] x86: use typedef for SetupData struct
      commit: 1127a29a33f975943d89fc36ab934ad37810e4b7
[3/4] x86: reinitialize RNG seed on system reboot
      commit: e997fc36def502dc3e9da915816083f086dff8f2
[4/4] x86: re-enable rng seeding via SetupData
      commit: a731bae726f2b3168dea9d5137cb7e47444448eb

Best regards,

Patch

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 050eedc0c8..96d205927e 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -37,6 +37,7 @@ 
 #include "sysemu/whpx.h"
 #include "sysemu/numa.h"
 #include "sysemu/replay.h"
+#include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpu-timers.h"
 #include "sysemu/xen.h"
@@ -764,6 +765,24 @@  static bool load_elfboot(const char *kernel_filename,
     return true;
 }
 
+typedef struct SetupDataFixup {
+    void *pos;
+    hwaddr orig_val, new_val;
+    uint32_t addr;
+} SetupDataFixup;
+
+static void fixup_setup_data(void *opaque)
+{
+    SetupDataFixup *fixup = opaque;
+    stq_p(fixup->pos, fixup->new_val);
+}
+
+static void reset_setup_data(void *opaque)
+{
+    SetupDataFixup *fixup = opaque;
+    stq_p(fixup->pos, fixup->orig_val);
+}
+
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
@@ -1088,8 +1107,11 @@  void x86_load_linux(X86MachineState *x86ms,
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
     }
 
-    /* Offset 0x250 is a pointer to the first setup_data link. */
-    stq_p(header + 0x250, first_setup_data);
+    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;
 
     /*
      * If we're starting an encrypted VM, it will be OVMF based, which uses the
@@ -1099,16 +1121,20 @@  void x86_load_linux(X86MachineState *x86ms,
      * file the user passed in.
      */
     if (!sev_enabled()) {
+        SetupDataFixup *fixup = g_malloc(sizeof(*fixup));
+
         memcpy(setup, header, MIN(sizeof(header), setup_size));
+        /* Offset 0x250 is a pointer to the first setup_data link. */
+        fixup->pos = setup + 0x250;
+        fixup->orig_val = ldq_p(fixup->pos);
+        fixup->new_val = first_setup_data;
+        fixup->addr = cpu_to_le32(real_addr);
+        fw_cfg_add_bytes_callback(fw_cfg, FW_CFG_SETUP_ADDR, fixup_setup_data, NULL,
+                                  fixup, &fixup->addr, sizeof(fixup->addr), true);
+        qemu_register_reset(reset_setup_data, fixup);
+    } else {
+        fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
     }
-
-    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_SETUP_ADDR, real_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
     sev_load_ctx.setup_data = (char *)setup;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d605f3f45a..564bda3395 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -692,12 +692,12 @@  static const VMStateDescription vmstate_fw_cfg = {
     }
 };
 
-static void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
-                                      FWCfgCallback select_cb,
-                                      FWCfgWriteCallback write_cb,
-                                      void *callback_opaque,
-                                      void *data, size_t len,
-                                      bool read_only)
+void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
+                               FWCfgCallback select_cb,
+                               FWCfgWriteCallback write_cb,
+                               void *callback_opaque,
+                               void *data, size_t len,
+                               bool read_only)
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 0e7a8bc7af..e4fef393be 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -117,6 +117,28 @@  struct FWCfgMemState {
  */
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
 
+/**
+ * fw_cfg_add_bytes_callback:
+ * @s: fw_cfg device being modified
+ * @key: selector key value for new fw_cfg item
+ * @select_cb: callback function when selecting
+ * @write_cb: callback function after a write
+ * @callback_opaque: argument to be passed into callback function
+ * @data: pointer to start of item data
+ * @len: size of item data
+ * @read_only: is file read only
+ *
+ * Add a new fw_cfg item, available by selecting the given key, as a raw
+ * "blob" of the given size. The data referenced by the starting pointer
+ * is only linked, NOT copied, into the data structure of the fw_cfg device.
+ */
+void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
+                               FWCfgCallback select_cb,
+                               FWCfgWriteCallback write_cb,
+                               void *callback_opaque,
+                               void *data, size_t len,
+                               bool read_only);
+
 /**
  * fw_cfg_add_string:
  * @s: fw_cfg device being modified