Re: [PATCH v2] drm/amdkfd: Fix an illegal memory access

[Felix Kuehling <felix.kuehling@amd.com>]

Am 2023-02-21 um 22:05 schrieb qu.huang@linux.dev:
> In the kfd_wait_on_events() function, the kfd_event_waiter structure is
> allocated by alloc_event_waiters(), but the event field of the waiter
> structure is not initialized; When copy_from_user() fails in the
> kfd_wait_on_events() function, it will enter exception handling to
> release the previously allocated memory of the waiter structure;
> Due to the event field of the waiters structure being accessed
> in the free_waiters() function, this results in illegal memory access
> and system crash, here is the crash log:
>
> localhost kernel: RIP: 0010:native_queued_spin_lock_slowpath+0x185/0x1e0
> localhost kernel: RSP: 0018:ffffaa53c362bd60 EFLAGS: 00010082
> localhost kernel: RAX: ff3d3d6bff4007cb RBX: 0000000000000282 RCX: 00000000002c0000
> localhost kernel: RDX: ffff9e855eeacb80 RSI: 000000000000279c RDI: ffffe7088f6a21d0
> localhost kernel: RBP: ffffe7088f6a21d0 R08: 00000000002c0000 R09: ffffaa53c362be64
> localhost kernel: R10: ffffaa53c362bbd8 R11: 0000000000000001 R12: 0000000000000002
> localhost kernel: R13: ffff9e7ead15d600 R14: 0000000000000000 R15: ffff9e7ead15d698
> localhost kernel: FS:  0000152a3d111700(0000) GS:ffff9e855ee80000(0000) knlGS:0000000000000000
> localhost kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> localhost kernel: CR2: 0000152938000010 CR3: 000000044d7a4000 CR4: 00000000003506e0
> localhost kernel: Call Trace:
> localhost kernel: _raw_spin_lock_irqsave+0x30/0x40
> localhost kernel: remove_wait_queue+0x12/0x50
> localhost kernel: kfd_wait_on_events+0x1b6/0x490 [hydcu]
> localhost kernel: ? ftrace_graph_caller+0xa0/0xa0
> localhost kernel: kfd_ioctl+0x38c/0x4a0 [hydcu]
> localhost kernel: ? kfd_ioctl_set_trap_handler+0x70/0x70 [hydcu]
> localhost kernel: ? kfd_ioctl_create_queue+0x5a0/0x5a0 [hydcu]
> localhost kernel: ? ftrace_graph_caller+0xa0/0xa0
> localhost kernel: __x64_sys_ioctl+0x8e/0xd0
> localhost kernel: ? syscall_trace_enter.isra.18+0x143/0x1b0
> localhost kernel: do_syscall_64+0x33/0x80
> localhost kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9
> localhost kernel: RIP: 0033:0x152a4dff68d7
>
> Changes since v1:
>    * Allocate the waiter structure using kzalloc, removing the initialization of activated;
>    * '(event_waiters) &&' in the 'for' loop has also been removed.
>
> Signed-off-by: Qu Huang <qu.huang@linux.dev>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 729d26d..bb54f6c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -780,13 +780,12 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>
>   	event_waiters = kmalloc_array(num_events,
>   					sizeof(struct kfd_event_waiter),
> -					GFP_KERNEL);
> +					GFP_KERNEL | __GFP_ZERO);

This is basically the same as kcalloc. Why not just use that? No need to 
send another patch. I'll fix it up on my end and apply the patch to 
amd-staging-drm-next.

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

Thanks,
   Felix


>   	if (!event_waiters)
>   		return NULL;
>
> -	for (i = 0; (event_waiters) && (i < num_events) ; i++) {
> +	for (i = 0; i < num_events; i++) {
>   		init_wait(&event_waiters[i].wait);
> -		event_waiters[i].activated = false;
>   	}
>
>   	return event_waiters;
> --
> 1.8.3.1