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

[Stefano Garzarella <sgarzare@redhat.com>]

On Mon, Sep 07, 2026 at 04:58:55PM +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_get_param_data(), which looks up the parameter area,
>checks byte_offset against its size, and returns the offset-adjusted
>data pointer directly (NULL on failure), with the remaining space
>returned via an output parameter, instead of the raw
>QIgvmParameterData entry. This keeps the byte_offset arithmetic in
>one place instead of repeating param_entry->data + byte_offset and
>param_entry->size - byte_offset at every call site.
>
>Use it in the vp-count and environment-info handlers, adding an
>explicit check that the fixed-size write fits in the remaining space.
>

Should we add a Fixes tag?

>Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
>---
> backends/igvm.c                | 70 +++++++++++++++++++++++++++++++++++-------
> include/system/igvm-internal.h |  6 ++++
> 2 files changed, 65 insertions(+), 11 deletions(-)
>
>diff --git a/backends/igvm.c b/backends/igvm.c
>index 7b7bdc72b7..a8340d7d55 100644
>--- a/backends/igvm.c
>+++ b/backends/igvm.c
>@@ -101,6 +101,39 @@ qigvm_find_param_entry(QIgvm *igvm, uint32_t parameter_area_index,
>     return NULL;
> }
>
>+/*
>+ * Look up a parameter area and check that param->byte_offset falls within
>+ * it, in one step. On success, returns the offset-adjusted write location
>+ * within the parameter area and sets *param_size to the remaining space
>+ * there, so callers never need to touch param->byte_offset themselves.
>+ * Returns NULL on failure.
>+ */

How about something a little more concise and straight to the point?

/*
  * Get parameter area data at byte_offset with bounds validation.
  * On success, returns offset-adjusted data pointer and sets param_size
  * to remaining space.
  * Returns NULL on failure.
  */

>+uint8_t *
>+qigvm_get_param_data(QIgvm *igvm, uint32_t parameter_area_index,

IIUC all callers are just passing param->parameter_area_index, since we 
have `param`, can we avoid passsing `parameter_area_index` as parameter?

>+                     const IGVM_VHS_PARAMETER *param,
>+                     uint32_t *param_size,
>+                     Error **errp)
>+{
>+    QIgvmParameterData *param_entry;
>+
>+    assert(param_size);
>+
>+    param_entry = qigvm_find_param_entry(igvm, parameter_area_index, errp);
>+    if (!param_entry) {
>+        return NULL;
>+    }
>+
>+    if (param->byte_offset > param_entry->size) {
>+        error_setg(errp,
>+                   "IGVM: byte_offset 0x%x exceeds parameter area size 0x%x",
>+                   param->byte_offset, param_entry->size);
>+        return NULL;
>+    }
>+
>+    *param_size = param_entry->size - param->byte_offset;
>+    return param_entry->data + param->byte_offset;
>+}
>+
> 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,
>@@ -682,17 +715,25 @@ static int qigvm_directive_vp_count(QIgvm *ctx, const uint8_t *header_data,
>                                     Error **errp)
> {
>     const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
>-    QIgvmParameterData *param_entry;
>+    uint8_t *param_data;
>+    uint32_t param_size;
>     uint32_t *vp_count;
>     CPUState *cpu;
>
>-    param_entry = qigvm_find_param_entry(ctx,
>-                                         param->parameter_area_index, errp);
>-    if (param_entry == NULL) {
>+    param_data = qigvm_get_param_data(ctx, param->parameter_area_index,

Can we avoid `param_data` at all and assing it directly to `vp_count` ?

>+                                       param, &param_size, errp);
>+    if (!param_data) {
>         return -1;
>     }
>
>-    vp_count = (uint32_t *)(param_entry->data + param->byte_offset);
>+    if (sizeof(*vp_count) > param_size) {
>+        error_setg(errp,
>+                   "IGVM: vp-count parameter exceeds parameter area "
>+                   "defined in IGVM file");
>+        return -1;
>+    }
>+
>+    vp_count = (uint32_t *)param_data;
>     *vp_count = 0;
>     CPU_FOREACH(cpu)
>     {
>@@ -707,17 +748,24 @@ static int qigvm_directive_environment_info(QIgvm *ctx,
>                                             Error **errp)
> {
>     const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
>-    QIgvmParameterData *param_entry;
>+    uint8_t *param_data;
>+    uint32_t param_size;
>     IgvmEnvironmentInfo *environmental_state;
>
>-    param_entry = qigvm_find_param_entry(ctx,
>-                                         param->parameter_area_index, errp);
>-    if (param_entry == NULL) {
>+    param_data = qigvm_get_param_data(ctx, param->parameter_area_index,
>+                                       param, &param_size, errp);

Ditto.

>+    if (!param_data) {
>+        return -1;
>+    }
>+
>+    if (sizeof(*environmental_state) > param_size) {
>+        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 = (IgvmEnvironmentInfo *)param_data;
>     environmental_state->memory_is_shared = 1;
>
>     return 0;
>diff --git a/include/system/igvm-internal.h b/include/system/igvm-internal.h
>index 9e9fa1d9af..f5ee2d5b1b 100644
>--- a/include/system/igvm-internal.h
>+++ b/include/system/igvm-internal.h
>@@ -81,4 +81,10 @@ QIgvmParameterData*
> qigvm_find_param_entry(QIgvm *igvm, uint32_t parameter_area_index,
>                        Error **errp);
>
>+uint8_t *
>+qigvm_get_param_data(QIgvm *igvm, uint32_t parameter_area_index,
>+                     const IGVM_VHS_PARAMETER *param,
>+                     uint32_t *param_size,
>+                     Error **errp);
>+
> #endif
>
>-- 
>2.55.0
>