[PATCH 0/2] libxl/ACPI: address observations from XSA-464

[PATCH 0/2] libxl/ACPI: address observations from XSA-464

[Jan Beulich]

1: don't hard-code guest page size
2: bound RSDP allocation

Jan

[PATCH 1/2] libxl/ACPI: don't hard-code guest page size

[Jan Beulich]

We have libxl_ctxt.page_size for this purpose; use it to eliminate a
latent buffer overrun.

Fixes: 14c0d328da2b ("libxl/acpi: Build ACPI tables for HVMlite guests")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Yet better might be to limit the size to what's actually used (libacpi's
struct acpi_info). That would then also have avoided the respective part
of XSA-???.

--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -218,7 +218,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
         dom->acpi_modules[0].guest_addr_out = 0x100000 - 64;
 
     dom->acpi_modules[1].data = (void *)config.infop;
-    dom->acpi_modules[1].length = 4096;
+    dom->acpi_modules[1].length = libxl_ctxt.page_size;
     dom->acpi_modules[1].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS;
 
     dom->acpi_modules[2].data = libxl_ctxt.buf;

Re: [PATCH 1/2] libxl/ACPI: don't hard-code guest page size

[Anthony PERARD]

On Mon, Nov 25, 2024 at 04:15:28PM +0100, Jan Beulich wrote:
> We have libxl_ctxt.page_size for this purpose; use it to eliminate a
> latent buffer overrun.

The 4096 here might actually refer to the size used to allocate
`config.infop`, which is `libxl_ctxt.page_size`. So I don't if the
explanation is correct, but at least now the same value is used for both
zmalloc() and .lenght.

> Fixes: 14c0d328da2b ("libxl/acpi: Build ACPI tables for HVMlite guests")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

> ---
> Yet better might be to limit the size to what's actually used (libacpi's
> struct acpi_info). That would then also have avoided the respective part
> of XSA-???.

It's kind of hard to tell here how `infop` is going to be used from this
function, so changing the lenght just here might not do the right thing.

> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -218,7 +218,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>          dom->acpi_modules[0].guest_addr_out = 0x100000 - 64;
>  
>      dom->acpi_modules[1].data = (void *)config.infop;
> -    dom->acpi_modules[1].length = 4096;
> +    dom->acpi_modules[1].length = libxl_ctxt.page_size;
>      dom->acpi_modules[1].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS;
>  
>      dom->acpi_modules[2].data = libxl_ctxt.buf;

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH 2/2] libxl/ACPI: bound RSDP allocation

[Jan Beulich]

First instroduce a manifest constant, to avoid open-coding 64 in several
places. Then use this constant to bound the allocation.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Similarly bounding the info "page" allocation would be nice, but would
require knowing libacpi's struct acpi_info size here.

--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -20,6 +20,8 @@
 
  /* Number of pages holding ACPI tables */
 #define NUM_ACPI_PAGES 16
+/* Hard-coded size of RSDP */
+#define RSDP_LEN 64
 #define ALIGN(p, a) (((p) + ((a) - 1)) & ~((a) - 1))
 
 struct libxl_acpi_ctxt {
@@ -177,7 +179,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     }
 
     /* These are all copied into guest memory, so use zero-ed memory. */
-    config.rsdp = (unsigned long)libxl__zalloc(gc, libxl_ctxt.page_size);
+    config.rsdp = (unsigned long)libxl__zalloc(gc, RSDP_LEN);
     config.infop = (unsigned long)libxl__zalloc(gc, libxl_ctxt.page_size);
     /* Pages to hold ACPI tables */
     libxl_ctxt.buf = libxl__zalloc(gc, NUM_ACPI_PAGES *
@@ -204,18 +206,18 @@ int libxl__dom_load_acpi(libxl__gc *gc,
                       libxl_ctxt.guest_start) >> libxl_ctxt.page_shift;
 
     dom->acpi_modules[0].data = (void *)config.rsdp;
-    dom->acpi_modules[0].length = 64;
+    dom->acpi_modules[0].length = RSDP_LEN;
     /*
      * Some Linux versions cannot properly process hvm_start_info.rsdp_paddr
      * and so we need to put RSDP in location that can be discovered by ACPI's
-     * standard search method, in R-O BIOS memory (we chose last 64 bytes)
+     * standard search method, in R-O BIOS memory (we chose last RSDP_LEN bytes)
      */
     if (strcmp(xc_dom_guest_os(dom), "linux") ||
         xc_dom_feature_get(dom, XENFEAT_linux_rsdp_unrestricted))
         dom->acpi_modules[0].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS +
             (1 + acpi_pages_num) * libxl_ctxt.page_size;
     else
-        dom->acpi_modules[0].guest_addr_out = 0x100000 - 64;
+        dom->acpi_modules[0].guest_addr_out = 0x100000 - RSDP_LEN;
 
     dom->acpi_modules[1].data = (void *)config.infop;
     dom->acpi_modules[1].length = libxl_ctxt.page_size;

Re: [PATCH 2/2] libxl/ACPI: bound RSDP allocation

[Anthony PERARD]

On Mon, Nov 25, 2024 at 04:15:49PM +0100, Jan Beulich wrote:
> First instroduce a manifest constant, to avoid open-coding 64 in several
> places. Then use this constant to bound the allocation.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Hopefully, `struct acpi_20_rsdp` isn't going to be bigger that 64, but
it would probably not work well anyway seen how `config.rsdp` is used
here.

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

> ---
> Similarly bounding the info "page" allocation would be nice, but would
> require knowing libacpi's struct acpi_info size here.

Or register the allocation size in `config`, so acpi_build_tables() can
check if there's enough space. Something like `config.info_size`.

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH 2/2] libxl/ACPI: bound RSDP allocation

[Jan Beulich]

On 25.11.2024 18:04, Anthony PERARD wrote:
> On Mon, Nov 25, 2024 at 04:15:49PM +0100, Jan Beulich wrote:
>> First instroduce a manifest constant, to avoid open-coding 64 in several
>> places. Then use this constant to bound the allocation.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Hopefully, `struct acpi_20_rsdp` isn't going to be bigger that 64, but
> it would probably not work well anyway seen how `config.rsdp` is used
> here.
> 
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks.

>> ---
>> Similarly bounding the info "page" allocation would be nice, but would
>> require knowing libacpi's struct acpi_info size here.
> 
> Or register the allocation size in `config`, so acpi_build_tables() can
> check if there's enough space. Something like `config.info_size`.

That would feel kind of backwards. It should be libacpi to specify the
size it needs, yet that won't work as libacpi is the consumer of the
config struct. We could of course add acpi_get_info_size() to libacpi,
for libxl to use. 

Jan