Hi Christian,
On 12/13/2024 4:13 PM, Christian König wrote:
Am 12.12.24 um 15:25 schrieb Arunpravin Paneer Selvam:
Fix out-of-bounds issue in userq fence create when
accessing the userq xa structure. Added a lock to
protect the race condition.
BUG: KASAN: slab-out-of-bounds in
amdgpu_userq_fence_create+0x726/0x880 [amdgpu]
[ +0.000006] Call Trace:
[ +0.000005] <TASK>
[ +0.000005] dump_stack_lvl+0x6c/0x90
[ +0.000011] print_report+0xc4/0x5e0
[ +0.000009] ? srso_return_thunk+0x5/0x5f
[ +0.000008] ? kasan_complete_mode_report_info+0x26/0x1d0
[ +0.000007] ? amdgpu_userq_fence_create+0x726/0x880 [amdgpu]
[ +0.000405] kasan_report+0xdf/0x120
[ +0.000009] ? amdgpu_userq_fence_create+0x726/0x880 [amdgpu]
[ +0.000405] __asan_report_store8_noabort+0x17/0x20
[ +0.000007] amdgpu_userq_fence_create+0x726/0x880 [amdgpu]
[ +0.000406] ? __pfx_amdgpu_userq_fence_create+0x10/0x10 [amdgpu]
[ +0.000408] ? srso_return_thunk+0x5/0x5f
[ +0.000008] ? ttm_resource_move_to_lru_tail+0x235/0x4f0 [ttm]
[ +0.000013] ? srso_return_thunk+0x5/0x5f
[ +0.000008] amdgpu_userq_signal_ioctl+0xd29/0x1c70 [amdgpu]
[ +0.000412] ? __pfx_amdgpu_userq_signal_ioctl+0x10/0x10 [amdgpu]
[ +0.000404] ? try_to_wake_up+0x165/0x1840
[ +0.000010] ? __pfx_futex_wake_mark+0x10/0x10
[ +0.000011] drm_ioctl_kernel+0x178/0x2f0 [drm]
[ +0.000050] ? __pfx_amdgpu_userq_signal_ioctl+0x10/0x10 [amdgpu]
[ +0.000404] ? __pfx_drm_ioctl_kernel+0x10/0x10 [drm]
[ +0.000043] ? __kasan_check_read+0x11/0x20
[ +0.000007] ? srso_return_thunk+0x5/0x5f
[ +0.000007] ? __kasan_check_write+0x14/0x20
[ +0.000008] drm_ioctl+0x513/0xd20 [drm]
[ +0.000040] ? __pfx_amdgpu_userq_signal_ioctl+0x10/0x10 [amdgpu]
[ +0.000407] ? __pfx_drm_ioctl+0x10/0x10 [drm]
[ +0.000044] ? srso_return_thunk+0x5/0x5f
[ +0.000007] ? _raw_spin_lock_irqsave+0x99/0x100
[ +0.000007] ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[ +0.000006] ? __rseq_handle_notify_resume+0x188/0xc30
[ +0.000008] ? srso_return_thunk+0x5/0x5f
[ +0.000008] ? srso_return_thunk+0x5/0x5f
[ +0.000006] ? _raw_spin_unlock_irqrestore+0x27/0x50
[ +0.000010] amdgpu_drm_ioctl+0xcd/0x1d0 [amdgpu]
[ +0.000388] __x64_sys_ioctl+0x135/0x1b0
[ +0.000009] x64_sys_call+0x1205/0x20d0
[ +0.000007] do_syscall_64+0x4d/0x120
[ +0.000008] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ +0.000007] RIP: 0033:0x7f7c3d31a94f
Signed-off-by: Arunpravin Paneer Selvam
<Arunpravin.PaneerSelvam@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index 3a88f754a395..49dc78c2f0d7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -229,6 +229,7 @@ int amdgpu_userq_fence_create(struct
amdgpu_usermode_queue *userq,
unsigned long index, count = 0;
int i = 0;
+ xa_lock(&userq->fence_drv_xa);
Don't you allocate the userq->fence_drv_xa after counting the number
of objects?
If yes then you need to drop the lock again for that.
We are allocating memory for userq_fence->fence_drv_array using the
kvmalloc_array(),
should we drop the lock for this memory allocation and again acquire the
lock for
moving the fence_drv references from userq->fence_drv_xa to
userq_fence->fence_drv_array
code block. Is this correct?
xa_for_each(&userq->fence_drv_xa, index, stored_fence_drv)
count++;
@@ -240,12 +241,13 @@ int amdgpu_userq_fence_create(struct
amdgpu_usermode_queue *userq,
if (userq_fence->fence_drv_array) {
xa_for_each(&userq->fence_drv_xa, index,
stored_fence_drv) {
userq_fence->fence_drv_array[i] = stored_fence_drv;
- xa_erase(&userq->fence_drv_xa, index);
+ __xa_erase(&userq->fence_drv_xa, index);
It's *much* more efficient to release all entries at once by calling
xa_destroy() after the loop I think.
sure, I will try with xa_destroy().
Thanks,
Arun.
Regards,
Christian.
i++;
}
}
userq_fence->fence_drv_array_count = i;
+ xa_unlock(&userq->fence_drv_xa);
} else {
userq_fence->fence_drv_array = NULL;
userq_fence->fence_drv_array_count = 0;
