Re: [PATCH 2/2] igvm: validate byte_offset before using it in parameter directives

[Stefano Garzarella <sgarzare@redhat.com>]

On Wed, Sep 02, 2026 at 04:21:59PM +0200, Luigi Leonardi wrote:
>None of the directive handlers that place data at a byte_offset within
>a parameter area validated that byte_offset actually falls within the
>parameter area's size. A malformed IGVM file with byte_offset > size would
>underflow the "size - byte_offset" computation used to determine
>remaining space, wrapping to a huge value and defeating the size
>check, then write out of bounds through param_entry->data +
>byte_offset.
>
>Add qigvm_param_offset_valid() to check byte_offset against the
>parameter area size, and call it from each handler before doing any
>arithmetic with byte_offset.
>
>When possible, add an explicit check that fixed-size write fits
>in the remaining space.
>
>Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
>---
> backends/igvm.c                | 46 ++++++++++++++++++++++++++++++++++++++++++
> include/system/igvm-internal.h |  4 ++++
> target/i386/igvm.c             |  4 ++++
> 3 files changed, 54 insertions(+)
>
>diff --git a/backends/igvm.c b/backends/igvm.c
>index a6e3a58022..5d2364bf70 100644
>--- a/backends/igvm.c
>+++ b/backends/igvm.c
>@@ -101,6 +101,22 @@ qigvm_find_param_entry(QIgvm *igvm, uint32_t parameter_area_index,
>     return NULL;
> }
>
>+/*
>+ * Check that byte_offset falls within the parameter area
>+ */
>+bool qigvm_param_offset_valid(const QIgvmParameterData *param_entry,
>+                              const IGVM_VHS_PARAMETER *param,
>+                              Error **errp)
>+{
>+    if (param->byte_offset > param_entry->size) {

Good, this is exaclt what I was suggesting in patch 1, but why 
introducing this check in a later commit? This will break bisectability 
IMO.

That said, I think we should move these checks in the helpers I 
suggested in patch 1, or maybe we should change qigvm_find_param_entry() 
or add wrapper around it to do everything (find, validate, return 
pointer/size).

Stefano

>+        error_setg(errp,
>+                   "IGVM: byte_offset 0x%x exceeds parameter area size 
>0x%x",
>+                   param->byte_offset, param_entry->size);
>+        return false;
>+    }
>+    return true;
>+}
>+
> static int qigvm_directive_page_data(QIgvm *ctx, const uint8_t *header_data,
>                                      Error **errp);
> static int qigvm_directive_vp_context(QIgvm *ctx, const uint8_t *header_data,
>@@ -632,6 +648,10 @@ static int qigvm_directive_memory_map(QIgvm *ctx, const uint8_t *header_data,
>         return -1;
>     }
>
>+    if (!qigvm_param_offset_valid(param_entry, param, errp)) {
>+        return -1;
>+    }
>+
>     max_entry_count = (param_entry->size - param->byte_offset) /
>                       sizeof(IGVM_VHS_MEMORY_MAP_ENTRY);
>     mm_entry = (IGVM_VHS_MEMORY_MAP_ENTRY *)(param_entry->data +
>@@ -694,6 +714,17 @@ static int qigvm_directive_vp_count(QIgvm *ctx, const uint8_t *header_data,
>         return -1;
>     }
>
>+    if (!qigvm_param_offset_valid(param_entry, param, errp)) {
>+        return -1;
>+    }
>+
>+    if (sizeof(*vp_count) > param_entry->size - param->byte_offset) {
>+        error_setg(errp,
>+                   "IGVM: vp-count parameter exceeds parameter area "
>+                   "defined in IGVM file");
>+        return -1;
>+    }
>+
>     vp_count = (uint32_t *)(param_entry->data + param->byte_offset);
>     *vp_count = 0;
>     CPU_FOREACH(cpu)
>@@ -718,6 +749,17 @@ static int qigvm_directive_environment_info(QIgvm *ctx,
>         return -1;
>     }
>
>+    if (!qigvm_param_offset_valid(param_entry, param, errp)) {
>+        return -1;
>+    }
>+
>+    if (sizeof(*environmental_state) > param_entry->size - param->byte_offset) {
>+        error_setg(errp,
>+                   "IGVM: environment-info parameter exceeds parameter area "
>+                   "defined in IGVM file");
>+        return -1;
>+    }
>+
>     environmental_state =
>         (IgvmEnvironmentInfo *)(param_entry->data + param->byte_offset);
>     environmental_state->memory_is_shared = 1;
>@@ -825,6 +867,10 @@ static int qigvm_directive_device_tree(QIgvm *ctx, const uint8_t *header_data,
>         return -1;
>     }
>
>+    if (!qigvm_param_offset_valid(param_entry, param, errp)) {
>+        return -1;
>+    }
>+
>     if (ctx->machine_state->fdt == NULL) {
>         error_setg(errp, "IGVM: device tree not available");
>         return -1;
>diff --git a/include/system/igvm-internal.h b/include/system/igvm-internal.h
>index 9e9fa1d9af..7e0d1512ae 100644
>--- a/include/system/igvm-internal.h
>+++ b/include/system/igvm-internal.h
>@@ -81,4 +81,8 @@ QIgvmParameterData*
> qigvm_find_param_entry(QIgvm *igvm, uint32_t parameter_area_index,
>                        Error **errp);
>
>+bool qigvm_param_offset_valid(const QIgvmParameterData *param_entry,
>+                              const IGVM_VHS_PARAMETER *param,
>+                              Error **errp);
>+
> #endif
>diff --git a/target/i386/igvm.c b/target/i386/igvm.c
>index 4d9d97385a..e758989d0d 100644
>--- a/target/i386/igvm.c
>+++ b/target/i386/igvm.c
>@@ -197,6 +197,10 @@ int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data, Error **errp)
>         return -1;
>     }
>
>+    if (!qigvm_param_offset_valid(param_entry, param, errp)) {
>+        return -1;
>+    }
>+
>     GArray *madt = acpi_build_madt_standalone(ctx->machine_state);
>
>     if (madt->len <= param_entry->size - param->byte_offset) {
>
>-- 
>2.55.0
>