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]

Message-ID: <20200615075109.GA22427@kozik-lap>
Date:   Mon, 15 Jun 2020 09:51:09 +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 Mon, Jun 15, 2020 at 09:15:40AM +0200, Krzysztof Kozlowski wrote:
> 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.

disable_irq() should fix real world case but won't fix DEBUG_SHIRQ.
I'll rework it as well but then we go to bigger change again.

Best regards,
Krzysztof