Re: [PATCH net v4 2/2] s390/ctcm: Fix use-after-free in channel_remove()

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch waits for channel timers and tasklets too late.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is the same
  cancel-without-sync-then-free pattern in sibling fsm_timers.

--

s390/ctcm: Fix use-after-free in channel_remove()

In channel_remove(), this patch replaces fsm_deltimer() with
timer_shutdown_sync() for ch->timer and ch->sweep_timer. It also moves
the MPC tasklet_kill() calls and kfree(ch->discontact_th) so they run
after the timers are shut down and before kfree_fsm(ch->fsm).

Link: https://sashiko.dev/#/patchset/20260922101913.239103-1-nagamani@linux.ibm.com?part=2

[Severity: High]
Do these waits run too late to protect the state that the channel
callbacks use?

The only teardown caller of channel_remove() is ctcm_shutdown_device(),
and it calls it after the net_device and its private state are gone:

drivers/s390/net/ctcm_main.c:ctcm_shutdown_device() {
    ...
	if (dev) {
		unregister_netdev(dev);
		ctcm_free_netdevice(dev);
	}
    ...
	ccw_device_set_offline(cgdev->cdev[1]);
	ccw_device_set_offline(cgdev->cdev[0]);
	channel_remove(priv->channel[CTCM_READ]);
	channel_remove(priv->channel[CTCM_WRITE]);
    ...
}

ctcm_free_netdevice() does kfree_fsm(grp->fsm), kfree(grp),
priv->mpcg = NULL, kfree_fsm(priv->fsm) and priv->fsm = NULL. Under MODULE
it also calls free_netdev(dev). The ccw_device_set_offline() calls in
between can sleep, so a channel timer or tasklet can fire in that window.

The channel callbacks reach this state through ch->netdev->ml_priv:

  ctcm_chx_txretry() (ch->timer):
	fsm_event(priv->fsm, DEV_EVENT_TXDOWN, dev);

  ctcmpc_chx_send_sweep() (sweep_timer):
	if (grp->in_sweep == 0)

  ctcmpc_bh() (ch_tasklet):
	while ((fsm_getstate(grp->fsm) != MPCG_STATE_FLOWC) &&

  ctcm_ccw_check_rc()->ctcm_chx_iofatal():
	fsm_event(priv->fsm, ...);

The circular chain in the commit message also goes through priv->fsm or
priv->mpcg. For example, mpc_action_go_inop() does this before it reaches
tasklet_hi_schedule():

	grp =  priv->mpcg;
	grp->flow_off_called = 0;

At channel_remove() time priv->fsm and priv->mpcg are already freed or
NULL. Wouldn't that chain crash before it can re-arm anything?

If so, timer_shutdown_sync() only waits for a callback that is already
dereferencing freed or NULL memory. tasklet_kill() also lets a queued
ch_tasklet run against the freed grp.

channel_free() moves ch->fsm to CTC_STATE_IDLE, which has no handlers,
so most late ch->fsm events do nothing. But that state change takes no
lock. ctcm_irq_handler() runs FSM actions under the ccwdev lock, and for
MPC ctcm_close() does nothing:

	if (!IS_MPC(priv))
		fsm_event(priv->fsm, DEV_EVENT_STOP, dev);

As a result the MPC channels are not halted. An action that races
channel_free() could leave the channel FSM in TXIDLE or TX with timers
armed.

ctcmpc_bh() never checks the ch->fsm state, and ctc_mpc_flow_control()
can schedule it at any time.

The ctcm_new_device() out_dev path uses the same order, but its channels
were never started there.

Could both channels' timers and tasklets be stopped, and the subchannels
taken offline, before ctcm_free_netdevice() and kfree_fsm(priv->fsm) run?
That would mean moving the quiescing out of channel_remove() instead of
reordering it inside channel_remove().

[Severity: High]
This isn't a bug introduced by this patch, but the other fsm_timers in
this driver seem to follow the same pattern: cancel without a sync, then
free.

priv->restart_timer is bound to priv->fsm by fsm_settimer() in
ctcm_init_netdevice(). grp->timer is bound to grp->fsm by fsm_settimer()
in ctcmpc_init_mpc_group(). Neither timer is ever cancelled with a sync
call. The only cancellations are fsm_deltimer(), which is timer_delete().

mpc_action_go_inop() always re-arms the restart timer:

	fsm_deltimer(&priv->restart_timer);
	fsm_addtimer(&priv->restart_timer, 500, DEV_EVENT_RESTART, dev);

dev_action_restart() re-arms it for 1s in the MPC case. So during MPC
group recovery the restart timer is usually pending.

For MPC, ctcm_close() does nothing. So ctcm_shutdown_device() then calls
ctcm_free_netdevice(), which frees grp->fsm, grp (which contains
grp->timer) and priv->fsm while these timers may be pending or running.
After that, ctcm_remove_device() does kfree(priv), and priv contains
restart_timer.

When one of these timers expires, fsm_expire_timer() calls
fsm_event(this->fi, ...) on a freed fsm_instance. That reads the freed
state and jumpmatrix, and calls a function pointer taken from freed
memory. A timer still pending inside freed grp or priv memory can also
corrupt the timer wheel.

There is a similar ordering problem in ctcm_free_netdevice():

	if (grp->fsm)
		kfree_fsm(grp->fsm);
	dev_kfree_skb(grp->xid_skb);
	dev_kfree_skb(grp->rcvd_xid_skb);
	tasklet_kill(&grp->mpc_tasklet2);
	kfree(grp);

Can a pending mpc_tasklet2 run here after its FSM has been freed?

Should priv->restart_timer and grp->timer also get timer_shutdown_sync()
before priv->fsm and grp are freed? And should mpc_tasklet2 be killed
before kfree_fsm(grp->fsm)?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930070234.569500-1-nagamani%40linux.ibm.com