From: David Woodhouse <dwmw@amazon.co.uk>

In e820_add_entry() the e820_table is reallocated with g_renew() to make
space for a new entry. However, fw_cfg_arch_create() just uses the existing
e820_table pointer.

This leads to a use-after-free if anything adds a new entry after fw_cfg
is set up. Shift the addition of the etc/e820 file to the machine done
notifier, and add a sanity check to ensure that e820_table isn't
modified after the pointer gets stashed.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/e820_memory_layout.c | 8 ++++++++
 hw/i386/fw_cfg.c             | 7 ++++---
 hw/i386/microvm.c            | 5 +++--
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
index 06970ac44a..c96515909e 100644
--- a/hw/i386/e820_memory_layout.c
+++ b/hw/i386/e820_memory_layout.c
@@ -8,13 +8,20 @@
 
 #include "qemu/osdep.h"
 #include "qemu/bswap.h"
+#include "qemu/error-report.h"
 #include "e820_memory_layout.h"
 
 static size_t e820_entries;
 struct e820_entry *e820_table;
+static gboolean e820_done;
 
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
+    if (e820_done) {
+        warn_report("warning: E820 modified after being consumed");
+        return -1;
+    }
+
     /* new "etc/e820" file -- include ram and reserved entries */
     e820_table = g_renew(struct e820_entry, e820_table, e820_entries + 1);
     e820_table[e820_entries].address = cpu_to_le64(address);
@@ -27,6 +34,7 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 
 int e820_get_num_entries(void)
 {
+    e820_done = true;
     return e820_entries;
 }
 
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 6e0d9945d0..e046ad1a54 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -102,6 +102,10 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg,
                         smbios_anchor, smbios_anchor_len);
     }
 #endif
+
+    /* Add etc/e820 late, once all regions should be present */
+    fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
+                    sizeof(struct e820_entry) * e820_get_num_entries());
 }
 
 FWCfgState *fw_cfg_arch_create(MachineState *ms,
@@ -139,9 +143,6 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
 #endif
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
 
-    fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
-                    sizeof(struct e820_entry) * e820_get_num_entries());
-
     fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index fec63cacfa..89b2abcebf 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -324,8 +324,6 @@ static void microvm_memory_init(MicrovmMachineState *mms)
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, machine->smp.max_cpus);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
-    fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
-                    sizeof(struct e820_entry) * e820_get_num_entries());
 
     rom_set_fw(fw_cfg);
 
@@ -586,9 +584,12 @@ static void microvm_machine_done(Notifier *notifier, void *data)
 {
     MicrovmMachineState *mms = container_of(notifier, MicrovmMachineState,
                                             machine_done);
+    X86MachineState *x86ms = X86_MACHINE(mms);
 
     acpi_setup_microvm(mms);
     dt_setup_microvm(mms);
+    fw_cfg_add_file(x86ms->fw_cfg, "etc/e820", e820_table,
+                    sizeof(struct e820_entry) * e820_get_num_entries());
 }
 
 static void microvm_powerdown_req(Notifier *notifier, void *data)
-- 
2.44.0

On Mon, 17 Jun 2024 at 14:46, David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> In e820_add_entry() the e820_table is reallocated with g_renew() to make
> space for a new entry. However, fw_cfg_arch_create() just uses the existing
> e820_table pointer.
>
> This leads to a use-after-free if anything adds a new entry after fw_cfg
> is set up. Shift the addition of the etc/e820 file to the machine done
> notifier, and add a sanity check to ensure that e820_table isn't
> modified after the pointer gets stashed.

Given that e820_add_entry() will happily g_renew() the memory,
it seems a bit bug-prone to have e820_table be a global variable.
Maybe we should have an e820_add_fw_cfg_file() which does the

    fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
                    sizeof(struct e820_entry) * e820_get_num_entries());

