* [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base @ 2019-12-16 6:27 Heyi Guo 2019-12-16 11:14 ` Marc Zyngier 2020-02-08 14:58 ` [tip: irq/urgent] irqchip/gic-v3: Only provision redistributors that are enabled in ACPI tip-bot2 for Marc Zyngier 0 siblings, 2 replies; 6+ messages in thread From: Heyi Guo @ 2019-12-16 6:27 UTC (permalink / raw) To: linux-kernel Cc: wanghaibin.wang, Heyi Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier 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@huawei.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Marc Zyngier <maz@kernel.org> --- 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... */ -- 2.19.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base 2019-12-16 6:27 [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base Heyi Guo @ 2019-12-16 11:14 ` Marc Zyngier 2019-12-16 13:50 ` Guoheyi 2020-02-08 14:58 ` [tip: irq/urgent] irqchip/gic-v3: Only provision redistributors that are enabled in ACPI tip-bot2 for Marc Zyngier 1 sibling, 1 reply; 6+ messages in thread From: Marc Zyngier @ 2019-12-16 11:14 UTC (permalink / raw) To: Heyi Guo; +Cc: linux-kernel, wanghaibin.wang, Thomas Gleixner, Jason Cooper 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@huawei.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Marc Zyngier <maz@kernel.org> > --- > 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. 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? 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; } -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base 2019-12-16 11:14 ` Marc Zyngier @ 2019-12-16 13:50 ` Guoheyi 2019-12-16 14:23 ` Marc Zyngier 0 siblings, 1 reply; 6+ messages in thread From: Guoheyi @ 2019-12-16 13:50 UTC (permalink / raw) To: Marc Zyngier; +Cc: linux-kernel, wanghaibin.wang, Thomas Gleixner, Jason Cooper 在 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@huawei.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Jason Cooper <jason@lakedaemon.net> >> Cc: Marc Zyngier <maz@kernel.org> >> --- >> 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; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base 2019-12-16 13:50 ` Guoheyi @ 2019-12-16 14:23 ` Marc Zyngier 2019-12-17 1:20 ` Guoheyi 0 siblings, 1 reply; 6+ messages in thread From: Marc Zyngier @ 2019-12-16 14:23 UTC (permalink / raw) To: Guoheyi; +Cc: linux-kernel, wanghaibin.wang, Thomas Gleixner, Jason Cooper On 2019-12-16 13:50, Guoheyi wrote: > 在 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@huawei.com> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Cc: Jason Cooper <jason@lakedaemon.net> >>> Cc: Marc Zyngier <maz@kernel.org> >>> --- >>> 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 > :) CPU hotplug would imply that the redistributors are available. My interpretation of the ACPI MADT GICC subtable is that the redistributors are simply inaccessible when disabled. Otherwise, it'd be legitimate to just map them and live with redistributors that do not have a corresponding CPU (which we otherwise do). See ebe2f8718007 for details. If we need to support redistributors becoming enabled under our feet, then we'll have to handle this in a different way. We're not there yet. >> 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. Can I take this as a Tested-by: ? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base 2019-12-16 14:23 ` Marc Zyngier @ 2019-12-17 1:20 ` Guoheyi 0 siblings, 0 replies; 6+ messages in thread From: Guoheyi @ 2019-12-17 1:20 UTC (permalink / raw) To: Marc Zyngier; +Cc: linux-kernel, wanghaibin.wang, Thomas Gleixner, Jason Cooper 在 2019/12/16 22:23, Marc Zyngier 写道: > On 2019-12-16 13:50, Guoheyi wrote: >> 在 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@huawei.com> >>>> Cc: Thomas Gleixner <tglx@linutronix.de> >>>> Cc: Jason Cooper <jason@lakedaemon.net> >>>> Cc: Marc Zyngier <maz@kernel.org> >>>> --- >>>> 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 >> :) > > CPU hotplug would imply that the redistributors are available. > My interpretation of the ACPI MADT GICC subtable is that the > redistributors are simply inaccessible when disabled. > > Otherwise, it'd be legitimate to just map them and live with > redistributors that do not have a corresponding CPU (which we > otherwise do). See ebe2f8718007 for details. > > If we need to support redistributors becoming enabled under our > feet, then we'll have to handle this in a different way. We're > not there yet. Got it. > >>> 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. > > Can I take this as a Tested-by: ? Sure. Tested-by: Heyi Guo <guoheyi@huawei.com> Thanks, Heyi > > Thanks, > > M. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: irq/urgent] irqchip/gic-v3: Only provision redistributors that are enabled in ACPI 2019-12-16 6:27 [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base Heyi Guo 2019-12-16 11:14 ` Marc Zyngier @ 2020-02-08 14:58 ` tip-bot2 for Marc Zyngier 1 sibling, 0 replies; 6+ messages in thread From: tip-bot2 for Marc Zyngier @ 2020-02-08 14:58 UTC (permalink / raw) To: linux-tip-commits; +Cc: Marc Zyngier, Heyi Guo, x86, LKML The following commit has been merged into the irq/urgent branch of tip: Commit-ID: 926b5dfa6b8dc666ff398044af6906b156e1d949 Gitweb: https://git.kernel.org/tip/926b5dfa6b8dc666ff398044af6906b156e1d949 Author: Marc Zyngier <maz@kernel.org> AuthorDate: Mon, 16 Dec 2019 11:24:57 Committer: Marc Zyngier <maz@kernel.org> CommitterDate: Tue, 28 Jan 2020 13:17:46 irqchip/gic-v3: Only provision redistributors that are enabled in ACPI We currently allocate redistributor region structures for individual redistributors when ACPI doesn't present us with compact MMIO regions covering multiple redistributors. It turns out that we allocate these structures even when the redistributor is flagged as disabled by ACPI. It works fine until someone actually tries to tarse one of these structures, and access the corresponding MMIO region. Instead, track the number of enabled redistributors, and only allocate what is required. This makes sure that there is no invalid data to misuse. Signed-off-by: Marc Zyngier <maz@kernel.org> Reported-by: Heyi Guo <guoheyi@huawei.com> Tested-by: Heyi Guo <guoheyi@huawei.com> Link: https://lore.kernel.org/r/20191216062745.63397-1-guoheyi@huawei.com --- drivers/irqchip/irq-gic-v3.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 286f982..c1f7af9 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -1839,6 +1839,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; @@ -1933,8 +1934,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 @@ -1964,8 +1967,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; } ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-08 14:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-16 6:27 [PATCH] irq-gic-v3: fix NULL dereference of disabled redist_base Heyi Guo 2019-12-16 11:14 ` Marc Zyngier 2019-12-16 13:50 ` Guoheyi 2019-12-16 14:23 ` Marc Zyngier 2019-12-17 1:20 ` Guoheyi 2020-02-08 14:58 ` [tip: irq/urgent] irqchip/gic-v3: Only provision redistributors that are enabled in ACPI tip-bot2 for Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®