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

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

[slp]

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>
---
 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

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

[Michael S. Tsirkin]

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

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

[Sergio Lopez Pascual]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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?
>

I guess, for quite a while, we were just lucky that memory region wasn't
touched by the time the FW in the guest uses it (possibly helped by the
fact this happens very early in the VM lifetime). In recent
versions/builds this is no longer the case.

Sergio.

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

[Philippe Mathieu-Daudé]

On 8/11/24 23:03, 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>
> ---
>   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));

Preferably using g_memdup2(header, sizeof(header)),

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   
>               /* load initrd */
>               if (initrd_filename) {

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

[Philippe Mathieu-Daudé]

On 8/11/24 23:03, 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>
> ---
>   hw/i386/x86-common.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Patch queued to hw-misc, thanks.