linux-kernel - Re: [Linux-stm32] [PATCH 2/2] pinctrl: stm32: fix GPIO level interrupts

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <72700f397d32f6a81164aafbf6f9d30b510672cf.camel@foss.st.com>
Date:   Thu, 14 Dec 2023 18:12:46 +0100
From:   Antonio Borneo <antonio.borneo@...s.st.com>
To:     Ben Wolsieffer <ben.wolsieffer@...ring.com>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-gpio@...r.kernel.org>
Subject: Re: [Linux-stm32] [PATCH 2/2] pinctrl: stm32: fix GPIO level
 interrupts

On Mon, 2023-12-04 at 15:33 -0500, Ben Wolsieffer wrote:
> The STM32 doesn't support GPIO level interrupts in hardware, so the
> driver tries to emulate them using edge interrupts, by retriggering the
> interrupt if necessary based on the pin state after the handler
> finishes.
> 
> Currently, this functionality does not work because the irqchip uses
> handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask()
> callbacks after handling the interrupt. This patch fixes this by using
> handle_level_irq() for level interrupts, which causes irq_unmask() to be
> called to retrigger the interrupt.
> 
> Signed-off-by: Ben Wolsieffer <ben.wolsieffer@...ring.com>
> ---
>  drivers/pinctrl/stm32/pinctrl-stm32.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 603f900e88c1..fb9532601cbb 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -348,12 +348,15 @@ static int stm32_gpio_set_type(struct irq_data *d, unsigned int type)
>         case IRQ_TYPE_EDGE_RISING:
>         case IRQ_TYPE_EDGE_FALLING:
>         case IRQ_TYPE_EDGE_BOTH:
> +               irq_set_handler_locked(d, handle_edge_irq);

Hi,
this patch causes a NULL pointer dereference and crashes the kernel boot on STM32 MPU's,
either STM32MP13x, STM32MP15x and the new STM32MP25x.

Please do not merge it as is.

This pinctrl-stm32 driver is shared between STM32 MCUs and MPUs.
In both cases the EXTI is the parent interrupt controller of this pinctrl, but despite
the fact that there is a single file irq-stm32-exti.c, it contains two independent
drivers, one for MCUs and the other for MPUs.
Swapping in this function the irq_desc::handle_irq between handle_edge_irq() and
handle_level_irq() is probably fine for MCU (I have not tested it).
But on MPUs the default handler is handle_fasteoi_irq(); should not be changed here.

Checking quickly ... this function calls irq_chip_set_type_parent() at the very end.
It will in turn call EXTI's irq_set_type(), which has different implementations for MCU
and MPU.
By moving this handler swap in the MCU specific stm32_irq_set_type() it will not impact
MPUs.

Best Regards,
Antonio


>                 parent_type = type;
>                 break;
>         case IRQ_TYPE_LEVEL_HIGH:
> +               irq_set_handler_locked(d, handle_level_irq);
>                 parent_type = IRQ_TYPE_EDGE_RISING;
>                 break;
>         case IRQ_TYPE_LEVEL_LOW:
> +               irq_set_handler_locked(d, handle_level_irq);
>                 parent_type = IRQ_TYPE_EDGE_FALLING;
>                 break;
>         default: