Commit 5137b354 authored by Dmitry Torokhov's avatar Dmitry Torokhov Committed by Jiri Kosina

HID: fix out of bound access in extract() and implement()

extract() and implement() access buffer containing reports in 64-bit
chunks, but there is no guarantee that buffers are padded to 64 bit
boundary. In fact, KASAN has caught such OOB access with i2c-hid and
Synaptics touch controller.

Instead of trying to hunt all parties that allocate buffers and make
sure they are padded, let's switch extract() and implement() to byte
access. It is a bit slower, bit we are not dealing with super fast
devices here.

Also let's fix link to the HID spec while we are at it.
Signed-off-by: default avatarDmitry Torokhov <dtor@chromium.org>
Reviewed-by: default avatarBenjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: default avatarJiri Kosina <jkosina@suse.cz>
parent c2848f2e
...@@ -1075,7 +1075,7 @@ static u32 s32ton(__s32 value, unsigned n) ...@@ -1075,7 +1075,7 @@ static u32 s32ton(__s32 value, unsigned n)
* Extract/implement a data field from/to a little endian report (bit array). * Extract/implement a data field from/to a little endian report (bit array).
* *
* Code sort-of follows HID spec: * Code sort-of follows HID spec:
* http://www.usb.org/developers/devclass_docs/HID1_11.pdf * http://www.usb.org/developers/hidpage/HID1_11.pdf
* *
* While the USB HID spec allows unlimited length bit fields in "report * While the USB HID spec allows unlimited length bit fields in "report
* descriptors", most devices never use more than 16 bits. * descriptors", most devices never use more than 16 bits.
...@@ -1083,20 +1083,37 @@ static u32 s32ton(__s32 value, unsigned n) ...@@ -1083,20 +1083,37 @@ static u32 s32ton(__s32 value, unsigned n)
* Search linux-kernel and linux-usb-devel archives for "hid-core extract". * Search linux-kernel and linux-usb-devel archives for "hid-core extract".
*/ */
__u32 hid_field_extract(const struct hid_device *hid, __u8 *report, static u32 __extract(u8 *report, unsigned offset, int n)
unsigned offset, unsigned n)
{ {
u64 x; unsigned int idx = offset / 8;
unsigned int bit_nr = 0;
unsigned int bit_shift = offset % 8;
int bits_to_copy = 8 - bit_shift;
u32 value = 0;
u32 mask = n < 32 ? (1U << n) - 1 : ~0U;
while (n > 0) {
value |= ((u32)report[idx] >> bit_shift) << bit_nr;
n -= bits_to_copy;
bit_nr += bits_to_copy;
bits_to_copy = 8;
bit_shift = 0;
idx++;
}
return value & mask;
}
if (n > 32) u32 hid_field_extract(const struct hid_device *hid, u8 *report,
unsigned offset, unsigned n)
{
if (n > 32) {
hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n", hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n",
n, current->comm); n, current->comm);
n = 32;
}
report += offset >> 3; /* adjust byte index */ return __extract(report, offset, n);
offset &= 7; /* now only need bit offset into one byte */
x = get_unaligned_le64(report);
x = (x >> offset) & ((1ULL << n) - 1); /* extract bit field */
return (u32) x;
} }
EXPORT_SYMBOL_GPL(hid_field_extract); EXPORT_SYMBOL_GPL(hid_field_extract);
...@@ -1106,31 +1123,56 @@ EXPORT_SYMBOL_GPL(hid_field_extract); ...@@ -1106,31 +1123,56 @@ EXPORT_SYMBOL_GPL(hid_field_extract);
* The data mangled in the bit stream remains in little endian * The data mangled in the bit stream remains in little endian
* order the whole time. It make more sense to talk about * order the whole time. It make more sense to talk about
* endianness of register values by considering a register * endianness of register values by considering a register
* a "cached" copy of the little endiad bit stream. * a "cached" copy of the little endian bit stream.
*/ */
static void implement(const struct hid_device *hid, __u8 *report,
unsigned offset, unsigned n, __u32 value) static void __implement(u8 *report, unsigned offset, int n, u32 value)
{ {
u64 x; unsigned int idx = offset / 8;
u64 m = (1ULL << n) - 1; unsigned int size = offset + n;
unsigned int bit_shift = offset % 8;
int bits_to_set = 8 - bit_shift;
u8 bit_mask = 0xff << bit_shift;
if (n > 32) while (n - bits_to_set >= 0) {
report[idx] &= ~bit_mask;
report[idx] |= value << bit_shift;
value >>= bits_to_set;
n -= bits_to_set;
bits_to_set = 8;
bit_mask = 0xff;
bit_shift = 0;
idx++;
}
/* last nibble */
if (n) {
if (size % 8)
bit_mask &= (1U << (size % 8)) - 1;
report[idx] &= ~bit_mask;
report[idx] |= (value << bit_shift) & bit_mask;
}
}
static void implement(const struct hid_device *hid, u8 *report,
unsigned offset, unsigned n, u32 value)
{
u64 m;
if (n > 32) {
hid_warn(hid, "%s() called with n (%d) > 32! (%s)\n", hid_warn(hid, "%s() called with n (%d) > 32! (%s)\n",
__func__, n, current->comm); __func__, n, current->comm);
n = 32;
}
m = (1ULL << n) - 1;
if (value > m) if (value > m)
hid_warn(hid, "%s() called with too large value %d! (%s)\n", hid_warn(hid, "%s() called with too large value %d! (%s)\n",
__func__, value, current->comm); __func__, value, current->comm);
WARN_ON(value > m); WARN_ON(value > m);
value &= m; value &= m;
report += offset >> 3; __implement(report, offset, n, value);
offset &= 7;
x = get_unaligned_le64(report);
x &= ~(m << offset);
x |= ((u64)value) << offset;
put_unaligned_le64(x, report);
} }
/* /*
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment