Re: [PATCH for-5.10] spi: rpc-if: Fix use-after-free on unbind

From: Lukas Wunner <lukas@wunner.de>
To: Sergey Shtylyov <s.shtylyov@omprussia.ru>
Cc: Mark Brown <broonie@kernel.org>,
	linux-spi@vger.kernel.org,
	Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Subject: Re: [PATCH for-5.10] spi: rpc-if: Fix use-after-free on unbind
Date: Sun, 29 Nov 2020 12:35:48 +0100	[thread overview]
Message-ID: <20201129113548.GA2587@wunner.de> (raw)
In-Reply-To: <9534f4fb-6f5e-b538-6903-e702a7301b1d@omprussia.ru>

On Sat, Nov 28, 2020 at 11:20:50PM +0300, Sergey Shtylyov wrote:
> On 11/16/20 11:23 AM, Lukas Wunner wrote:
> > rpcif_spi_remove() accesses the driver's private data after calling
> > spi_unregister_controller() even though that function releases the last
> > reference on the spi_controller and thereby frees the private data.
> 
> OK, your analysis seems correct (sorry for the delay admitting this :-).

Thanks!  Is it okay to take this for an Acked-by?

> Not sure why spi_unregister_controller() drops the device reference
> while spi_register_controller() itself doesn't allocate the memory... 

Yes, that's exactly what I'm trying to move away from with
devm_spi_alloc_master() (introduced in v5.10-rc5 by 5e844cc37a5c).
The API as it has been so far has made it really easy to shoot oneself
in the foot.

> > Fix by switching over to the new devm_spi_alloc_master() helper which
> > keeps the private data accessible until the driver has unbound.
> 
>    Perhaps the order of the calls in the remove() method could be reversed? 

I'm not familiar with power management on these Renesas controllers
but rpcif_disable_rpm() calls pm_runtime_put_sync(), which I assume
may put the controller to sleep.

SPI transfers may still be ongoing until spi_unregister_controller()
returns.  Specifically, this function unbinds and unregisters all
SPI slaves attached to the controller and the slaves' drivers may
need to perform SPI transfers to quiesce interrupts on the slaves etc.

Thus, the correct order is to call spi_unregister_controller() first
and only then perform further teardown steps.  So the order in
rpcif_spi_remove() seems correct to me.

The only thing that looks confusing is that rpcif_enable_rpm() calls
pm_runtime_enable(), whereas rpcif_disable_rpm() calls
pm_runtime_put_sync().  That looks incongruent.

Thanks,

Lukas