On Mon, 2024-06-17 at 15:15 +0100, Peter Maydell wrote: > On Mon, 17 Jun 2024 at 14:46, David Woodhouse <dw...@infradead.org> wrote: > > > > From: David Woodhouse <d...@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 <d...@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.
smime.p7s
Description: S/MIME cryptographic signature
