Re: [PATCH v2 2/2] drm/xe/queue: move xa_alloc to prevent UAF

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 9/25/2024 1:24 PM, Matthew Auld wrote:
> On 25/09/2024 11:44, Nirmoy Das wrote:
>>
>> On 9/25/2024 11:51 AM, Matthew Auld wrote:
>>> On 25/09/2024 10:33, Nirmoy Das wrote:
>>>>
>>>> On 9/25/2024 9:14 AM, Matthew Auld wrote:
>>>>> Evil user can guess the next id of the queue before the ioctl completes
>>>>> and then call queue destroy ioctl to trigger UAF since create ioctl is
>>>>> still referencing the same queue. Move the xa_alloc all the way to the end
>>>>> to prevent this.
>>>>
>>>> The commit message doesn't match the diff, xa_alloc already happening at the end here.
>>>
>>> It's not at the end. It is dereferencing the q to set xef after the xa_alloc, but that needs to happen before it is visible to userspace.
>>
>>
>> Ah I see, that make sense.
>>
>> I think now  we have to undo xe_file_get() if xe_alloc fails.
>
> That is already handled by the queue put().

Ok,  that took some digging. Thanks for clearing my doubts.

Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

>
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>
>>>>
>>>>> v2:
>>>>>    - Rebase
>>>>>
>>>>> Fixes: 2149ded63079 ("drm/xe: Fix use after free when client stats are captured")
>>>>> Signed-off-by: Matthew Auld<matthew.auld@intel.com>
>>>>> Cc: Matthew Brost<matthew.brost@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/xe/xe_exec_queue.c | 4 +++-
>>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
>>>>> index 7743ebdcbf4b..d098d2dd1b2d 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
>>>>> @@ -635,12 +635,14 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>>>>>            }
>>>>>        }
>>>>>    +    q->xef = xe_file_get(xef);
>>>>> +
>>>>> +    /* user id alloc must always be last in ioctl to prevent UAF */
>>>>>        err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
>>>>>        if (err)
>>>>>            goto kill_exec_queue;
>>>>>          args->exec_queue_id = id;
>>>>> -    q->xef = xe_file_get(xef);
>>>>>          return 0;
>>>>>