[PATCH v5 01/11] x86/vioapic: Add ioapic_check() to validate IO-APIC state before restore

[Julian Vetter <julian.vetter@xxxxxxxxxx>]

Register a check callback for the IOAPIC HVM save/restore entry,
following the pattern established by vpic_check() for the virtual PIC.
The function first verifies the target domain actually has a virtual
IO-APIC, returning -ENODEV otherwise. It then validates individual
fields of the saved state: base_address must be non-zero, page-aligned,
and leave room for the MMIO window below the domain's physical address
limit. The APIC ID must fit the 4-bit field vioapic_write_indirect()
stores and no redirection table entry may carry a delivery_status bit,
which is read-only and always cleared on a guest write.

Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
---
Changes in v5:
- The check now verifies base_address is page-alignment and
  hap_paddr_bits and not just "!= 0".
- The APIC ID is bounded by the IO_APIC_reg_02 field and not a
  hard-coded 0xf (added comment).
- Replaced the ioregsel check with a loop rejecting any entry that has
  delivery_status set.
- Commit message rewritten.

Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
---
 xen/arch/x86/hvm/vioapic.c | 44 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 222e59e2c2..80dc9148a9 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -323,6 +323,7 @@ static void vioapic_write_indirect(
          * Presumably because we emulate an Intel IOAPIC which only has a
          * 4 bit ID field (compared to 8 for AMD), using union IO_APIC_reg_02
          * for the ID register (union IO_APIC_reg_00's ID field is 8 bits).
+         * ioapic_check() validates the saved id field accordingly.
          */
         vioapic->id = ((union IO_APIC_reg_02){ .raw = val }).bits.arbitration;
         break;
@@ -595,6 +596,47 @@ int vioapic_get_trigger_mode(const struct domain *d, 
unsigned int gsi)
     return vioapic->redirtbl[pin].fields.trig_mode;
 }
 
+static int cf_check ioapic_check(const struct domain *d, hvm_domain_context_t 
*h)
+{
+    const HVM_SAVE_TYPE(IOAPIC) *s;
+
+    if ( !has_vioapic(d) )
+        return -ENODEV;
+
+    s = hvm_get_entry(IOAPIC, h);
+    if ( !s )
+        return -ENODATA;
+
+    /*
+     * base_address must be non-zero, page-aligned (hardware constraint), and
+     * within the guest's physical address space (with room for the full MMIO
+     * window).
+     */
+    if ( !s->base_address ||
+         !IS_ALIGNED(s->base_address, PAGE_SIZE) ||
+         s->base_address > (1ULL << hap_paddr_bits) - VIOAPIC_MEM_LENGTH )
+        return -EINVAL;
+
+    /*
+     * vioapic_write_indirect() stores only the 4-bit arbitration field of
+     * IO_APIC_reg_02 as the APIC ID. See that function's comment for why
+     * IO_APIC_reg_02 is used rather than IO_APIC_reg_00.
+     */
+    if ( s->id > ((union IO_APIC_reg_02){ .raw = ~0U }).bits.arbitration )
+        return -EINVAL;
+
+    /*
+     * Reject redirection table entries carrying bits that
+     * vioapic_write_redirent() would never store: delivery_status is read-only
+     * and always cleared on a guest write.
+     */
+    for ( unsigned int i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )
+        if ( s->redirtbl[i].fields.delivery_status )
+            return -EINVAL;
+
+    return 0;
+}
+
 static int cf_check ioapic_save(struct vcpu *v, hvm_domain_context_t *h)
 {
     const struct domain *d = v->domain;
@@ -631,7 +673,7 @@ static int cf_check ioapic_load(struct domain *d, 
hvm_domain_context_t *h)
     return 0;
 }
 
-HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, NULL, ioapic_load, 1,
+HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_check, ioapic_load, 1,
                           HVMSR_PER_DOM);
 
 void vioapic_reset(struct domain *d)
-- 
2.53.0



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech