From: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
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, via
a new fw_cfg_add_e820() function.
Also make e820_table private and use an e820_get_table() accessor function
for it, which sets a flag that will trigger an assert() for any *later*
attempts to add to the table.
Make e820_add_entry() return void, as most callers don't check for error
anyway.
Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
Message-Id: <a2708734f004b224f33d3b4824e9a5a262431568.camel@xxxxxxxxxxxxx>
Reviewed-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
---
hw/i386/e820_memory_layout.h | 8 ++------
hw/i386/fw_cfg.h | 1 +
hw/i386/e820_memory_layout.c | 17 ++++++++++++-----
hw/i386/fw_cfg.c | 18 +++++++++++++-----
hw/i386/microvm.c | 4 ++--
hw/i386/pc.c | 1 +
target/i386/kvm/kvm.c | 6 +-----
target/i386/kvm/xen-emu.c | 7 +------
8 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/hw/i386/e820_memory_layout.h b/hw/i386/e820_memory_layout.h
index 7c239aa033..b50acfa201 100644
--- a/hw/i386/e820_memory_layout.h
+++ b/hw/i386/e820_memory_layout.h
@@ -22,13 +22,9 @@ struct e820_entry {
uint32_t type;
} QEMU_PACKED __attribute((__aligned__(4)));
-extern struct e820_entry *e820_table;
-
-int e820_add_entry(uint64_t address, uint64_t length, uint32_t type);
-int e820_get_num_entries(void);
+void e820_add_entry(uint64_t address, uint64_t length, uint32_t type);
bool e820_get_entry(int index, uint32_t type,
uint64_t *address, uint64_t *length);
-
-
+int e820_get_table(struct e820_entry **table);
#endif
diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
index 92e310f5fd..e560fd7be8 100644
--- a/hw/i386/fw_cfg.h
+++ b/hw/i386/fw_cfg.h
@@ -27,5 +27,6 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg,
SmbiosEntryPointType ep_type);
void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg);
void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg);
+void fw_cfg_add_e820(FWCfgState *fw_cfg);
#endif
diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
index 06970ac44a..3e848fb69c 100644
--- a/hw/i386/e820_memory_layout.c
+++ b/hw/i386/e820_memory_layout.c
@@ -11,22 +11,29 @@
#include "e820_memory_layout.h"
static size_t e820_entries;
-struct e820_entry *e820_table;
+static struct e820_entry *e820_table;
+static gboolean e820_done;
-int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
+void e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
{
+ assert(!e820_done);
+
/* 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);
e820_table[e820_entries].length = cpu_to_le64(length);
e820_table[e820_entries].type = cpu_to_le32(type);
e820_entries++;
-
- return e820_entries;
}
-int e820_get_num_entries(void)
+int e820_get_table(struct e820_entry **table)
{
+ e820_done = true;
+
+ if (table) {
+ *table = e820_table;
+ }
+
return e820_entries;
}
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 7c43c325ef..0e4494627c 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -48,6 +48,15 @@ const char *fw_cfg_arch_key_name(uint16_t key)
return NULL;
}
+/* Add etc/e820 late, once all regions should be present */
+void fw_cfg_add_e820(FWCfgState *fw_cfg)
+{
+ struct e820_entry *table;
+ int nr_e820 = e820_get_table(&table);
+
+ fw_cfg_add_file(fw_cfg, "etc/e820", table, nr_e820 * sizeof(*table));
+}
+
void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg,
SmbiosEntryPointType ep_type)
{
@@ -60,6 +69,7 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg,
PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
MachineClass *mc = MACHINE_GET_CLASS(pcms);
X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
+ int nr_e820;
if (pcmc->smbios_defaults) {
/* These values are guest ABI, do not change */
@@ -78,8 +88,9 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg,
}
/* build the array of physical mem area from e820 table */
- mem_array = g_malloc0(sizeof(*mem_array) * e820_get_num_entries());
- for (i = 0, array_count = 0; i < e820_get_num_entries(); i++) {
+ nr_e820 = e820_get_table(NULL);
+ mem_array = g_malloc0(sizeof(*mem_array) * nr_e820);
+ for (i = 0, array_count = 0; i < nr_e820; i++) {
uint64_t addr, len;
if (e820_get_entry(i, E820_RAM, &addr, &len)) {
@@ -138,9 +149,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..40edcee7af 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,11 @@ 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_e820(x86ms->fw_cfg);
}
static void microvm_powerdown_req(Notifier *notifier, void *data)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 77415064c6..d2c29fbfcb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -625,6 +625,7 @@ void pc_machine_done(Notifier *notifier, void *data)
acpi_setup();
if (x86ms->fw_cfg) {
fw_cfg_build_smbios(pcms, x86ms->fw_cfg, pcms->smbios_entry_point_type);
+ fw_cfg_add_e820(x86ms->fw_cfg);
fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg);
/* update FW_CFG_NB_CPUS to account for -device added CPUs */
fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index dd8b0f3313..bf182570fe 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2706,11 +2706,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
}
/* Tell fw_cfg to notify the BIOS to reserve the range. */
- ret = e820_add_entry(identity_base, 0x4000, E820_RESERVED);
- if (ret < 0) {
- fprintf(stderr, "e820_add_entry() table is full\n");
- return ret;
- }
+ e820_add_entry(identity_base, 0x4000, E820_RESERVED);
shadow_mem = object_property_get_int(OBJECT(s), "kvm-shadow-mem", &error_abort);
if (shadow_mem != -1) {
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index fc2c2321ac..2f89dc628e 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -176,12 +176,7 @@ int kvm_xen_init(KVMState *s, uint32_t hypercall_msr)
s->xen_caps = xen_caps;
/* Tell fw_cfg to notify the BIOS to reserve the range. */
- ret = e820_add_entry(XEN_SPECIAL_AREA_ADDR, XEN_SPECIAL_AREA_SIZE,
- E820_RESERVED);
- if (ret < 0) {
- fprintf(stderr, "e820_add_entry() table is full\n");
- return ret;
- }
+ e820_add_entry(XEN_SPECIAL_AREA_ADDR, XEN_SPECIAL_AREA_SIZE, E820_RESERVED);
/* The pages couldn't be overlaid until KVM was initialized */
xen_primary_console_reset();
--
MST
