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

[Sergey Shtylyov <s.shtylyov@omprussia.ru>]

On 11/29/20 2:35 PM, 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 yet. :-)

>> 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.

   Maybe it needs to be fixed, rather than using the managed device API?

>>> 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.

   Sigh, that's a stupid typo on my part, being fixed now to pm_runtim_disable()...

> 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.

   OK. :-)

> 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.

   Do you need a link to the fix (it a whole patchset of minor fixes)?

> Thanks,
> 
> Lukas

MBR, Sergei