Re: [PATCH] uhid: Pad short UHID_CREATE writes from compat tasks
From: David Herrmann <hidden>
Date: 2013-11-26 12:45:58
Hi On Tue, Nov 26, 2013 at 8:02 AM, Ben Hutchings [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Short event writes are normally padded with zeroes, but the compat fixup for UHID_CREATE didn't ensure this. This appears to allow an information leak. Compile-tested only. Fixes: befde0226a59 ('HID: uhid: make creating devices work on 64/32 systems') Signed-off-by: Ben Hutchings <redacted> Cc: stable@vger.kernel.org --- I have no familiarity with uhid so I haven't written a test for this. It looks like it would be possible to write a UHID_CREATE event that only covers fields up to rd_size, and the following data on the heap would be copied to the HID device metadata and be readable that way. Ben. drivers/hid/uhid.c | 3 +++ 1 file changed, 3 insertions(+)diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 5bf2fb7..579a7115 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c@@ -298,6 +298,9 @@ static int uhid_event_from_user(const char __user *buffer, size_t len, kfree(compat); return -EFAULT; } + if (len < sizeof(*compat)) + memset((char *)buffer + len, 0, + sizeof(*compat) - len);
This should be "compat", not "buffer". Anyhow, nice catch! But the better fix imho is to use kzalloc() for the "compat" object. This isn't performance-critical and we can avoid any other off-by-one bug or future conversion errors. And besides, it's far easier to read than this memset(). Thanks David
/* Shuffle the data over to proper structure */
event->type = type;
--
Ben Hutchings
Usenet is essentially a HUGE group of people passing notes in class.
- Rachel Kadel, `A Quick Guide to Newsgroup Etiquette'