Re: WTF is HIDIOCGRDESC supposed to do (aside of being a roothole)?

[Jiri Kosina]

On Mon, 15 Oct 2007, Al Viro wrote:

> This
> +   if (get_user(len, (int __user *)arg))
> +   return -EFAULT;
> +   if (copy_to_user(*((__u8 **)(user_arg +
> +   sizeof(__u32))),
> +   dev->hid->rdesc, len))
> is an instant trouble 

Ouch. My bad, I mis-merged from the wrong version of the branch when 
preparing the branch to merge.

The patch below is needed. Thanks a lot for catching this. I will take 
this through my tree in the next round of updates if Linus doesn't pick it 
up.



From: Jiri Kosina <[EMAIL PROTECTED]>

HID: fix HIDIOCGRDESC memory access in hidraw

Fix bogus copying of data into userspace when HIDIOCGRDESC is issued. 
HID-transport layer makes sure that dev->hid->rdesc is not larger than 
HID_MAX_DESCRIPTOR_SIZE.

Signed-off-by: Jiri Kosina <[EMAIL PROTECTED]>

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 8503197..11ced6a 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -229,9 +229,15 @@ static int hidraw_ioctl(struct inode *inode, struct file 
*file, unsigned int cmd
 
if (get_user(len, (int __user *)arg))
return -EFAULT;
-   if (copy_to_user(*((__u8 **)(user_arg +
-   sizeof(__u32))),
-   dev->hid->rdesc, len))
+
+   if (len > HID_MAX_DESCRIPTOR_SIZE - 1)
+   return -EINVAL;
+
+   if (copy_to_user(user_arg + offsetof(
+   struct 
hidraw_report_descriptor,
+   value[0]),
+   dev->hid->rdesc,
+   min(dev->hid->rsize, 
len)))
return -EFAULT;
return 0;
}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 55e51f9..edb8024 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -29,13 +29,6 @@
  * Vojtech Pavlik, Simunkova 1594, Prague 8, 182 00 Czech Republic
  */
 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
 /*
  * USB HID (Human Interface Device) interface class code
  */
@@ -69,6 +62,17 @@
 #define HID_DT_REPORT  (USB_TYPE_CLASS | 0x02)
 #define HID_DT_PHYSICAL(USB_TYPE_CLASS | 0x03)
 
+#define HID_MAX_DESCRIPTOR_SIZE4096
+
+#ifdef __KERNEL__
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
 /*
  * We parse each description item into this structure. Short items data
  * values are expanded to 32-bit signed int, long items contain a pointer
@@ -311,7 +315,6 @@ struct hid_global {
  * This is the local environment. It is persistent up the next main-item.
  */
 
-#define HID_MAX_DESCRIPTOR_SIZE4096
 #define HID_MAX_USAGES 8192
 #define HID_DEFAULT_NUM_COLLECTIONS16
 
@@ -560,4 +563,5 @@ static inline int hid_ff_init(struct hid_device *hid) { 
return -1; }
 #define err_hid(format, arg...) printk(KERN_ERR "%s: " format "\n" , \
__FILE__ , ## arg)
 #endif
+#endif
 
diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index 6676cd5..0536f29 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -15,9 +15,11 @@
  * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
  */
 
