Re: [PATCH] hw/i386/elfboot: allocate "header" in heap

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Nov 09, 2024 at 12:03:14AM +0100, slp@redhat.com wrote:
> From: Sergio Lopez <slp@redhat.com>
> 
> In x86_load_linux(), we were using a stack-allocated array as data for
> fw_cfg_add_bytes(). Since the latter just takes a reference to the
> pointer instead of copying the data, it can happen that the contents
> have been overridden by the time the guest attempts to access them.
> 
> Instead of using the stack-allocated array, allocate some memory from
> the heap, copy the contents of the array, and use it for fw_cfg.
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>

Wow. How did this ever work?


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Paolo, you queueing this?

> ---
>  hw/i386/x86-common.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> index bc360a9ea4..d51ebad4d6 100644
> --- a/hw/i386/x86-common.c
> +++ b/hw/i386/x86-common.c
> @@ -697,9 +697,12 @@ void x86_load_linux(X86MachineState *x86ms,
>                  strlen(kernel_cmdline) + 1);
>              fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
>  
> +            setup = g_malloc(sizeof(header));
> +            memcpy(setup, header, sizeof(header));
> +
>              fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, sizeof(header));
>              fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA,
> -                             header, sizeof(header));
> +                             setup, sizeof(header));
>  
>              /* load initrd */
>              if (initrd_filename) {
> -- 
> 2.45.2