-- that would then let us make e820_table be file-local, and so
it's then easy to audit that all the functions that look at
e820_table check that the table has been finalized first (because
they're all in this one file).

> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  hw/i386/e820_memory_layout.c | 8 ++++++++
>  hw/i386/fw_cfg.c             | 7 ++++---
>  hw/i386/microvm.c            | 5 +++--
>  3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
> index 06970ac44a..c96515909e 100644
> --- a/hw/i386/e820_memory_layout.c
> +++ b/hw/i386/e820_memory_layout.c
> @@ -8,13 +8,20 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/bswap.h"
> +#include "qemu/error-report.h"
>  #include "e820_memory_layout.h"
>
>  static size_t e820_entries;
>  struct e820_entry *e820_table;
> +static gboolean e820_done;
>
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>  {
> +    if (e820_done) {
> +        warn_report("warning: E820 modified after being consumed");
> +        return -1;
> +    }

I think this should be a fatal error (i.e. assert) -- it should
never happen, and always would be a bug in QEMU somewhere.

Currently e820_add_entry() returns the number of entries
currently present. Of the various callsites, almost all ignore
the return value. Two treat it as a "negative means error"
situation (with an error handling path that's currently dead code):
target/i386/kvm/kvm.c and target/i386/kvm/xen-emu.c.

My suggestion is that we make e820_add_entry() return void,
and remove that dead error handling path.

thanks
-- PMM

On Mon, 2024-06-17 at 15:15 +0100, Peter Maydell wrote:
> On Mon, 17 Jun 2024 at 14:46, David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > In e820_add_entry() the e820_table is reallocated with g_renew() to make
> > space for a new entry. However, fw_cfg_arch_create() just uses the existing
> > e820_table pointer.
> > 
> > This leads to a use-after-free if anything adds a new entry after fw_cfg
> > is set up. Shift the addition of the etc/e820 file to the machine done
> > notifier, and add a sanity check to ensure that e820_table isn't
> > modified after the pointer gets stashed.
> 
> Given that e820_add_entry() will happily g_renew() the memory,
> it seems a bit bug-prone to have e820_table be a global variable.
> Maybe we should have an e820_add_fw_cfg_file() which does the
> 
>     fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
>                     sizeof(struct e820_entry) * e820_get_num_entries());
> 
> -- that would then let us make e820_table be file-local, and so
> it's then easy to audit that all the functions that look at
> e820_table check that the table has been finalized first (because
> they're all in this one file).

Yeah, I pondered that, but wasn't sure I wanted to add a dependency on
fw_cfg directly in the e820 code. So I pondered making e820_table
static and using an accessor function... but then figured that since
there's *already* an accessor for the table size, I could just use
that.

I suppose we could have a single function which returns both the table
pointer *and* its size. It's a slight cleanup, but seemed like more
churn that it was worth, and being C obviously it can't literally
*return* both, so it just gets slightly ugly. Happy to do it if you
feel strongly.

> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >  hw/i386/e820_memory_layout.c | 8 ++++++++
> >  hw/i386/fw_cfg.c             | 7 ++++---
> >  hw/i386/microvm.c            | 5 +++--
> >  3 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
> > index 06970ac44a..c96515909e 100644
> > --- a/hw/i386/e820_memory_layout.c
> > +++ b/hw/i386/e820_memory_layout.c
> > @@ -8,13 +8,20 @@
> > 
> >  #include "qemu/osdep.h"
> >  #include "qemu/bswap.h"
> > +#include "qemu/error-report.h"
> >  #include "e820_memory_layout.h"
> > 
> >  static size_t e820_entries;
> >  struct e820_entry *e820_table;
> > +static gboolean e820_done;
> > 
> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> >  {
> > +    if (e820_done) {
> > +        warn_report("warning: E820 modified after being consumed");
> > +        return -1;
> > +    }
> 
> I think this should be a fatal error (i.e. assert) -- it should
> never happen, and always would be a bug in QEMU somewhere.

OK.

> Currently e820_add_entry() returns the number of entries
> currently present. Of the various callsites, almost all ignore
> the return value. Two treat it as a "negative means error"
> situation (with an error handling path that's currently dead code):
> target/i386/kvm/kvm.c and target/i386/kvm/xen-emu.c.
> 
> My suggestion is that we make e820_add_entry() return void,
> and remove that dead error handling path.

Ack.