[PATCH] uhid: Pad short UHID_CREATE writes from compat tasks

[PATCH] uhid: Pad short UHID_CREATE writes from compat tasks

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

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);
 
 			/* 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'

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] uhid: Pad short UHID_CREATE writes from compat tasks

[David Herrmann]

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'

[PATCH v2] HID: uhid: fix leak for 64/32 UHID_CREATE

[David Herrmann]

UHID allows short writes so user-space can omit unused fields. We
automatically set them to 0 in the kernel. However, the 64/32 bit
compat-handler didn't do that in the UHID_CREATE fallback. This will
reveal random kernel heap data (of random size, even) to user-space.

Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/hid/uhid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 93b00d7..cedc6da 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -287,7 +287,7 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
 			 */
 			struct uhid_create_req_compat *compat;
 
-			compat = kmalloc(sizeof(*compat), GFP_KERNEL);
+			compat = kzalloc(sizeof(*compat), GFP_KERNEL);
 			if (!compat)
 				return -ENOMEM;
 
-- 
1.8.4.2

Re: [PATCH v2] HID: uhid: fix leak for 64/32 UHID_CREATE

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On Tue, 2013-11-26 at 13:58 +0100, David Herrmann wrote:
> UHID allows short writes so user-space can omit unused fields. We
> automatically set them to 0 in the kernel. However, the 64/32 bit
> compat-handler didn't do that in the UHID_CREATE fallback. This will
> reveal random kernel heap data (of random size, even) to user-space.
> 
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> Cc: stable@vger.kernel.org

Fixes: befde0226a59 ('HID: uhid: make creating devices work on 64/32 systems')

(that should make it clear which versions need the fix)

> ---
>  drivers/hid/uhid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 93b00d7..cedc6da 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -287,7 +287,7 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
>  			 */
>  			struct uhid_create_req_compat *compat;
>  
> -			compat = kmalloc(sizeof(*compat), GFP_KERNEL);
> +			compat = kzalloc(sizeof(*compat), GFP_KERNEL);
>  			if (!compat)
>  				return -ENOMEM;
>  

-- 
Ben Hutchings
Usenet is essentially a HUGE group of people passing notes in class.
                      - Rachel Kadel, `A Quick Guide to Newsgroup Etiquette'

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v2] HID: uhid: fix leak for 64/32 UHID_CREATE

[Jiri Kosina]

On Tue, 26 Nov 2013, Ben Hutchings wrote:

> On Tue, 2013-11-26 at 13:58 +0100, David Herrmann wrote:
> > UHID allows short writes so user-space can omit unused fields. We
> > automatically set them to 0 in the kernel. However, the 64/32 bit
> > compat-handler didn't do that in the UHID_CREATE fallback. This will
> > reveal random kernel heap data (of random size, even) to user-space.
> > 
> > Reported-by: Ben Hutchings <ben@decadent.org.uk>
> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> > Cc: stable@vger.kernel.org
> 
> Fixes: befde0226a59 ('HID: uhid: make creating devices work on 64/32 systems')
> 
> (that should make it clear which versions need the fix)

Thanks a lot, applied with the Fixes: annotation added.

-- 
Jiri Kosina
SUSE Labs