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 <[email protected]>
---
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, ¶m_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, ¶m_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
