Re: [PATCH] drm/xe: prevent potential UAF in pf_provision_vf_ggtt()

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

On Wed, Aug 28, 2024 at 11:43:42AM +0100, Matthew Auld wrote:
> The node ptr can point to an already freed ptr, if we hit the path with
> an already allocated node. We later dereference that pointer with:
> 
> 	xe_gt_assert(gt, !xe_ggtt_node_allocated(node));
> 
> which is a potential UAF. 

Not true because xe_ggtt_node_allocated is checking for that.

But probably after this patch we could remove the check there?!

> Fix this by not stashing the ptr for node.
> Also since it is likely a bad idea to leave config->ggtt_region pointing
> to a stale ptr, also set that to NULL by calling
> pf_release_vf_config_ggtt() instead of pf_release_ggtt().

This is a very good idea.

I wonder if this should be a separate patch, or another commit message,
but the end result is cleaner code, so no reason to block:

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

> 
> Fixes: 34e804220f69 ("drm/xe: Make xe_ggtt_node struct independent")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> index 41ed07b153b5..be198a426cdc 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -390,7 +390,7 @@ static void pf_release_vf_config_ggtt(struct xe_gt *gt, struct xe_gt_sriov_confi
>  static int pf_provision_vf_ggtt(struct xe_gt *gt, unsigned int vfid, u64 size)
>  {
>  	struct xe_gt_sriov_config *config = pf_pick_vf_config(gt, vfid);
> -	struct xe_ggtt_node *node = config->ggtt_region;
> +	struct xe_ggtt_node *node;
>  	struct xe_tile *tile = gt_to_tile(gt);
>  	struct xe_ggtt *ggtt = tile->mem.ggtt;
>  	u64 alignment = pf_get_ggtt_alignment(gt);
> @@ -402,14 +402,14 @@ static int pf_provision_vf_ggtt(struct xe_gt *gt, unsigned int vfid, u64 size)
>  
>  	size = round_up(size, alignment);
>  
> -	if (xe_ggtt_node_allocated(node)) {
> +	if (xe_ggtt_node_allocated(config->ggtt_region)) {
>  		err = pf_distribute_config_ggtt(tile, vfid, 0, 0);
>  		if (unlikely(err))
>  			return err;
>  
> -		pf_release_ggtt(tile, node);
> +		pf_release_vf_config_ggtt(gt, config);
>  	}
> -	xe_gt_assert(gt, !xe_ggtt_node_allocated(node));
> +	xe_gt_assert(gt, !xe_ggtt_node_allocated(config->ggtt_region));
>  
>  	if (!size)
>  		return 0;
> -- 
> 2.46.0
>