On Mon, 22 Sep 2025 14:54:43 +0200 Mika Westerberg wrote:
> > The original code relies on cancel_delayed_work() in tb_dp_dprx_stop(),
> > which does not ensure that the delayed work item tunnel->dprx_work has
> > fully completed if it was already running. This leads to use-after-free
> > scenarios where tb_tunnel is deallocated by tb_tunnel_put(), while
> > tunnel->dprx_work remains active and attempts to dereference tb_tunnel
> > in tb_dp_dprx_work().
> >
> > A typical race condition is illustrated below:
> >
> > CPU 0 | CPU 1
> > tb_dp_tunnel_active() |
> > tb_deactivate_and_free_tunnel()| tb_dp_dprx_start()
> > tb_tunnel_deactivate() | queue_delayed_work()
> > tb_dp_activate() |
> > tb_dp_dprx_stop() | tb_dp_dprx_work() //delayed worker
> > cancel_delayed_work() |
> > tb_tunnel_put(tunnel); |
> > | tunnel = container_of(...); //UAF
> > | tunnel-> //UAF
> >
> > Replacing cancel_delayed_work() with cancel_delayed_work_sync() is
> > not feasible as it would introduce a deadlock: both tb_dp_dprx_work()
> > and the cleanup path acquire tb->lock, and cancel_delayed_work_sync()
> > would wait indefinitely for the work item that cannot proceed.
> >
> > Instead, implement proper reference counting:
> > - If cancel_delayed_work() returns true (work is pending), we release
> > the reference in the stop function.
> > - If it returns false (work is executing or already completed), the
> > reference is released in delayed work function itself.
> >
> > This ensures the tb_tunnel remains valid during work item execution
> > while preventing memory leaks.
> >
> > This bug was found by static analysis.
> >
> > Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
> > Signed-off-by: Duoming Zhou <duoming@xxxxxxxxxx>
> > ---
> > drivers/thunderbolt/tunnel.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
> > index d52efe3f658c..89fa0c626d3e 100644
> > --- a/drivers/thunderbolt/tunnel.c
> > +++ b/drivers/thunderbolt/tunnel.c
> > @@ -1073,6 +1073,7 @@ static void tb_dp_dprx_work(struct work_struct *work)
> >
> > if (tunnel->callback)
> > tunnel->callback(tunnel, tunnel->callback_data);
> > + tb_tunnel_put(tunnel);
> > }
> >
> > static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
> > @@ -1097,11 +1098,14 @@ static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
> >
> > static void tb_dp_dprx_stop(struct tb_tunnel *tunnel)
> > {
> > + bool ret;
> > +
>
> Why you need variable here?
>
> > if (tunnel->dprx_started) {
> > tunnel->dprx_started = false;
> > tunnel->dprx_canceled = true;
> > - cancel_delayed_work(&tunnel->dprx_work);
> > - tb_tunnel_put(tunnel);
> > + ret = cancel_delayed_work(&tunnel->dprx_work);
> > + if (ret)
> > + tb_tunnel_put(tunnel);
>
> Just do:
>
> if (cancel_delayed_work(...))
> tb_tunnel_put(tunnel);
>
Thank you for your suggestions, I will modify the code
and send the patch v2.
Best regards,
Duoming Zhou
