| 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
| ||
|
Date: Sat, 31 Jul 2021 01:20:19 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: David Collins <collinsd@...eaurora.org>,
linux-kernel@...r.kernel.org
Cc: David Collins <collinsd@...eaurora.org>,
linux-arm-msm@...r.kernel.org, Kiran Gunda <kgunda@...eaurora.org>,
Anirudh Ghayal <aghayal@...eaurora.org>,
Subbaraman Narayanamurthy <subbaram@...eaurora.org>,
Thomas Gleixner <tglx@...utronix.de>,
Marc Zyngier <maz@...nel.org>
Subject: Re: [RESEND PATCH] spmi: spmi-pmic-arb: fix irq_set_type race condition
+irqchip driver maintainers
Quoting David Collins (2021-07-28 11:02:09)
> The qpnpint_irq_set_type() callback function configures the type
> (edge vs level) and polarity (high, low, or both) of a particular
> PMIC interrupt within a given peripheral. To do this, it reads
> the three consecutive IRQ configuration registers, modifies the
> specified IRQ bit within the register values, and finally writes
> the three modified register values back to the PMIC. While a
> spinlock is used to provide mutual exclusion on the SPMI bus
> during the register read and write calls, there is no locking
> around the overall read, modify, write sequence. This opens up
> the possibility of a race condition if two tasks set the type of
> a PMIC IRQ within the same peripheral simultaneously.
>
> When the race condition is encountered, both tasks will read the
> old value of the registers and IRQ bits set by one of the tasks
> will be dropped upon the register write of the other task. This
> then leads to PMIC IRQs being enabled with an incorrect type and
> polarity configured. Such misconfiguration can lead to an IRQ
> storm that overwhelms the system and causes it to crash.
>
> This race condition and IRQ storm have been observed when using
> a pair of pm8941-pwrkey devices to handle PMK8350 pwrkey and
> resin interrupts. The independent devices probe asynchronously
> in parallel and can simultaneously request and configure PMIC
> IRQs in the same PMIC peripheral.
>
> For a good case, the IRQ configuration calls end up serialized
> due to timing deltas and the register read/write sequence looks
> like this:
>
> 1. pwrkey probe: SPMI read(0x1311): 0x00, 0x00, 0x00
> 2. pwrkey probe: SPMI write(0x1311): 0x80, 0x80, 0x80
> 3. resin probe: SPMI read(0x1311): 0x80, 0x80, 0x80
> 4. resin probe: SPMI write(0x1311): 0xC0, 0xC0, 0xC0
>
> The final register states after both devices have requested and
> enabled their respective IRQs is thus:
>
> 0x1311: 0xC0
> 0x1312: 0xC0
> 0x1313: 0xC0
> 0x1314: 0x00
> 0x1315: 0xC0
>
> For a bad case, the IRQ configuration calls end up occurring
> simultaneously and the race condition is encountered. The
> register read/write sequence then looks like this:
>
> 1. pwrkey probe: SPMI read(0x1311): 0x00, 0x00, 0x00
> 2. resin probe: SPMI read(0x1311): 0x00, 0x00, 0x00
> 3. pwrkey probe: SPMI write(0x1311): 0x80, 0x80, 0x80
> 4. resin probe: SPMI write(0x1311): 0x40, 0x40, 0x40
>
> In this case, the final register states after both devices have
> requested and enabled their respective IRQs is thus:
>
> 0x1311: 0x40
> 0x1312: 0x40
> 0x1313: 0x40
> 0x1314: 0x00
> 0x1315: 0xC0
>
> This corresponds to the resin IRQ being configured for both
> rising and falling edges, as expected. However, the pwrkey IRQ
> is misconfigured as level type with both polarity high and low
> set to disabled. The PMIC IRQ triggering hardware treats this
> particular register configuration as if level low triggering is
> enabled.
>
> The raw pwrkey IRQ signal is low when the power key is not being
> pressed. Thus, the pwrkey IRQ begins firing continuously in an
> IRQ storm.
>
> Fix the race condition by locking a spinlock for the duration of
> the read, modify, write sequence in the qpnpint_irq_set_type()
> function.
>
> Fixes: 67b563f1f258 ("spmi: pmic_arb: add support for interrupt handling")
> Signed-off-by: David Collins <collinsd@...eaurora.org>
> ---
> drivers/spmi/spmi-pmic-arb.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index bbbd311eda03..379ad6c1c14a 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -127,6 +127,7 @@ struct apid_data {
> * @intr: address of the SPMI interrupt control registers.
> * @cnfg: address of the PMIC Arbiter configuration registers.
> * @lock: lock to synchronize accesses.
> + * @irq_lock: lock to ensure mutual exclusion for IRQ type setting
> * @channel: execution environment channel to use for accesses.
> * @irq: PMIC ARB interrupt.
> * @ee: the current Execution Environment
> @@ -146,6 +147,7 @@ struct spmi_pmic_arb {
> void __iomem *core;
> resource_size_t core_size;
> raw_spinlock_t lock;
> + raw_spinlock_t irq_lock;
Maybe a better name is type_lock given that it's about the irq type
setting rmw sequence.
> u8 channel;
> int irq;
> u8 ee;
> @@ -600,10 +602,13 @@ static void qpnpint_irq_unmask(struct irq_data *d)
>
> static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
> {
> + struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
> struct spmi_pmic_arb_qpnpint_type type;
> irq_flow_handler_t flow_handler;
> u8 irq = hwirq_to_irq(d->hwirq);
> + unsigned long flags;
>
> + raw_spin_lock_irqsave(&pmic_arb->irq_lock, flags);
> qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
>
> if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
> @@ -616,8 +621,10 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
> flow_handler = handle_edge_irq;
> } else {
> if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
> - (flow_type & (IRQF_TRIGGER_LOW)))
> + (flow_type & (IRQF_TRIGGER_LOW))) {
> + raw_spin_unlock_irqrestore(&pmic_arb->irq_lock, flags);
> return -EINVAL;
> + }
>
> type.type &= ~BIT(irq); /* level trig */
> if (flow_type & IRQF_TRIGGER_HIGH)
> @@ -629,6 +636,8 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
> }
>
> qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
> + raw_spin_unlock_irqrestore(&pmic_arb->irq_lock, flags);
Could we have a qpnpint_spmi_set_bit/clear_bit() API that takes the bit
we want to touch as an argument and then does it all under the originial
pmic_arb->lock? Then we don't need a different lock, we can avoid that
drop the lock under the else if condition above, and the area for the
lock will be contained within the set/clear function instead of here.
> +
> irq_set_handler_locked(d, flow_handler);
>
> return 0;
> @@ -1285,6 +1294,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, ctrl);
> raw_spin_lock_init(&pmic_arb->lock);
> + raw_spin_lock_init(&pmic_arb->irq_lock);
>
> ctrl->cmd = pmic_arb_cmd;
> ctrl->read_cmd = pmic_arb_read_cmd;
Powered by blists - more mailing lists