linux-kernel - Re: [PATCH] arm64: kdump: fix interrupt handling done during machine_crash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Mar 2018 17:05:09 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Grzegorz Jaszczyk <jaz@...ihalf.com>
Cc:     Mark Rutland <mark.rutland@....com>, catalin.marinas@....com,
        will.deacon@....com, james.morse@....com,
        "AKASHI, Takahiro" <takahiro.akashi@...aro.org>,
        Hoeun Ryu <hoeun.ryu@...il.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Nadav Haklai <nadavh@...vell.com>,
        Marcin Wojtas <mw@...ihalf.com>
Subject: Re: [PATCH] arm64: kdump: fix interrupt handling done during
 machine_crash_shutdown

On 09/03/18 10:33, Grzegorz Jaszczyk wrote:
>> Not using EOImode==1 is definitely an oddity (at least on the host), but
>> that doesn't mean it shouldn't work.
>>
>> The reason the thing is hanging is that although we correctly deactivate
>> the interrupt, nothing performs the priority drop. Your write to EOI
>> helps in the sense that it guarantees that both priority drop and
>> deactivate are done with the same operation, but that's not something
>> we'd want to expose.
>>
>> My preferred approach would be to nuke the active priority registers at
>> boot time, as the CPUs come up. I'll try to write something this week.
> 
> I've made a PoC which performs priority drop at boot time as you
> suggested and it works with both GICv2 and GICv3 but I see some
> problem:
> It seems that the only way to drop the priority is to perform write to
> EOI register (the GIC_RPR is RO). According to GIC documentation a
> write to EOI register must correspond to the most recent valid read
> from IAR. The problem is that the interrupt was already acked in the
> 'original' kernel, so reading GICC_IAR in crashdump kernel returns
> spurious interrupt and it seems that there is no way to figure out
> appropriate irqnr for EOI write. Nevertheless I've observed that
> choosing random irqnr for EOI write works fine (maybe because all
> interrupts in Linux uses the same priority?).
> 
> Here is the PoC (not ready for submission only for further
> discussion): https://pastebin.com/gLYNuRiZ
> 
> Looking forward to your feedback.

Well, the feedback is that this patch is really horrible, and choosing a 
random IRQ is at best hilarious. It also doesn't account for multiple 
priorities being active... In short, this is just fundamentally broken.

I gave you a clue about the active priority registers. Why didn't you 
try that?

Anyway, here's something that should fix it.

	M.

>From 6ace9d56c96203c4d11a23cbec6a6c216164bb88 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@....com>
Date: Fri, 9 Mar 2018 14:53:19 +0000
Subject: [PATCH] irqchip/gic-v2: Reset APRn registers at boot time

Booting a crash kernel while in an interrupt handler is likely
to leave the Active Priority Registers with some state that
is not relevant to the new kernel, and is likely to lead
to erratic behaviours such as interrupts not firing as their
priority is already active.

As a sanity measure, wipe the APRs clean on startup.

Signed-off-by: Marc Zyngier <marc.zyngier@....com>
---
 drivers/irqchip/irq-gic.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 121af5cf688f..79801c24800b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -453,15 +453,26 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 	return mask;
 }
 
+static bool gic_check_gicv2(void __iomem *base)
+{
+	u32 val = readl_relaxed(base + GIC_CPU_IDENT);
+	return (val & 0xff0fff) == 0x02043B;
+}
+
 static void gic_cpu_if_up(struct gic_chip_data *gic)
 {
 	void __iomem *cpu_base = gic_data_cpu_base(gic);
 	u32 bypass = 0;
 	u32 mode = 0;
+	int i;
 
 	if (gic == &gic_data[0] && static_key_true(&supports_deactivate))
 		mode = GIC_CPU_CTRL_EOImodeNS;
 
+	if (gic_check_gicv2(cpu_base))
+		for (i = 0; i < 4; i++)
+			writel_relaxed(0, cpu_base + GIC_CPU_ACTIVEPRIO + i * 4);
+
 	/*
 	* Preserve bypass disable bits to be written back later
 	*/
@@ -1264,12 +1275,6 @@ static int __init gicv2_force_probe_cfg(char *buf)
 }
 early_param("irqchip.gicv2_force_probe", gicv2_force_probe_cfg);
 
-static bool gic_check_gicv2(void __iomem *base)
-{
-	u32 val = readl_relaxed(base + GIC_CPU_IDENT);
-	return (val & 0xff0fff) == 0x02043B;
-}
-
 static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 {
 	struct resource cpuif_res;
-- 
2.14.2


-- 
Jazz is not dead. It just smells funny...