Re: [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base
From: Guoheyi
Date: Mon Dec 16 2019 - 08:50:30 EST
å 2019/12/16 19:14, Marc Zyngier åé:
Hi Heyi,
On 2019-12-16 06:27, Heyi Guo wrote:
If we use ACPI MADT GICC structure to pass single redistributor base,
and mark some GICC as disabled, we'll get below call trace during
boot:
[ÂÂÂ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ÂÂÂ 0.000000] GICv3: 256 SPIs implemented
[ÂÂÂ 0.000000] GICv3: 0 Extended SPIs implemented
[ÂÂÂ 0.000000] GICv3: Distributor has no Range Selector support
[ÂÂÂ 0.000000] Unable to handle kernel paging request at virtual
address 000000000000ffe8
[ÂÂÂ 0.000000] Mem abort info:
[ÂÂÂ 0.000000]ÂÂ ESR = 0x96000004
[ÂÂÂ 0.000000]ÂÂ EC = 0x25: DABT (current EL), IL = 32 bits
[ÂÂÂ 0.000000]ÂÂ SET = 0, FnV = 0
[ÂÂÂ 0.000000]ÂÂ EA = 0, S1PTW = 0
[ÂÂÂ 0.000000] Data abort info:
[ÂÂÂ 0.000000]ÂÂ ISV = 0, ISS = 0x00000004
[ÂÂÂ 0.000000]ÂÂ CM = 0, WnR = 0
[ÂÂÂ 0.000000] [000000000000ffe8] user address but active_mm is swapper
[ÂÂÂ 0.000000] Internal error: Oops: 96000004 [#1] SMP
[ÂÂÂ 0.000000] Modules linked in:
[ÂÂÂ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.5.0-rc1 #5
[ÂÂÂ 0.000000] pstate: 20000085 (nzCv daIf -PAN -UAO)
[ÂÂÂ 0.000000] pc : gic_iterate_rdists+0x58/0x130
[ÂÂÂ 0.000000] lr : gic_iterate_rdists+0x80/0x130
[ÂÂÂ 0.000000] sp : ffff8000113d3cb0
[ÂÂÂ 0.000000] x29: ffff8000113d3cb0 x28: 0000000000000000
[ÂÂÂ 0.000000] x27: 0000000000000000 x26: 0000000000000018
[ÂÂÂ 0.000000] x25: 000000000000ffe8 x24: 000000000000003f
[ÂÂÂ 0.000000] x23: ffff800010588040 x22: 00000000000005e8
[ÂÂÂ 0.000000] x21: ffff8000113df7d0 x20: 0000030f00003f11
[ÂÂÂ 0.000000] x19: 0000000000000000 x18: ffffffffffffffff
[ÂÂÂ 0.000000] x17: 0000000014aeb8dc x16: 00000000c3ba0ccf
[ÂÂÂ 0.000000] x15: ffff8000113d9908 x14: ffff8000913d3a37
[ÂÂÂ 0.000000] x13: ffff8000113d3a45 x12: ffff800011402000
[ÂÂÂ 0.000000] x11: ffff8000113d39d0 x10: ffff8000113db980
[ÂÂÂ 0.000000] x9 : 00000000ffffffd0 x8 : ffff8000106dca98
[ÂÂÂ 0.000000] x7 : 000000000000005b x6 : 0000000000000000
[ÂÂÂ 0.000000] x5 : 0000000000000000 x4 : ffff8000128c0000
[ÂÂÂ 0.000000] x3 : ffff8000128a0000 x2 : ffff0003fc3c7000
[ÂÂÂ 0.000000] x1 : 0000000000000001 x0 : 000000000000ffe8
[ÂÂÂ 0.000000] Call trace:
[ÂÂÂ 0.000000]Â gic_iterate_rdists+0x58/0x130
[ÂÂÂ 0.000000]Â gic_init_bases+0x200/0x4b4
[ÂÂÂ 0.000000]Â gic_acpi_init+0x148/0x284
[ÂÂÂ 0.000000]Â acpi_match_madt+0x4c/0x84
[ÂÂÂ 0.000000]Â acpi_table_parse_entries_array+0x188/0x278
[ÂÂÂ 0.000000]Â acpi_table_parse_entries+0x70/0x98
[ÂÂÂ 0.000000]Â acpi_table_parse_madt+0x40/0x50
[ÂÂÂ 0.000000]Â __acpi_probe_device_table+0x88/0xe4
[ÂÂÂ 0.000000]Â irqchip_init+0x38/0x40
[ÂÂÂ 0.000000]Â init_IRQ+0x168/0x19c
[ÂÂÂ 0.000000]Â start_kernel+0x328/0x508
[ÂÂÂ 0.000000] Code: f90017b6 9b3a7f16 f8766853 8b190260 (b9400000)
[ÂÂÂ 0.000000] ---[ end trace ae5cf232d924bfc1 ]---
[ÂÂÂ 0.000000] Kernel panic - not syncing: Fatal exception
[ÂÂÂ 0.000000] Rebooting in 3 seconds..
In this case, nr_redist_regions counts all GICC structures but only
enabled ones have redistributor mapped. So add check to avoid NULL
deference of redist_base.
Signed-off-by: Heyi Guo <guoheyi@xxxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Jason Cooper <jason@xxxxxxxxxxxxxx>
Cc: Marc Zyngier <maz@xxxxxxxxxx>
---
Âdrivers/irqchip/irq-gic-v3.c | 7 +++++++
Â1 file changed, 7 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d6218012097b..bd9d55cadef9 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -781,6 +781,13 @@ static int gic_iterate_rdists(int (*fn)(struct
redist_region *, void __iomem *))
ÂÂÂÂÂÂÂÂ u64 typer;
ÂÂÂÂÂÂÂÂ u32 reg;
+ÂÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂÂ * redist_base may be NULL if we use single_redist and some
GICC
+ÂÂÂÂÂÂÂÂ * structure is disabled.
+ÂÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ if (!ptr)
+ÂÂÂÂÂÂÂÂÂÂÂ continue;
+
ÂÂÂÂÂÂÂÂ reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
ÂÂÂÂÂÂÂÂ if (reg != GIC_PIDR2_ARCH_GICv3 &&
ÂÂÂÂÂÂÂÂÂÂÂÂ reg != GIC_PIDR2_ARCH_GICv4) { /* We're in trouble... */
This feels like the wrong fix. The redistributor region array should
be completely populated, and there is an assumption all over this driver
that there is no junk in these structures.
Oh, I thought the place holder for disabled GICR in nr_redist_regions
were for some special reason, like CPU hotplug. Now I know I was wrong :)
You're seeing this because we don't track the number of *enabled* rdists,
and allocate the number of regions based on the number of overall GICC
entries instead of the number of enabled redistributors.
How about this instead?
It looks good to me, and works fine in my case.
Thanks,
Heyi
ÂÂÂÂÂÂÂ M.
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 3a1866682dd0..9b26a860340b 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1861,6 +1861,7 @@ static struct
ÂÂÂÂ struct redist_region *redist_regs;
ÂÂÂÂ u32 nr_redist_regions;
ÂÂÂÂ bool single_redist;
+ÂÂÂ int enabled_rdists;
ÂÂÂÂ u32 maint_irq;
ÂÂÂÂ int maint_irq_mode;
ÂÂÂÂ phys_addr_t vcpu_base;
@@ -1955,8 +1956,10 @@ static int __init gic_acpi_match_gicc(union
acpi_subtable_headers *header,
ÂÂÂÂÂ * If GICC is enabled and has valid gicr base address, then it means
ÂÂÂÂÂ * GICR base is presented via GICC
ÂÂÂÂÂ */
-ÂÂÂ if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address)
+ÂÂÂ if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address) {
+ÂÂÂÂÂÂÂ acpi_data.enabled_rdists++;
ÂÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
ÂÂÂÂ /*
ÂÂÂÂÂ * It's perfectly valid firmware can pass disabled GICC entry,
driver
@@ -1986,8 +1989,10 @@ static int __init
gic_acpi_count_gicr_regions(void)
ÂÂÂÂ count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ gic_acpi_match_gicc, 0);
-ÂÂÂ if (count > 0)
+ÂÂÂ if (count > 0) {
ÂÂÂÂÂÂÂÂ acpi_data.single_redist = true;
+ÂÂÂÂÂÂÂ count = acpi_data.enabled_rdists;
+ÂÂÂ }
ÂÂÂÂ return count;
Â}