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

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

On 29/09/26 5:04 AM, Jakub Kicinski wrote:

Thank you for the detailed review. Responses inline.

> [Severity: Low]
> This isn't a bug, but "Patch 1" and "patch 2" won't mean anything once
> these commits are in git history.
> 
> The prerequisite is "s390/ctcm: Fix timer corruption in fsm_addtimer()".
> Its Fixes: tag (e99e88a9d2b0) is different from the one on this commit
> (293d984f0e36). Without it, fsm_addtimer() calls timer_setup() again, which
> restores ->function and undoes timer_shutdown_sync(&ch->sweep_timer.tl).
> 
> Could the commit message name the prerequisite by title or SHA? Could the
> Cc: stable line also use the "Cc: stable@vger.kernel.org # <ver>: <sha>:
> <title>" form, so the dependency is recorded in a way tools can pick up?
> 

Fixed in v4:
- Named the prerequisite by commit title ("s390/ctcm: Fix timer
  corruption in fsm_addtimer()") throughout the commit message.
- Updated the tag to structured format:
  Cc: stable@vger.kernel.org # depends on: s390/ctcm: Fix timer corruption in fsm_addtimer()

> 
> [Severity: High]
> Is this quiesce early enough? On the offline path, channel_remove() only
> runs after ctcm_free_netdevice() has already freed the shared MPC state:
> 
> ctcm_shutdown_device()
>     channel_free(priv->channel[CTCM_READ])
>     channel_free(priv->channel[CTCM_WRITE])
>     unregister_netdev(dev)
>     ctcm_free_netdevice(dev)
>         kfree_fsm(grp->fsm)
>         kfree(grp)
>         priv->mpcg = NULL
>         kfree_fsm(priv->fsm)
>         free_netdev(dev)        /* MODULE builds only */
>     ...
>     channel_remove(priv->channel[CTCM_READ])
>     channel_remove(priv->channel[CTCM_WRITE])
> 
> ctcmpc_bh(), the ch_tasklet handler, dereferences these objects without
> checking the channel FSM state. Setting CTC_STATE_IDLE in channel_free()
> therefore doesn't stop a tasklet that is already pending:
> 
> drivers/s390/net/ctcm_mpc.c:ctcmpc_bh() {
> 	struct net_device *dev	= ch->netdev;
> 	struct ctcm_priv  *priv	= dev->ml_priv;
> 	struct mpc_group  *grp	= priv->mpcg;
>     ...
> 	while ((fsm_getstate(grp->fsm) != MPCG_STATE_FLOWC) &&
>     ...
> }
> 
> Suppose the RX path scheduled rch->ch_tasklet just before the device was
> taken offline, and the tasklet is still pending (for example deferred to
> ksoftirqd) when ctcm_free_netdevice() runs. Can it then dereference a NULL
> priv->mpcg, or a freed dev in MODULE builds, before channel_remove() gets
> to tasklet_kill()?
> 
> Something similar can happen with sweep_timer. An in-flight action in
> ctcmpc_chx_send_sweep() can overwrite the CTC_STATE_IDLE set by
> channel_free() and re-arm wch->timer:
> 
> 	fsm_addtimer(&wch->timer, CTCM_TIME_5_SEC, CTC_EVENT_TIMER, wch);
> 	fsm_newstate(wch->fsm, CTC_STATE_TX);
> 
> If that timer fires before channel_remove(), the TX timer action would use
> ch->netdev and priv->mpcg, which by then are freed or NULL.
> 
> The error path in ctcm_new_device() has the same order: it calls
> ctcm_free_netdevice() at out_dev and channel_remove() on both channels
> afterwards.
> 
> Would it make sense to quiesce the tasklets and timers of both channels
> before ctcm_free_netdevice() is called?
> 

Confirmed pre-existing bug in ctcm_shutdown_device() / ctcm_new_device()
error path, outside the scope of channel_remove(). Aswin Karuvally has
picked this up and is preparing a fix in a follow-up series.

> 
> [Severity: Medium]
> Can a timer callback schedule ch_disc_tasklet again after the
> tasklet_kill() above?
> 
> tasklet_kill() waits for the current run and clears TASKLET_STATE_SCHED.
> It doesn't stop a later tasklet_hi_schedule(), and the timer actions can
> reach one. 
> [ ... ]> Would a second tasklet_kill() after the timers are synced, or a
> tasklet_disable() before them, close this?
> 

Fixed in v4 by shutting down timers before tasklet_kill():

  if (IS_MPC(ch))
      timer_shutdown_sync(&ch->sweep_timer.tl);
  timer_shutdown_sync(&ch->timer.tl);
  if (IS_MPC(ch)) {
      tasklet_kill(&ch->ch_tasklet);
      tasklet_kill(&ch->ch_disc_tasklet);
      kfree(ch->discontact_th);
  }

timer_shutdown_sync() waits for any active callback and prevents further
rearming. Shutting down sweep_timer first breaks the sweep -> ch->timer
re-arm chain, and shutting down ch->timer prevents scheduling
ch_disc_tasklet via mpc_action_go_inop(). tasklet_kill() then cleans up
any in-flight instances.

timer_shutdown_sync() is also used for non-MPC ch->timer since
ctcm_chx_txretry() can re-arm ch->timer on that path as well.

> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the same removal sequence
> seems to leave two other FSM timers running on freed memory.
> 
> ctcmpc_init_mpc_group() binds grp->timer, which is embedded in grp, with
> fsm_settimer(grp->fsm, &grp->timer). ctcm_init_netdevice() binds
> priv->restart_timer to priv->fsm. ctcm_free_netdevice() frees all of these
> without first cancelling either timer synchronously:
> 
> [ ... ]

> Should grp->timer and priv->restart_timer be stopped with
> timer_delete_sync() or timer_shutdown_sync() before ctcm_free_netdevice()
> frees them?
> 
Confirmed pre-existing bug in ctcm_free_netdevice(). Aswin Karuvally has
picked this up for the follow-up series as well.

These changes are included in the v4 series posted to the list.

Nagamani