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

[Nagamani PV <nagamani@linux.ibm.com>]

On 04/10/26 12:47 PM, netdev-bot+sashiko@kernel.org wrote:
> [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)?
> 
> [ ... ]
> 

Both are confirmed pre-existing issues in ctcm_shutdown_device() and
ctcm_free_netdevice(), outside the scope of channel_remove(). They are
tracked for a separate follow-up fix.

Nagamani