From 0a8bf6cee48864204dcd10f5244bddd95fa6805a Mon Sep 17 00:00:00 2001 From: Sainath Grandhi Date: Tue, 18 Jun 2019 13:35:05 -0700 Subject: [PATCH] hv: Avoid run-time buffer overflows with IOAPIC data structures Remove couple of run-time ASSERTs in ioapic module by checking for the number of interrupt pins per IO-APICs against the configured MAX_IOAPIC_LINES in the initialization flow. Also remove the need for two MACROs specifying the max. number of interrupt lines per IO-APIC and add a config item MAX_IOAPIC_LINES for the same. Tracked-On: #3299 Signed-off-by: Sainath Grandhi Acked-by: Eddie Dong --- hypervisor/arch/x86/Kconfig | 5 ++ hypervisor/arch/x86/ioapic.c | 77 +++++++++++++++++++--------- hypervisor/include/arch/x86/ioapic.h | 7 +-- 3 files changed, 61 insertions(+), 28 deletions(-) diff --git a/hypervisor/arch/x86/Kconfig b/hypervisor/arch/x86/Kconfig index d897526d8..7acd1db80 100644 --- a/hypervisor/arch/x86/Kconfig +++ b/hypervisor/arch/x86/Kconfig @@ -264,6 +264,11 @@ config MAX_IOAPIC_NUM range 1 8 default 1 +config MAX_IOAPIC_LINES + int "Maximum number of interrupt lines per IOAPIC" + range 1 120 + default 120 + config MAX_IR_ENTRIES int "Maximum number of Interrupt Remapping Entries" default 256 diff --git a/hypervisor/arch/x86/ioapic.c b/hypervisor/arch/x86/ioapic.c index d4e9384d0..28d1f1117 100644 --- a/hypervisor/arch/x86/ioapic.c +++ b/hypervisor/arch/x86/ioapic.c @@ -15,21 +15,15 @@ #include #include -#define IOAPIC_MAX_PIN 240U #define IOAPIC_INVALID_ID 0xFFU -/* - * IOAPIC_MAX_LINES is architecturally defined. - * The usable RTEs may be a subset of the total on a per IO APIC basis. - */ -#define IOAPIC_MAX_LINES 120U -#define NR_MAX_GSI (CONFIG_MAX_IOAPIC_NUM * IOAPIC_MAX_LINES) +#define NR_MAX_GSI (CONFIG_MAX_IOAPIC_NUM * CONFIG_MAX_IOAPIC_LINES) static struct gsi_table gsi_table_data[NR_MAX_GSI]; static uint32_t ioapic_nr_gsi; static spinlock_t ioapic_lock; -static union ioapic_rte saved_rte[CONFIG_MAX_IOAPIC_NUM][IOAPIC_MAX_PIN]; +static union ioapic_rte saved_rte[CONFIG_MAX_IOAPIC_NUM][CONFIG_MAX_IOAPIC_LINES]; /* * the irq to ioapic pin mapping should extract from ACPI MADT table @@ -104,13 +98,13 @@ uint32_t get_pic_pin_from_ioapic_pin(uint32_t pin_index) return pin_id; } +/* + * @pre irq_num < NR_MAX_GSI + */ void *ioapic_get_gsi_irq_addr(uint32_t irq_num) { - void *addr = NULL; - if (irq_num < NR_MAX_GSI) { - addr = gsi_table_data[irq_num].addr; - } - return addr; + + return gsi_table_data[irq_num].addr; } uint32_t ioapic_get_nr_gsi(void) @@ -372,8 +366,6 @@ ioapic_nr_pins(void *ioapic_base) * interrupt input pins. */ nr_pins = (((version & IOAPIC_MAX_RTE_MASK) >> MAX_RTE_SHIFT) + 1U); - ASSERT(nr_pins > NR_LEGACY_IRQ, "Legacy IRQ num > total GSI"); - ASSERT(nr_pins <= IOAPIC_MAX_PIN, "IOAPIC pins exceeding 240"); return nr_pins; } @@ -394,12 +386,53 @@ uint8_t ioapic_irq_to_ioapic_id(uint32_t irq) int32_t init_ioapic_id_info(void) { int32_t ret = 0; + uint8_t ioapic_id; + void *addr; + uint32_t nr_pins, gsi; ioapic_num = parse_madt_ioapic(&ioapic_array[0]); - if (ioapic_num > (uint16_t)CONFIG_MAX_IOAPIC_NUM) { + if (ioapic_num <= (uint16_t)CONFIG_MAX_IOAPIC_NUM) { + /* + * Iterate thru all the IO-APICs on the platform + * Check the number of pins available on each IOAPIC is less + * than the CONFIG_MAX_IOAPIC_LINES + */ + + gsi = 0U; + for (ioapic_id = 0U; ioapic_id < ioapic_num; ioapic_id++) { + addr = map_ioapic(ioapic_array[ioapic_id].addr); + hv_access_memory_region_update((uint64_t)addr, PAGE_SIZE); + + nr_pins = ioapic_nr_pins(addr); + if (nr_pins <= (uint32_t) CONFIG_MAX_IOAPIC_LINES) { + gsi += nr_pins; + ioapic_array[ioapic_id].nr_pins = nr_pins; + } else { + pr_err ("Pin count %x of IOAPIC with %x > CONFIG_MAX_IOAPIC_LINES, bump up CONFIG_MAX_IOAPIC_LINES!", + nr_pins, ioapic_array[ioapic_id].id); + ret = -EINVAL; + break; + } + } + + /* + * Check if total pin count, can be inferred by GSI, is + * atleast same as the number of Legacy IRQs, NR_LEGACY_IRQ + */ + + if (ret == 0) { + if (gsi < (uint32_t) NR_LEGACY_IRQ) { + pr_err ("Total pin count (%x) is less than NR_LEGACY_IRQ!", gsi); + ret = -EINVAL; + } + } + } else { + pr_err ("Number of IOAPIC on platform %x > CONFIG_MAX_IOAPIC_NUM, try bumping up CONFIG_MAX_IOAPIC_NUM!", + ioapic_num); ret = -EINVAL; } + return ret; } @@ -417,9 +450,8 @@ void ioapic_setup_irqs(void) uint32_t pin, nr_pins; addr = map_ioapic(ioapic_array[ioapic_id].addr); - hv_access_memory_region_update((uint64_t)addr, PAGE_SIZE); - nr_pins = ioapic_nr_pins(addr); + nr_pins = ioapic_array[ioapic_id].nr_pins; for (pin = 0U; pin < nr_pins; pin++) { gsi_table_data[gsi].ioapic_id = ioapic_array[ioapic_id].id; gsi_table_data[gsi].addr = addr; @@ -459,7 +491,6 @@ void ioapic_setup_irqs(void) /* system max gsi numbers */ ioapic_nr_gsi = gsi; - ASSERT(ioapic_nr_gsi <= NR_MAX_GSI, "GSI table overflow"); } void suspend_ioapic(void) @@ -469,11 +500,9 @@ void suspend_ioapic(void) for (ioapic_id = 0U; ioapic_id < ioapic_num; ioapic_id++) { void *addr; - uint32_t nr_pins; addr = map_ioapic(get_ioapic_base(ioapic_id)); - nr_pins = ioapic_nr_pins(addr); - for (ioapic_pin = 0U; ioapic_pin < nr_pins; ioapic_pin++) { + for (ioapic_pin = 0U; ioapic_pin < ioapic_array[ioapic_id].nr_pins; ioapic_pin++) { ioapic_get_rte_entry(addr, ioapic_pin, &saved_rte[ioapic_id][ioapic_pin]); } @@ -487,11 +516,9 @@ void resume_ioapic(void) for (ioapic_id = 0U; ioapic_id < ioapic_num; ioapic_id++) { void *addr; - uint32_t nr_pins; addr = map_ioapic(get_ioapic_base(ioapic_id)); - nr_pins = ioapic_nr_pins(addr); - for (ioapic_pin = 0U; ioapic_pin < nr_pins; ioapic_pin++) { + for (ioapic_pin = 0U; ioapic_pin < ioapic_array[ioapic_id].nr_pins; ioapic_pin++) { ioapic_set_rte_entry(addr, ioapic_pin, saved_rte[ioapic_id][ioapic_pin]); } diff --git a/hypervisor/include/arch/x86/ioapic.h b/hypervisor/include/arch/x86/ioapic.h index 249a890bd..09dc95840 100644 --- a/hypervisor/include/arch/x86/ioapic.h +++ b/hypervisor/include/arch/x86/ioapic.h @@ -13,9 +13,10 @@ #define NR_LEGACY_PIN NR_LEGACY_IRQ struct ioapic_info { - uint8_t id; - uint32_t addr; - uint32_t gsi_base; + uint8_t id; /* IOAPIC ID as indicated in ACPI MADT */ + uint32_t addr; /* IOAPIC Register address */ + uint32_t gsi_base; /* Global System Interrupt where this IO-APIC's interrupt input start */ + uint32_t nr_pins; /* Number of Interrupt inputs as determined by Max. Redir Entry Register */ }; void ioapic_setup_irqs(void);