linux-kernel - Re: [PATCH 1/2] spi: spi-fsl-dspi: Fix external abort on interrupt in exit paths

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 15 Jun 2020 09:15:40 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Vladimir Oltean <vladimir.oltean@....com>,
        linux-spi <linux-spi@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Wolfram Sang <wsa@...nel.org>, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] spi: spi-fsl-dspi: Fix external abort on interrupt
 in exit paths

On Sun, Jun 14, 2020 at 06:48:04PM +0300, Vladimir Oltean wrote:
> On Sun, 14 Jun 2020 at 13:57, Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >
> > If interrupt comes late, during probe error path or device remove (could
> > be triggered with CONFIG_DEBUG_SHIRQ), the interrupt handler
> > dspi_interrupt() will access registers with the clock being disabled.  This
> > leads to external abort on non-linefetch on Toradex Colibri VF50 module
> > (with Vybrid VF5xx):
> >
> >     $ echo 4002d000.spi > /sys/devices/platform/soc/40000000.bus/4002d000.spi/driver/unbind
> >
> >     Unhandled fault: external abort on non-linefetch (0x1008) at 0x8887f02c
> >     Internal error: : 1008 [#1] ARM
> >     CPU: 0 PID: 136 Comm: sh Not tainted 5.7.0-next-20200610-00009-g5c913fa0f9c5-dirty #74
> >     Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
> >       (regmap_mmio_read32le) from [<8061885c>] (regmap_mmio_read+0x48/0x68)
> >       (regmap_mmio_read) from [<8060e3b8>] (_regmap_bus_reg_read+0x24/0x28)
> >       (_regmap_bus_reg_read) from [<80611c50>] (_regmap_read+0x70/0x1c0)
> >       (_regmap_read) from [<80611dec>] (regmap_read+0x4c/0x6c)
> >       (regmap_read) from [<80678ca0>] (dspi_interrupt+0x3c/0xa8)
> >       (dspi_interrupt) from [<8017acec>] (free_irq+0x26c/0x3cc)
> >       (free_irq) from [<8017dcec>] (devm_irq_release+0x1c/0x20)
> >       (devm_irq_release) from [<805f98ec>] (release_nodes+0x1e4/0x298)
> >       (release_nodes) from [<805f9ac8>] (devres_release_all+0x40/0x60)
> >       (devres_release_all) from [<805f5134>] (device_release_driver_internal+0x108/0x1ac)
> >       (device_release_driver_internal) from [<805f521c>] (device_driver_detach+0x20/0x24)
> >
> > The resource-managed framework should not be used for interrupt handling,
> > because the resource will be released too late - after disabling clocks.
> > The interrupt handler is not prepared for such case.
> >
> > Fixes: 349ad66c0ab0 ("spi:Add Freescale DSPI driver for Vybrid VF610 platform")
> > Cc: <stable@...r.kernel.org>
> > Signed-off-by: Krzysztof Kozlowski <krzk@...nel.org>
> >
> > ---
> 
> I don't buy this argument that "the resource-managed framework should
> not be used for interrupt handling". What is it there for, then?

It was created long time ago for memory allocations and since then
people ported to all other possibilities and used in drivers.  Just
because you can do something, does not necessarily mean that you
should...

> Could you just call disable_irq before clk_disable_unprepare instead
> of this massive rework?

This massive rework is 9 insertions and 4 deletions, indeed I made
impressive, huge commit with significant impact. disable_irq() could work
as well so if this is preferred, no problem from my side.

Best regards,
Krzysztof