Re: [PATCH] hw/i386/fw_cfg: Add etc/e820 to fw_cfg late

[Peter Maydell <peter.maydell@linaro.org>]

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