Re: [PATCH] drm/xe: Don't keep stale pointer to bo->ggtt_node

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Sat, Sep 07, 2024 at 12:03:48AM +0200, Michal Wajdeczko wrote:
> When we fail to map a BO in the GGTT, we release our GGTT node
> placeholder, but leave stale bo->ggtt_node pointer to it, which
> triggers an assert immediately followed by a crash, due to UAF:
> 
> [ ] xe 0000:00:02.0: [drm] Assertion `bo->ggtt_node->base.size == bo->size` failed!
> [ ] WARNING: CPU: 4 PID: 126 at drivers/gpu/drm/xe/xe_ggtt.c:689 xe_ggtt_remove_bo+0x1d9/0x250 [xe]
> [ ] RIP: 0010:xe_ggtt_remove_bo+0x1d9/0x250 [xe]
> [ ] Call Trace:
> [ ]  <TASK>
> [ ]  ? __warn+0x88/0x190
> [ ]  ? xe_ggtt_remove_bo+0x1d9/0x250 [xe]
> [ ]  ? report_bug+0x1c3/0x1d0
> [ ]  ? handle_bug+0x42/0x70
> [ ]  ? exc_invalid_op+0x14/0x70
> [ ]  ? asm_exc_invalid_op+0x16/0x20
> [ ]  ? xe_ggtt_remove_bo+0x1d9/0x250 [xe]
> [ ]  ? xe_ggtt_remove_bo+0x1d9/0x250 [xe]
> [ ]  xe_ttm_bo_destroy+0x11f/0x260 [xe]
> [ ]  ? ttm_bo_release+0x31c/0x350 [ttm]
> [ ]  ? __mutex_unlock_slowpath+0x35/0x270
> [ ]  __xe_bo_create_locked+0x4a0/0x550 [xe]
> [ ]  ? mark_held_locks+0x49/0x80
> [ ]  xe_bo_create_pin_map_at+0x37/0x200 [xe]
> [ ]  xe_bo_create_pin_map+0x11/0x20 [xe]
> 
> While around, for similar reason, also don't keep an error pointer
> if we fail to allocate ggtt_node placeholder.
> 

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> Fixes: 34e804220f69 ("drm/xe: Make xe_ggtt_node struct independent")
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index f3fca5565d32..2895f154654c 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -619,16 +619,19 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  	bo->ggtt_node = xe_ggtt_node_init(ggtt);
>  	if (IS_ERR(bo->ggtt_node)) {
>  		err = PTR_ERR(bo->ggtt_node);
> +		bo->ggtt_node = NULL;
>  		goto out;
>  	}
>  
>  	mutex_lock(&ggtt->lock);
>  	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node->base, bo->size,
>  					  alignment, 0, start, end, 0);
> -	if (err)
> +	if (err) {
>  		xe_ggtt_node_fini(bo->ggtt_node);
> -	else
> +		bo->ggtt_node = NULL;
> +	} else {
>  		xe_ggtt_map_bo(ggtt, bo);
> +	}
>  	mutex_unlock(&ggtt->lock);
>  
>  	if (!err && bo->flags & XE_BO_FLAG_GGTT_INVALIDATE)
> -- 
> 2.43.0
>