* [PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF
@ 2024-09-23 12:57 Matthew Auld
2024-09-23 12:57 ` [PATCH 2/2] drm/xe/queue: " Matthew Auld
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Matthew Auld @ 2024-09-23 12:57 UTC (permalink / raw)
To: intel-xe; +Cc: Matthew Brost, stable
Evil user can guess the next id of the vm before the ioctl completes and
then call vm destroy ioctl to trigger UAF since create ioctl is still
referencing the same vm. Move the xa_alloc all the way to the end to
prevent this.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: <stable@vger.kernel.org> # v6.8+
---
drivers/gpu/drm/xe/xe_vm.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index a3d7cb7cfd22..f7182ef3d8e6 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1765,12 +1765,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
if (IS_ERR(vm))
return PTR_ERR(vm);
- mutex_lock(&xef->vm.lock);
- err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
- mutex_unlock(&xef->vm.lock);
- if (err)
- goto err_close_and_put;
-
if (xe->info.has_asid) {
down_write(&xe->usm.lock);
err = xa_alloc_cyclic(&xe->usm.asid_to_vm, &asid, vm,
@@ -1778,12 +1772,11 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
&xe->usm.next_asid, GFP_KERNEL);
up_write(&xe->usm.lock);
if (err < 0)
- goto err_free_id;
+ goto err_close_and_put;
vm->usm.asid = asid;
}
- args->vm_id = id;
vm->xef = xe_file_get(xef);
/* Record BO memory for VM pagetable created against client */
@@ -1796,12 +1789,17 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
args->reserved[0] = xe_bo_main_addr(vm->pt_root[0]->bo, XE_PAGE_SIZE);
#endif
- return 0;
-
-err_free_id:
+ /* user id alloc must always be last in ioctl to prevent UAF */
mutex_lock(&xef->vm.lock);
- xa_erase(&xef->vm.xa, id);
+ err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
mutex_unlock(&xef->vm.lock);
+ if (err)
+ goto err_close_and_put;
+
+ args->vm_id = id;
+
+ return 0;
+
err_close_and_put:
xe_vm_close_and_put(vm);
--
2.46.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/xe/queue: move xa_alloc to prevent UAF
2024-09-23 12:57 [PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF Matthew Auld
@ 2024-09-23 12:57 ` Matthew Auld
2024-09-23 16:05 ` Matthew Brost
2024-09-23 16:02 ` [PATCH 1/2] drm/xe/vm: " Matthew Brost
2024-09-25 20:09 ` ✗ CI.Patch_applied: failure for series starting with [1/2] " Patchwork
2 siblings, 1 reply; 7+ messages in thread
From: Matthew Auld @ 2024-09-23 12:57 UTC (permalink / raw)
To: intel-xe; +Cc: Matthew Brost
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.
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 7f28b7fc68d5..a1d4b9b0726e 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue.c
+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
@@ -635,6 +635,9 @@ 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 */
mutex_lock(&xef->exec_queue.lock);
err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
mutex_unlock(&xef->exec_queue.lock);
@@ -642,7 +645,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
goto kill_exec_queue;
args->exec_queue_id = id;
- q->xef = xe_file_get(xef);
return 0;
--
2.46.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF
2024-09-23 12:57 [PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF Matthew Auld
2024-09-23 12:57 ` [PATCH 2/2] drm/xe/queue: " Matthew Auld
@ 2024-09-23 16:02 ` Matthew Brost
2024-09-25 20:09 ` ✗ CI.Patch_applied: failure for series starting with [1/2] " Patchwork
2 siblings, 0 replies; 7+ messages in thread
From: Matthew Brost @ 2024-09-23 16:02 UTC (permalink / raw)
To: Matthew Auld; +Cc: intel-xe, stable
On Mon, Sep 23, 2024 at 01:57:34PM +0100, Matthew Auld wrote:
> Evil user can guess the next id of the vm before the ioctl completes and
> then call vm destroy ioctl to trigger UAF since create ioctl is still
> referencing the same vm. Move the xa_alloc all the way to the end to
> prevent this.
>
I think this is correct. Shall I merge this series [1] first and then
you can rebase?
Matt
[1] https://patchwork.freedesktop.org/patch/615303/?series=138938&rev=1
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <stable@vger.kernel.org> # v6.8+
> ---
> drivers/gpu/drm/xe/xe_vm.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index a3d7cb7cfd22..f7182ef3d8e6 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1765,12 +1765,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> if (IS_ERR(vm))
> return PTR_ERR(vm);
>
> - mutex_lock(&xef->vm.lock);
> - err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> - mutex_unlock(&xef->vm.lock);
> - if (err)
> - goto err_close_and_put;
> -
> if (xe->info.has_asid) {
> down_write(&xe->usm.lock);
> err = xa_alloc_cyclic(&xe->usm.asid_to_vm, &asid, vm,
> @@ -1778,12 +1772,11 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> &xe->usm.next_asid, GFP_KERNEL);
> up_write(&xe->usm.lock);
> if (err < 0)
> - goto err_free_id;
> + goto err_close_and_put;
>
> vm->usm.asid = asid;
> }
>
> - args->vm_id = id;
> vm->xef = xe_file_get(xef);
>
> /* Record BO memory for VM pagetable created against client */
> @@ -1796,12 +1789,17 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> args->reserved[0] = xe_bo_main_addr(vm->pt_root[0]->bo, XE_PAGE_SIZE);
> #endif
>
> - return 0;
> -
> -err_free_id:
> + /* user id alloc must always be last in ioctl to prevent UAF */
> mutex_lock(&xef->vm.lock);
> - xa_erase(&xef->vm.xa, id);
> + err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> mutex_unlock(&xef->vm.lock);
> + if (err)
> + goto err_close_and_put;
> +
> + args->vm_id = id;
> +
> + return 0;
> +
> err_close_and_put:
> xe_vm_close_and_put(vm);
>
> --
> 2.46.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/xe/queue: move xa_alloc to prevent UAF
2024-09-23 12:57 ` [PATCH 2/2] drm/xe/queue: " Matthew Auld
@ 2024-09-23 16:05 ` Matthew Brost
2024-09-23 16:09 ` Matthew Auld
0 siblings, 1 reply; 7+ messages in thread
From: Matthew Brost @ 2024-09-23 16:05 UTC (permalink / raw)
To: Matthew Auld; +Cc: intel-xe
On Mon, Sep 23, 2024 at 01:57:35PM +0100, 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.
>
> 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 7f28b7fc68d5..a1d4b9b0726e 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -635,6 +635,9 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> }
> }
>
> + q->xef = xe_file_get(xef);
Same comment as last patch, this looks coreect shall I merge my series
and then you rebase?
Matt
> +
> + /* user id alloc must always be last in ioctl to prevent UAF */
> mutex_lock(&xef->exec_queue.lock);
> err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
> mutex_unlock(&xef->exec_queue.lock);
> @@ -642,7 +645,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> goto kill_exec_queue;
>
> args->exec_queue_id = id;
> - q->xef = xe_file_get(xef);
>
> return 0;
>
> --
> 2.46.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/xe/queue: move xa_alloc to prevent UAF
2024-09-23 16:05 ` Matthew Brost
@ 2024-09-23 16:09 ` Matthew Auld
2024-09-24 16:05 ` Matthew Brost
0 siblings, 1 reply; 7+ messages in thread
From: Matthew Auld @ 2024-09-23 16:09 UTC (permalink / raw)
To: Matthew Brost; +Cc: intel-xe
On 23/09/2024 17:05, Matthew Brost wrote:
> On Mon, Sep 23, 2024 at 01:57:35PM +0100, 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.
>>
>> 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 7f28b7fc68d5..a1d4b9b0726e 100644
>> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
>> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
>> @@ -635,6 +635,9 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>> }
>> }
>>
>> + q->xef = xe_file_get(xef);
>
> Same comment as last patch, this looks coreect shall I merge my series
> and then you rebase?
Yeah, sounds fine.
>
> Matt
>
>> +
>> + /* user id alloc must always be last in ioctl to prevent UAF */
>> mutex_lock(&xef->exec_queue.lock);
>> err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
>> mutex_unlock(&xef->exec_queue.lock);
>> @@ -642,7 +645,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>> goto kill_exec_queue;
>>
>> args->exec_queue_id = id;
>> - q->xef = xe_file_get(xef);
>>
>> return 0;
>>
>> --
>> 2.46.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/xe/queue: move xa_alloc to prevent UAF
2024-09-23 16:09 ` Matthew Auld
@ 2024-09-24 16:05 ` Matthew Brost
0 siblings, 0 replies; 7+ messages in thread
From: Matthew Brost @ 2024-09-24 16:05 UTC (permalink / raw)
To: Matthew Auld; +Cc: intel-xe
On Mon, Sep 23, 2024 at 05:09:01PM +0100, Matthew Auld wrote:
> On 23/09/2024 17:05, Matthew Brost wrote:
> > On Mon, Sep 23, 2024 at 01:57:35PM +0100, 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.
> > >
> > > 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 7f28b7fc68d5..a1d4b9b0726e 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > @@ -635,6 +635,9 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> > > }
> > > }
> > > + q->xef = xe_file_get(xef);
> >
> > Same comment as last patch, this looks coreect shall I merge my series
> > and then you rebase?
>
> Yeah, sounds fine.
>
My series is merged, feel free to rebase at your convenience.
Matt
> >
> > Matt
> >
> > > +
> > > + /* user id alloc must always be last in ioctl to prevent UAF */
> > > mutex_lock(&xef->exec_queue.lock);
> > > err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
> > > mutex_unlock(&xef->exec_queue.lock);
> > > @@ -642,7 +645,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> > > goto kill_exec_queue;
> > > args->exec_queue_id = id;
> > > - q->xef = xe_file_get(xef);
> > > return 0;
> > > --
> > > 2.46.1
> > >
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✗ CI.Patch_applied: failure for series starting with [1/2] drm/xe/vm: move xa_alloc to prevent UAF
2024-09-23 12:57 [PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF Matthew Auld
2024-09-23 12:57 ` [PATCH 2/2] drm/xe/queue: " Matthew Auld
2024-09-23 16:02 ` [PATCH 1/2] drm/xe/vm: " Matthew Brost
@ 2024-09-25 20:09 ` Patchwork
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2024-09-25 20:09 UTC (permalink / raw)
To: Matthew Auld; +Cc: intel-xe
== Series Details ==
Series: series starting with [1/2] drm/xe/vm: move xa_alloc to prevent UAF
URL : https://patchwork.freedesktop.org/series/138995/
State : failure
== Summary ==
=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 88d592f72f7b drm-tip: 2024y-09m-25d-19h-23m-21s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_vm.c:1765
error: drivers/gpu/drm/xe/xe_vm.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: drm/xe/vm: move xa_alloc to prevent UAF
Patch failed at 0001 drm/xe/vm: move xa_alloc to prevent UAF
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-25 20:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-23 12:57 [PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF Matthew Auld
2024-09-23 12:57 ` [PATCH 2/2] drm/xe/queue: " Matthew Auld
2024-09-23 16:05 ` Matthew Brost
2024-09-23 16:09 ` Matthew Auld
2024-09-24 16:05 ` Matthew Brost
2024-09-23 16:02 ` [PATCH 1/2] drm/xe/vm: " Matthew Brost
2024-09-25 20:09 ` ✗ CI.Patch_applied: failure for series starting with [1/2] " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox