Re: [PATCH v2 2/2] drm/xe/dma-buf: fix UAF with retry loop

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Thu, 2026-05-07 at 12:55 +0100, Matthew Auld wrote:
> Retry doesn't work here, since bo will be freed on error, leading to
> UAF. However, now that we do the alloc & init before the attach, we
> can
> now combine this as one unit and have the init do the alloc for us.
> This
> should make the retry safe.
> 
> Reported by Sashiko.
> 
> Closes:
> https://sashiko.dev/#/patchset/20260506184332.86743-2-matthew.auld%40intel.com
> Fixes: eb289a5f6cc6 ("drm/xe: Convert xe_dma_buf.c for exhaustive
> eviction")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <stable@vger.kernel.org> # v6.18+

lgtm.
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>



> ---
>  drivers/gpu/drm/xe/xe_dma_buf.c | 42 +++++++++----------------------
> --
>  1 file changed, 11 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_dma_buf.c
> b/drivers/gpu/drm/xe/xe_dma_buf.c
> index 2332db502c8b..a54ec278a915 100644
> --- a/drivers/gpu/drm/xe/xe_dma_buf.c
> +++ b/drivers/gpu/drm/xe/xe_dma_buf.c
> @@ -258,16 +258,8 @@ struct dma_buf *xe_gem_prime_export(struct
> drm_gem_object *obj, int flags)
>  	return ERR_PTR(ret);
>  }
>  
> -/*
> - * Takes ownership of @storage: on success it is transferred to the
> returned
> - * drm_gem_object; on failure it is freed before returning the
> error.
> - * This matches the contract of xe_bo_init_locked() which frees
> @storage on
> - * its error paths, so callers need not (and must not) free @storage
> after
> - * this call.
> - */
>  static struct drm_gem_object *
> -xe_dma_buf_init_obj(struct drm_device *dev, struct xe_bo *storage,
> -		    struct dma_buf *dma_buf)
> +xe_dma_buf_create_obj(struct drm_device *dev, struct dma_buf
> *dma_buf)
>  {
>  	struct dma_resv *resv = dma_buf->resv;
>  	struct xe_device *xe = to_xe_device(dev);
> @@ -278,10 +270,8 @@ xe_dma_buf_init_obj(struct drm_device *dev,
> struct xe_bo *storage,
>  	int ret = 0;
>  
>  	dummy_obj = drm_gpuvm_resv_object_alloc(&xe->drm);
> -	if (!dummy_obj) {
> -		xe_bo_free(storage);
> +	if (!dummy_obj)
>  		return ERR_PTR(-ENOMEM);
> -	}
>  
>  	dummy_obj->resv = resv;
>  	xe_validation_guard(&ctx, &xe->val, &exec, (struct
> xe_val_flags) {}, ret) {
> @@ -290,8 +280,7 @@ xe_dma_buf_init_obj(struct drm_device *dev,
> struct xe_bo *storage,
>  		if (ret)
>  			break;
>  
> -		/* xe_bo_init_locked() frees storage on error */
> -		bo = xe_bo_init_locked(xe, storage, NULL, resv,
> NULL, dma_buf->size,
> +		bo = xe_bo_init_locked(xe, NULL, NULL, resv, NULL,
> dma_buf->size,
>  				       0, /* Will require 1way or
> 2way for vm_bind */
>  				       ttm_bo_type_sg,
> XE_BO_FLAG_SYSTEM, &exec);
>  		drm_exec_retry_on_contention(&exec);
> @@ -342,7 +331,6 @@ struct drm_gem_object *xe_gem_prime_import(struct
> drm_device *dev,
>  	const struct dma_buf_attach_ops *attach_ops;
>  	struct dma_buf_attachment *attach;
>  	struct drm_gem_object *obj;
> -	struct xe_bo *bo;
>  
>  	if (dma_buf->ops == &xe_dmabuf_ops) {
>  		obj = dma_buf->priv;
> @@ -357,22 +345,14 @@ struct drm_gem_object
> *xe_gem_prime_import(struct drm_device *dev,
>  		}
>  	}
>  
> -	bo = xe_bo_alloc();
> -	if (IS_ERR(bo))
> -		return ERR_CAST(bo);
> -
>  	/*
> -	 * xe_dma_buf_init_obj() takes ownership of the raw bo, so
> do not touch
> -	 * on fail, since it will already take care of cleanup. On
> success we
> -	 * still need to drop the ref, if something later fails.
> -	 *
> -	 * In addition this needs to happen before the attach, since
> -	 * it will create a new attachment for this, and add it to
> the list of
> -	 * attachments, at which point it is globally visible, and
> at any point
> -	 * the export side can call into on invalidate_mappings
> callback, which
> -	 * require a working object.
> +	 * This needs to happen before the attach, since it will
> create a new
> +	 * attachment for this, and add it to the list of
> attachments, at which
> +	 * point it is globally visible, and at any point the export
> side can
> +	 * call into on invalidate_mappings callback, which require
> a working
> +	 * object.
>  	 */
> -	obj = xe_dma_buf_init_obj(dev, bo, dma_buf);
> +	obj = xe_dma_buf_create_obj(dev, dma_buf);

Cant you just use xe_bo_create_novm() here?


>  	if (IS_ERR(obj))
>  		return obj;
>  
> @@ -382,7 +362,7 @@ struct drm_gem_object *xe_gem_prime_import(struct
> drm_device *dev,
>  		attach_ops = test->attach_ops;
>  #endif
>  
> -	attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
> attach_ops, &bo->ttm.base);
> +	attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
> attach_ops, obj);
>  	if (IS_ERR(attach)) {
>  		obj = ERR_CAST(attach);
>  		goto out_err;
> @@ -393,7 +373,7 @@ struct drm_gem_object *xe_gem_prime_import(struct
> drm_device *dev,
>  	return obj;
>  
>  out_err:
> -	xe_bo_put(bo);
> +	xe_bo_put(gem_to_xe_bo(obj));
>  
>  	return obj;
>  }