| Message ID | 1385449330.23855.46.camel@deadeye.wl.decadent.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi On Tue, Nov 26, 2013 at 8:02 AM, Ben Hutchings <ben@decadent.org.uk> wrote: > 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 <ben@decadent.org.uk> > 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' -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); /* Shuffle the data over to proper structure */ event->type = type;
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 <ben@decadent.org.uk> 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(+)