Bug 188621

Summary: Function kfd_wait_on_events() does not set error code when the call to copy_from_user() fails
Product: Drivers Reporter: bianpan (bianpan2010)
Component: Video(DRI - non Intel)Assignee: drivers_video-dri
Status: RESOLVED CODE_FIX    
Severity: normal CC: oded.gabbay
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: linux-4.9-rc6 Subsystem:
Regression: No Bisected commit-id:
Attachments: A patch to fix the bug

Description bianpan 2016-11-25 10:42:36 UTC
The return value of copy_from_user() indicates the number of bytes that cannot be copied, and there may be something wrong when the value is non-zero. In function kfd_wait_on_events() defined in file drivers/gpu/drm/amd/amdkfd/kfd_events.c, variable ret takes the error code. At line 743, the value of ret must be 0, and thus it will return 0 when copy_from_user() fails. 0 indicates there is no error, which is contrary to the fact. Maybe it is better to assign "-EFAULT" to ret when the check of the return value of copy_from_user() fails at line 741. Codes related to this bug are summarised as follows.

kfd_wait_on_events @@ drivers/gpu/drm/amd/amdkfd/kfd_events.c
718 int kfd_wait_on_events(struct kfd_process *p,
719                uint32_t num_events, void __user *data,
720                bool all, uint32_t user_timeout_ms,
721                enum kfd_event_wait_result *wait_result)
722 {
        ...
726     int ret = 0;
        ...
730     mutex_lock(&p->event_mutex);
731 
732     event_waiters = alloc_event_waiters(num_events);
733     if (!event_waiters) {
734         ret = -ENOMEM;
735         goto fail;
736     }
737 
738     for (i = 0; i < num_events; i++) {
739         struct kfd_event_data event_data;
740 
741         if (copy_from_user(&event_data, &events[i],
742                 sizeof(struct kfd_event_data)))
743             goto fail;   // insert "ret = -EFAULT;" before this jump instruction?
744 
745         ret = init_event_waiter(p, &event_waiters[i],
746                 event_data.event_id, i);
747         if (ret)
748             goto fail;
749     }
        ...
796 fail:
797     if (event_waiters)
798         free_waiters(num_events, event_waiters);
799 
800     mutex_unlock(&p->event_mutex);
801 
802     *wait_result = KFD_WAIT_ERROR;
803 
804     return ret;
805 }

Thanks very much!
Comment 1 Oded Gabbay 2016-11-25 11:53:18 UTC
I'll look into it.
Thanks for the bug.
Oded
Comment 2 bianpan 2016-12-01 08:22:07 UTC
Created attachment 246531 [details]
A patch to fix the bug

I created a patch to fix the bug. Please check whether it is suitable. Thanks!
Comment 3 Oded Gabbay 2016-12-01 08:36:46 UTC
I'll take a look.
Thanks!
Oded
Comment 4 Oded Gabbay 2017-01-16 15:40:21 UTC
Hi,
Sent the patch to upstream for kernel 4.11 merge window.

Oded
Comment 5 bianpan 2017-05-11 09:33:26 UTC
Bug fixed. Thanks. I will close it.