| Message ID | 3ce6d142356cb061b64d71a4e39525d9d7c52b12.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386/fw_cfg: Add etc/e820 to fw_cfg late | expand |
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.
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)