[PATCH v2] HSI: ssi_protocol: cancel xmit work in the stop path, not ssip_reset

[PATCH v2] HSI: ssi_protocol: cancel xmit work in the stop path, not ssip_reset

[Hyunjung Ko]

ssip_reset() runs in atomic context on several paths: the rx_wd and tx_wd
watchdog timers reach it through ssip_error(), and so do the HSI message
->complete callbacks (ssip_rx_data_complete(), ssip_rxcmd_complete(),
ssip_tx_data_complete()). cancel_work_sync() has an unconditional
might_sleep(), so calling it from ssip_reset() is invalid on those paths.
It is also called under spin_lock_bh(&ssi->lock), and the cancelled work
ssip_xmit_work() -> ssip_xmit() takes ssi->lock itself, so waiting for it
while holding the lock can deadlock.

cancel_work_sync(&ssi->work) was added to ssip_reset() to stop
ssip_xmit_work() from running after the device is torn down and freed
(see the Fixes commit). Move it to ssip_pn_stop() instead, which runs in
process context and is still on the teardown path
(ssi_protocol_remove() -> unregister_netdev() -> ndo_stop), so the
use-after-free fix is preserved. The watchdog and error reset paths do
not free the device, and ssip_xmit_work() bails out once main_state is no
longer ACTIVE, so they do not need a synchronous cancel.

Fixes: e3f88665a780 ("HSI: ssi_protocol: Fix use after free vulnerability in ssi_protocol Driver Due to Race Condition")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
Changes in v2:
 - v1 only moved cancel_work_sync() out of the ssi->lock section. Andy
   Shevchenko pointed out the ordering vs. the timers; looking closer,
   ssip_reset() is also reached from softirq (the rx_wd/tx_wd watchdog
   timers and the ->complete callbacks), where cancel_work_sync() cannot
   be called at all. So remove it from ssip_reset() entirely and cancel
   the work in the process-context stop path instead.
 - The three timers never queue ssi->work (keep_alive only re-arms
   itself; rx_wd/tx_wd call ssip_error()); the work is queued only by the
   xmit path, so dropping the sync cancel from the reset paths is safe.

 drivers/hsi/clients/ssi_protocol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hsi/clients/ssi_protocol.c b/drivers/hsi/clients/ssi_protocol.c
index 7c6ca83..aa2d6ed 100644
--- a/drivers/hsi/clients/ssi_protocol.c
+++ b/drivers/hsi/clients/ssi_protocol.c
@@ -401,7 +401,6 @@ static void ssip_reset(struct hsi_client *cl)
 	timer_delete(&ssi->rx_wd);
 	timer_delete(&ssi->tx_wd);
 	timer_delete(&ssi->keep_alive);
-	cancel_work_sync(&ssi->work);
 	ssi->main_state = 0;
 	ssi->send_state = 0;
 	ssi->recv_state = 0;
@@ -950,8 +949,10 @@ static int ssip_pn_open(struct net_device *dev)
 static int ssip_pn_stop(struct net_device *dev)
 {
 	struct hsi_client *cl = to_hsi_client(dev->dev.parent);
+	struct ssi_protocol *ssi = hsi_client_drvdata(cl);

 	ssip_reset(cl);
+	cancel_work_sync(&ssi->work);
 	hsi_unregister_port_event(cl);
 	hsi_release_port(cl);