+#include 
+
 struct hidraw_report_descriptor {
__u32 size;
-   __u8 *value;
+   __u8 value[HID_MAX_DESCRIPTOR_SIZE];
 };
 
 struct hidraw_devinfo {
@@ -40,8 +42,6 @@ struct hidraw_devinfo {
 /* kernel-only API declarations */
 #ifdef __KERNEL__
 
-#include 
-
 struct hidraw {
unsigned int minor;
int exist;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WTF is HIDIOCGRDESC supposed to do (aside of being a roothole)?

[Jiri Kosina]

On Mon, 15 Oct 2007, Al Viro wrote:

 This
 +   if (get_user(len, (int __user *)arg))
 +   return -EFAULT;
 +   if (copy_to_user(*((__u8 **)(user_arg +
 +   sizeof(__u32))),
 +   dev-hid-rdesc, len))
 is an instant trouble 

Ouch. My bad, I mis-merged from the wrong version of the branch when 
preparing the branch to merge.

The patch below is needed. Thanks a lot for catching this. I will take 
this through my tree in the next round of updates if Linus doesn't pick it 
up.



From: Jiri Kosina [EMAIL PROTECTED]

HID: fix HIDIOCGRDESC memory access in hidraw

Fix bogus copying of data into userspace when HIDIOCGRDESC is issued. 
HID-transport layer makes sure that dev-hid-rdesc is not larger than 
HID_MAX_DESCRIPTOR_SIZE.

Signed-off-by: Jiri Kosina [EMAIL PROTECTED]

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 8503197..11ced6a 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -229,9 +229,15 @@ static int hidraw_ioctl(struct inode *inode, struct file 
*file, unsigned int cmd
 
if (get_user(len, (int __user *)arg))
return -EFAULT;
-   if (copy_to_user(*((__u8 **)(user_arg +
-   sizeof(__u32))),
-   dev-hid-rdesc, len))
+
+   if (len  HID_MAX_DESCRIPTOR_SIZE - 1)
+   return -EINVAL;
+
+   if (copy_to_user(user_arg + offsetof(
+   struct 
hidraw_report_descriptor,
+   value[0]),
+   dev-hid-rdesc,
+   min(dev-hid-rsize, 
len)))
return -EFAULT;
return 0;
}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 55e51f9..edb8024 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -29,13 +29,6 @@
  * Vojtech Pavlik, Simunkova 1594, Prague 8, 182 00 Czech Republic
  */
 
-#include linux/types.h
-#include linux/slab.h
-#include linux/list.h
-#include linux/timer.h
-#include linux/workqueue.h
-#include linux/input.h
-
 /*
  * USB HID (Human Interface Device) interface class code
  */
@@ -69,6 +62,17 @@
 #define HID_DT_REPORT  (USB_TYPE_CLASS | 0x02)
 #define HID_DT_PHYSICAL(USB_TYPE_CLASS | 0x03)
 
+#define HID_MAX_DESCRIPTOR_SIZE4096
+
+#ifdef __KERNEL__
+
+#include linux/types.h
+#include linux/slab.h
+#include linux/list.h
+#include linux/timer.h
+#include linux/workqueue.h
+#include linux/input.h
+
 /*
  * We parse each description item into this structure. Short items data
  * values are expanded to 32-bit signed int, long items contain a pointer
@@ -311,7 +315,6 @@ struct hid_global {
  * This is the local environment. It is persistent up the next main-item.
  */
 
-#define HID_MAX_DESCRIPTOR_SIZE4096
 #define HID_MAX_USAGES 8192
 #define HID_DEFAULT_NUM_COLLECTIONS16
 
@@ -560,4 +563,5 @@ static inline int hid_ff_init(struct hid_device *hid) { 
return -1; }
 #define err_hid(format, arg...) printk(KERN_ERR %s:  format \n , \
__FILE__ , ## arg)
 #endif
+#endif
 
diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index 6676cd5..0536f29 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -15,9 +15,11 @@
  * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
  */
 
+#include linux/hid.h
+
 struct hidraw_report_descriptor {
__u32 size;
-   __u8 *value;
+   __u8 value[HID_MAX_DESCRIPTOR_SIZE];
 };
 
 struct hidraw_devinfo {
@@ -40,8 +42,6 @@ struct hidraw_devinfo {
 /* kernel-only API declarations */
 #ifdef __KERNEL__
 
-#include linux/hid.h
-
 struct hidraw {
unsigned int minor;
int exist;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

WTF is HIDIOCGRDESC supposed to do (aside of being a roothole)?

[Al Viro]

This

+   if (get_user(len, (int __user *)arg))
+   return -EFAULT;
+   if (copy_to_user(*((__u8 **)(user_arg +
+   sizeof(__u32))),
+   dev->hid->rdesc, len))

is an instant trouble - you dereference userland-supplied address and
expect it to be OK; then you take the obtained value and use it as
address to shove the data into.

Now,
a) dereference is Not Safe(tm), even if you have get_user()
succeeded just before (and it might be completely unrelated to userland
data at that address).
b) copying arbitrary amount of data?  Without any sanity checks on
len, when we'd just got it from userland?
c) just WTF is that thing supposed to do?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

WTF is HIDIOCGRDESC supposed to do (aside of being a roothole)?

[Al Viro]

This

+   if (get_user(len, (int __user *)arg))
+   return -EFAULT;
+   if (copy_to_user(*((__u8 **)(user_arg +
+   sizeof(__u32))),
+   dev-hid-rdesc, len))

is an instant trouble - you dereference userland-supplied address and
expect it to be OK; then you take the obtained value and use it as
address to shove the data into.

Now,
a) dereference is Not Safe(tm), even if you have get_user()
succeeded just before (and it might be completely unrelated to userland
data at that address).
b) copying arbitrary amount of data?  Without any sanity checks on
len, when we'd just got it from userland?
c) just WTF is that thing supposed to do?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/