[v1] thunderbolt: Fix use-after-free in tb_dp_dprx_work

[PATCH] thunderbolt: Fix use-after-free in tb_dp_dprx_work

Posted by Duoming Zhou 4 months, 2 weeks ago

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@zju.edu.cn>
---
 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;
+
 	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);
 	}
 }
 
-- 
2.34.1

Re: [PATCH] thunderbolt: Fix use-after-free in tb_dp_dprx_work

Posted by Mika Westerberg 4 months, 2 weeks ago

Hi,

On Mon, Sep 22, 2025 at 01:18:59PM +0800, Duoming Zhou 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@zju.edu.cn>
> ---
>  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);

>  	}
>  }
>  
> -- 
> 2.34.1

Re: [PATCH] thunderbolt: Fix use-after-free in tb_dp_dprx_work

Posted by duoming@zju.edu.cn 4 months, 2 weeks ago

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@zju.edu.cn>
> > ---
> >  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