Re: [PATCH 6.12 528/602] thunderbolt: Prevent XDomain delayed work use-after-free on disconnect

Hi Greg/Sasha,

On 30/07/26 7:45 pm, Greg Kroah-Hartman wrote:
> 6.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Michael Bommarito <[email protected]>
> 
> [ Upstream commit 2c5d2d3c3f70cde2565d7b279b544893a2035842 ]
> 
> tb_xdp_handle_request() runs on system_wq and queues
> xd->state_work via queue_delayed_work() in three request handlers:
> PROPERTIES_CHANGED_REQUEST, UUID_REQUEST (via start_handshake),
> and LINK_STATE_CHANGE_REQUEST.  Similarly, update_xdomain() queues
> xd->properties_changed_work when local properties change.
> 
> Concurrently, tb_xdomain_remove() calls stop_handshake() which does
> cancel_delayed_work_sync() on both delayed works.  Later,
> tb_xdomain_unregister() calls device_unregister() which eventually
> frees the xdomain.  Since commit 559c1e1e0134 ("thunderbolt: Run
> tb_xdp_handle_request() in system workqueue") moved the request
> handler off tb->wq, the handler and the remove path are no longer
> serialized.  If queue_delayed_work() executes after
> cancel_delayed_work_sync() but before the xdomain is freed, the
> delayed work fires on a freed object.
> 
> Add xd->removing that tb_xdomain_remove() sets under xd->lock
> before calling stop_handshake().  

let's remember this.

> Each external queue site holds
> the same lock and checks removing before calling
> queue_delayed_work().  This provides the mutual exclusion needed:
> either the queue site acquires the lock first and queues work that
> the subsequent cancel will see, or the remove path acquires the
> lock first and the queue site observes removing == true and skips
> the queue.
> 
> Fixes: 559c1e1e0134 ("thunderbolt: Run tb_xdp_handle_request() in system workqueue")
> Cc: [email protected]
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Michael Bommarito <[email protected]>
> Signed-off-by: Mika Westerberg <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>   drivers/thunderbolt/xdomain.c |   26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> --- a/drivers/thunderbolt/xdomain.c
> +++ b/drivers/thunderbolt/xdomain.c
> @@ -905,6 +905,19 @@ void tb_unregister_service_driver(struct
>   }
>   EXPORT_SYMBOL_GPL(tb_unregister_service_driver);
>   
> +static int update_xdomain(struct device *dev, void *data)
> +{
> +	struct tb_xdomain *xd;
> +
> +	xd = tb_to_xdomain(dev);
> +	if (xd) {
> +		queue_delayed_work(xd->tb->wq, &xd->properties_changed_work,
> +				   msecs_to_jiffies(50));
> +	}
> +
> +	return 0;
> +}
> +


I have run an AI assisted backport review and it spotted an issue. I
have taken a look, and this backport does not contain the fix described
by upstream 2c5d2d3c3f70.

Upstream adds a removing flag and protects queueing, for example:

     mutex_lock(&xd->lock);
     if (!xd->removing)
             queue_delayed_work(xd->tb->wq,
                                &xd->properties_changed_work, ...);
     mutex_unlock(&xd->lock);

It also sets xd->removing under the same lock before
stop_handshake() cancels delayed work.

The 6.12.y commit still has:

     if (xd)
             queue_delayed_work(xd->tb->wq,
                                &xd->properties_changed_work, ...);

and no removing field or guarded request-handler queue sites. Its diff
only relocates the unchanged update_xdomain() function, so work can
still be queued after cancellation and run after the xdomain is freed.

So I think this is incomplete backport ?


thoughts ?


Thanks,
Harshit

>   static ssize_t key_show(struct device *dev, struct device_attribute *attr,
>   			char *buf)
>   {
> @@ -2480,19 +2493,6 @@ bool tb_xdomain_handle_request(struct tb
>   	return ret > 0;
>   }
>   
> -static int update_xdomain(struct device *dev, void *data)
> -{
> -	struct tb_xdomain *xd;
> -
> -	xd = tb_to_xdomain(dev);
> -	if (xd) {
> -		queue_delayed_work(xd->tb->wq, &xd->properties_changed_work,
> -				   msecs_to_jiffies(50));
> -	}
> -
> -	return 0;
> -}
> -
>   static void update_all_xdomains(void)
>   {
>   	bus_for_each_dev(&tb_bus_type, NULL, NULL, update_xdomain);
> 
> 
>