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

[Matthew Auld]

Hi,

On 28/08/2024 19:23, Rodrigo Vivi wrote:
> 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.

Here the memory that node was pointing to was already freed, but node is 
still pointing at that freed memory. The node is not NULL at this point 
and so looks like a valid pointer. The node_allocated() call will then 
dereference that pointer, which is a UAF, AFAICT.

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

Yeah, I did consider just dropping it altogether, but ending up just 
keeping it. I can drop it you prefer?

> 
>> 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 at intel.com>

Thanks.

> 
>>
>> Fixes: 34e804220f69 ("drm/xe: Make xe_ggtt_node struct independent")
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at 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
>>