Re: [PATCH v13 1/6] can: j1939: fix possibility of UAF on module unload

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: premature module_put() in
  __j1939_priv_release_work()
--

commit 9938b84eee258ec84033c319ff76028286604caa
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

can: j1939: fix possibility of UAF on module unload

This commit ties a module reference to each struct j1939_priv and defers
its final release to a dedicated workqueue that is destroyed in
j1939_module_exit().

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index d9384cf0e356f..0b8ac4fe0479d 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
[ ... ]
> @@ -152,9 +159,9 @@ static inline void j1939_priv_set(struct net_device *ndev,
>  	can_ml->j1939_priv = priv;
>  }
>  
> -static void __j1939_priv_release(struct kref *kref)
> +static void __j1939_priv_release_work(struct work_struct *work)
>  {
> -	struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
> +	struct j1939_priv *priv = container_of(work, struct j1939_priv, release_work);
>  	struct net_device *ndev = priv->ndev;
>  
>  	netdev_dbg(priv->ndev, "%s: 0x%p\n", __func__, priv);
> @@ -165,6 +172,22 @@ static void __j1939_priv_release(struct kref *kref)
>  
>  	netdev_put(ndev, &priv->dev_tracker);
>  	kfree(priv);
> +	/* This is safe, for j1939_module_exit() waits for completion of
> +	 * this callback using destroy_workqueue().
> +	 */
> +	module_put(THIS_MODULE);
> +}
> +
> +static void __j1939_priv_release(struct kref *kref)
> +{
> +	struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
> +
> +	/* Since synchronously dropping module refcount of self is not safe,
> +	 * defer actual release to WQ context.
> +	 */
> +	INIT_WORK(&priv->release_work, __j1939_priv_release_work);
> +	queue_work(j1939_wq, &priv->release_work);
> +
>  }

[Severity: Medium]
Can __j1939_priv_release_work() drop the final module reference and allow
can-j1939 to be unmapped before the caller of j1939_priv_put() has finished
executing module code?

Later patches in this series defer struct j1939_ecu destruction to
j1939_wq, but struct j1939_session destruction remains synchronous.  An RX
session created by j1939_xtp_rx_rts_session_new() has no associated socket
and holds a reference to priv, so it can outlive all J1939 sockets.  When
its timer expires, j1939_session_destroy() drops the last reference to
priv before freeing the session and returning:

net/can/j1939/transport.c:j1939_session_destroy() {
    ...
	__j1939_session_drop(session);
	netdev_put(session->priv->ndev, &session->priv_dev_tracker);
	j1939_priv_put(session->priv);
	kfree(session);
}

Because j1939_wq is a WQ_HIGHPRI workqueue and destroy_workqueue(j1939_wq)
only waits for work items on j1939_wq, the module can be unloaded while
the timer callback is still unwinding:

CPU0                              CPU1
j1939_tp_rxtimer()
  j1939_session_put()
    j1939_session_destroy()
      j1939_priv_put()
        __j1939_priv_release()
          queue_work(j1939_wq, ...)
  [preempted]
                                  __j1939_priv_release_work()
                                    kfree(priv)
                                    module_put(THIS_MODULE)
                                  delete_module()
                                    j1939_module_exit()
                                      destroy_workqueue(j1939_wq)
                                    free_module()
      kfree(session)  <-- executes unmapped module text

Deferring session destruction (or the j1939_priv_put() call in
j1939_session_destroy()) to j1939_wq as well would ensure
destroy_workqueue(j1939_wq) waits for all module code paths to finish.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/7cc08b0f-290e-464b-ab13-0a5ce3c57f4f@I-love.SAKURA.ne.jp?part=1