In uhid_event_from_user(), if it is in_compat_syscall(), the 'type' of the
event is first fetched from the 'buffer' in userspace and checked. If the
'type' is UHID_CREATE, it is a messed up request with compat pointer, which
could be more than 256 bytes, so it is better allocated from the heap, as
mentioned in the comment. Its fields are then prepared one by one instead
of using a whole copy. For all other cases, the event object is copied
directly from user space. In other words, based on the 'type', the memory
size and structure of the event object vary.
Given that the 'buffer' resides in userspace, a malicious userspace process
can race to change the 'type' between the two copies, which will cause
inconsistency issues, potentially security issues. Plus, various operations
such as uhid_dev_destroy() and uhid_dev_input() are performed based on
'type' in function uhid_char_write(). If 'type' is modified by user, there
could be some issues such as uninitialized uses.
To fix this problem, we need to recheck the type after the second fetch to
make sure it is not UHID_CREATE.
Signed-off-by: Wenwen Wang <wang6...@umn.edu>
---
drivers/hid/uhid.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 3c55073..0220385 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -447,11 +447,17 @@ static int uhid_event_from_user(const char __user
*buffer, size_t len,
event->u.create.country = compat->country;
kfree(compat);
- return 0;
+ } else {
+ if (copy_from_user(event, buffer,
+ min(len, sizeof(*event))))
+ return -EFAULT;
+ if (event->type == UHID_CREATE)
+ return -EINVAL;
}
- /* All others can be copied directly */
+ return 0;
}
+ /* Others can be copied directly */
if (copy_from_user(event, buffer, min(len, sizeof(*event))))
return -EFAULT;
--
2.7.4
