[v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM

[PATCH v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM

Posted by Icenowy Zheng 1 week, 4 days ago

The drm gpuvm code doesn't protect find operation against map operation,
and the driver needs to ensure a map operation shouldn't happen when a
find operation is in progress.

In some cases a find operation will be in progress when doing map/unmap
operations, and the find operation will do a NULL pointer deference.

An example of the stack trace of such NULL deference is shown below:

```
Unable to handle kernel access to user memory without uaccess routines at
virtual address 0000000000000010

[<ffffffff01e989d4>] drm_gpuva_find+0x28/0x6c [drm_gpuvm]
[<ffffffff01ed3a40>] pvr_vm_unmap+0x34/0x68 [powervr]
[<ffffffff01ec69da>] pvr_ioctl_vm_unmap+0x2e/0x50 [powervr]
[<ffffffff8080ce0a>] drm_ioctl_kernel+0x8e/0xdc
[<ffffffff8080d016>] drm_ioctl+0x1be/0x3e0
[<ffffffff802bec3e>] __riscv_sys_ioctl+0xba/0xc4
[<ffffffff80d858b2>] do_trap_ecall_u+0x23e/0x3f4
[<ffffffff80d92288>] handle_exception+0x168/0x174
```

As all occurences of drm_gpuva_find*() are already guarded by
vm_ctx->lock, make pvr_vm_map() to acquire this lock to prevent
disturbing any find operation. This fixes the NULL deference problem in
drm_gpuva_find*().

Cc: stable@vger.kernel.org
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Fixes: 4bc736f890ce ("drm/imagination: vm: make use of GPUVM's drm_exec helper")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
Changes in v2:
- Dropped wrongly duplicated mutex_unlock() call and reordered it to
  before pvr_vm_bind_op_fini() (the lock is acquired after
  pvr_vm_bind_op_map_init() call). (Thanks to Brajesh)
- Added a extra Fixes pointing to the original broken code that was
  refactored by the original Fixes (but still broken). (Thanks to
  Alessio)
- Fixed some typos in commit message. (Thanks to Alessio)
- Added a stacktrace of the oops that occured on my board. (As suggested
  by Alessio)

 drivers/gpu/drm/imagination/pvr_vm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 396d349fb6ce4..ceb78694cd987 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -747,6 +747,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
 
 	pvr_gem_object_get(pvr_obj);
 
+	mutex_lock(&vm_ctx->lock);
 	err = drm_gpuvm_exec_lock(&vm_exec);
 	if (err)
 		goto err_cleanup;
@@ -756,6 +757,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
 	drm_gpuvm_exec_unlock(&vm_exec);
 
 err_cleanup:
+	mutex_unlock(&vm_ctx->lock);
 	pvr_vm_bind_op_fini(&bind_op);
 
 	return err;
-- 
2.52.0

Re: [PATCH v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM

Posted by Alessio Belle 5 days, 5 hours ago

On Tue, 14 Jul 2026 15:36:41 +0800, Icenowy Zheng wrote:
> The drm gpuvm code doesn't protect find operation against map operation,
> and the driver needs to ensure a map operation shouldn't happen when a
> find operation is in progress.
> 
> In some cases a find operation will be in progress when doing map/unmap
> operations, and the find operation will do a NULL pointer deference.
> 
> [...]

Applied to drm-misc-fixes, thanks!

[1/1] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM
      commit: 17e2030f37600994440f875dc410615d5c66ee6d

Best regards,
-- 
Alessio Belle <alessio.belle@imgtec.com>

Re: [PATCH v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM

Posted by Alessio Belle 1 week, 1 day ago

On Tue, 2026-07-14 at 15:36 +0800, Icenowy Zheng wrote:
> The drm gpuvm code doesn't protect find operation against map operation,
> and the driver needs to ensure a map operation shouldn't happen when a
> find operation is in progress.
> 
> In some cases a find operation will be in progress when doing map/unmap
> operations, and the find operation will do a NULL pointer deference.
> 
> An example of the stack trace of such NULL deference is shown below:
> 
> ```
> Unable to handle kernel access to user memory without uaccess routines at
> virtual address 0000000000000010
> 
> [<ffffffff01e989d4>] drm_gpuva_find+0x28/0x6c [drm_gpuvm]
> [<ffffffff01ed3a40>] pvr_vm_unmap+0x34/0x68 [powervr]
> [<ffffffff01ec69da>] pvr_ioctl_vm_unmap+0x2e/0x50 [powervr]
> [<ffffffff8080ce0a>] drm_ioctl_kernel+0x8e/0xdc
> [<ffffffff8080d016>] drm_ioctl+0x1be/0x3e0
> [<ffffffff802bec3e>] __riscv_sys_ioctl+0xba/0xc4
> [<ffffffff80d858b2>] do_trap_ecall_u+0x23e/0x3f4
> [<ffffffff80d92288>] handle_exception+0x168/0x174
> ```
> 
> As all occurences of drm_gpuva_find*() are already guarded by
> vm_ctx->lock, make pvr_vm_map() to acquire this lock to prevent
> disturbing any find operation. This fixes the NULL deference problem in
> drm_gpuva_find*().
> 
> Cc: stable@vger.kernel.org
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Fixes: 4bc736f890ce ("drm/imagination: vm: make use of GPUVM's drm_exec helper")
> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>

Reviewed-by: Alessio Belle <alessio.belle@imgtec.com>

p.s. there's a double typo deference -> dereference that I can fix before
applying this early next week.

Thanks,
Alessio

> ---
> Changes in v2:
> - Dropped wrongly duplicated mutex_unlock() call and reordered it to
>   before pvr_vm_bind_op_fini() (the lock is acquired after
>   pvr_vm_bind_op_map_init() call). (Thanks to Brajesh)
> - Added a extra Fixes pointing to the original broken code that was
>   refactored by the original Fixes (but still broken). (Thanks to
>   Alessio)
> - Fixed some typos in commit message. (Thanks to Alessio)
> - Added a stacktrace of the oops that occured on my board. (As suggested
>   by Alessio)
> 
>  drivers/gpu/drm/imagination/pvr_vm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 396d349fb6ce4..ceb78694cd987 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -747,6 +747,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
>  
>  	pvr_gem_object_get(pvr_obj);
>  
> +	mutex_lock(&vm_ctx->lock);
>  	err = drm_gpuvm_exec_lock(&vm_exec);
>  	if (err)
>  		goto err_cleanup;
> @@ -756,6 +757,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
>  	drm_gpuvm_exec_unlock(&vm_exec);
>  
>  err_cleanup:
> +	mutex_unlock(&vm_ctx->lock);
>  	pvr_vm_bind_op_fini(&bind_op);
>  
>  	return err;

Re: [PATCH v2] drm/imagination: acquire vm_ctx->lock before mapping memory to GPU VM

Posted by Icenowy Zheng 1 week, 1 day ago

在 2026-07-17五的 11:14 +0000，Alessio Belle写道：
> On Tue, 2026-07-14 at 15:36 +0800, Icenowy Zheng wrote:
> > The drm gpuvm code doesn't protect find operation against map
> > operation,
> > and the driver needs to ensure a map operation shouldn't happen
> > when a
> > find operation is in progress.
> > 
> > In some cases a find operation will be in progress when doing
> > map/unmap
> > operations, and the find operation will do a NULL pointer
> > deference.
> > 
> > An example of the stack trace of such NULL deference is shown
> > below:
> > 
> > ```
> > Unable to handle kernel access to user memory without uaccess
> > routines at
> > virtual address 0000000000000010
> > 
> > [<ffffffff01e989d4>] drm_gpuva_find+0x28/0x6c [drm_gpuvm]
> > [<ffffffff01ed3a40>] pvr_vm_unmap+0x34/0x68 [powervr]
> > [<ffffffff01ec69da>] pvr_ioctl_vm_unmap+0x2e/0x50 [powervr]
> > [<ffffffff8080ce0a>] drm_ioctl_kernel+0x8e/0xdc
> > [<ffffffff8080d016>] drm_ioctl+0x1be/0x3e0
> > [<ffffffff802bec3e>] __riscv_sys_ioctl+0xba/0xc4
> > [<ffffffff80d858b2>] do_trap_ecall_u+0x23e/0x3f4
> > [<ffffffff80d92288>] handle_exception+0x168/0x174
> > ```
> > 
> > As all occurences of drm_gpuva_find*() are already guarded by
> > vm_ctx->lock, make pvr_vm_map() to acquire this lock to prevent
> > disturbing any find operation. This fixes the NULL deference
> > problem in
> > drm_gpuva_find*().
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related
> > code")
> > Fixes: 4bc736f890ce ("drm/imagination: vm: make use of GPUVM's
> > drm_exec helper")
> > Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> 
> Reviewed-by: Alessio Belle <alessio.belle@imgtec.com>
> 
> p.s. there's a double typo deference -> dereference that I can fix
> before
> applying this early next week.

Oops I made the same mistake again when rephrasing the message...

Maybe I need some reinforced learning.

Thanks,
Icenowy

> 
> Thanks,
> Alessio
> 
> > ---
> > Changes in v2:
> > - Dropped wrongly duplicated mutex_unlock() call and reordered it
> > to
> >   before pvr_vm_bind_op_fini() (the lock is acquired after
> >   pvr_vm_bind_op_map_init() call). (Thanks to Brajesh)
> > - Added a extra Fixes pointing to the original broken code that was
> >   refactored by the original Fixes (but still broken). (Thanks to
> >   Alessio)
> > - Fixed some typos in commit message. (Thanks to Alessio)
> > - Added a stacktrace of the oops that occured on my board. (As
> > suggested
> >   by Alessio)
> > 
> >  drivers/gpu/drm/imagination/pvr_vm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c
> > b/drivers/gpu/drm/imagination/pvr_vm.c
> > index 396d349fb6ce4..ceb78694cd987 100644
> > --- a/drivers/gpu/drm/imagination/pvr_vm.c
> > +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> > @@ -747,6 +747,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx,
> > struct pvr_gem_object *pvr_obj,
> >  
> >  	pvr_gem_object_get(pvr_obj);
> >  
> > +	mutex_lock(&vm_ctx->lock);
> >  	err = drm_gpuvm_exec_lock(&vm_exec);
> >  	if (err)
> >  		goto err_cleanup;
> > @@ -756,6 +757,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx,
> > struct pvr_gem_object *pvr_obj,
> >  	drm_gpuvm_exec_unlock(&vm_exec);
> >  
> >  err_cleanup:
> > +	mutex_unlock(&vm_ctx->lock);
> >  	pvr_vm_bind_op_fini(&bind_op);
> >  
> >  	return err;