* [PATCH] HID: i2c-hid: fix OOB write in i2c_hid_set_or_send_report()
@ 2016-03-14 22:21 Dmitry Torokhov
2016-03-15 7:59 ` Benjamin Tissoires
0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2016-03-14 22:21 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, Mika Westerberg, Andrew Duggan,
Alexander Potapenko, linux-input, linux-kernel
From: Dmitry Torokhov <dtor@chromium.org>
Even though hid_hw_* checks that passed in data_len is less than
HID_MAX_BUFFER_SIZE it is not enough, as i2c-hid does not necessarily
allocate buffers of HID_MAX_BUFFER_SIZE but rather checks all device
reports and select largest size. In-kernel users normally just send as much
data as report needs, so there is no problem, but hidraw users can do
whatever they please:
BUG: KASAN: slab-out-of-bounds in memcpy+0x34/0x54 at addr ffffffc07135ea80
Write of size 4101 by task syz-executor/8747
CPU: 2 PID: 8747 Comm: syz-executor Tainted: G BU 3.18.0 #37
Hardware name: Google Tegra210 Smaug Rev 1,3+ (DT)
Call trace:
[<ffffffc00020ebcc>] dump_backtrace+0x0/0x258 arch/arm64/kernel/traps.c:83
[<ffffffc00020ee40>] show_stack+0x1c/0x2c arch/arm64/kernel/traps.c:172
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffc001958114>] dump_stack+0x90/0x140 lib/dump_stack.c:50
[< inline >] print_error_description mm/kasan/report.c:97
[< inline >] kasan_report_error mm/kasan/report.c:278
[<ffffffc0004597dc>] kasan_report+0x268/0x530 mm/kasan/report.c:305
[<ffffffc0004592e8>] __asan_storeN+0x20/0x150 mm/kasan/kasan.c:718
[<ffffffc0004594e0>] memcpy+0x30/0x54 mm/kasan/kasan.c:299
[<ffffffc001306354>] __i2c_hid_command+0x2b0/0x7b4 drivers/hid/i2c-hid/i2c-hid.c:178
[< inline >] i2c_hid_set_or_send_report drivers/hid/i2c-hid/i2c-hid.c:321
[<ffffffc0013079a0>] i2c_hid_output_raw_report.isra.2+0x3d4/0x4b8 drivers/hid/i2c-hid/i2c-hid.c:589
[<ffffffc001307ad8>] i2c_hid_output_report+0x54/0x68 drivers/hid/i2c-hid/i2c-hid.c:602
[< inline >] hid_hw_output_report include/linux/hid.h:1039
[<ffffffc0012cc7a0>] hidraw_send_report+0x400/0x414 drivers/hid/hidraw.c:154
[<ffffffc0012cc7f4>] hidraw_write+0x40/0x64 drivers/hid/hidraw.c:177
[<ffffffc0004681dc>] vfs_write+0x1d4/0x3cc fs/read_write.c:534
[< inline >] SYSC_pwrite64 fs/read_write.c:627
[<ffffffc000468984>] SyS_pwrite64+0xec/0x144 fs/read_write.c:614
Object at ffffffc07135ea80, in cache kmalloc-512
Object allocated with size 268 bytes.
Let's check data length against the buffer size before attempting to copy
data over.
Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
---
This should probably go to stable as well, leaving it to Jiri/Benjamin
to decide.
drivers/hid/i2c-hid/i2c-hid.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index b921693..bb89749 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -283,17 +283,21 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
+ u16 size;
+ int args_len;
+ int index = 0;
+
+ i2c_hid_dbg(ihid, "%s\n", __func__);
+
+ if (data_len > ihid->bufsize)
+ return -EINVAL;
- /* hid_hw_* already checked that data_len < HID_MAX_BUFFER_SIZE */
- u16 size = 2 /* size */ +
+ size = 2 /* size */ +
(reportID ? 1 : 0) /* reportID */ +
data_len /* buf */;
- int args_len = (reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
+ args_len = (reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
2 /* dataRegister */ +
size /* args */;
- int index = 0;
-
- i2c_hid_dbg(ihid, "%s\n", __func__);
if (!use_data && maxOutputLength == 0)
return -ENOSYS;
--
2.7.0.rc3.207.g0ac5344
--
Dmitry
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: i2c-hid: fix OOB write in i2c_hid_set_or_send_report()
2016-03-14 22:21 [PATCH] HID: i2c-hid: fix OOB write in i2c_hid_set_or_send_report() Dmitry Torokhov
@ 2016-03-15 7:59 ` Benjamin Tissoires
2016-03-15 13:34 ` Jiri Kosina
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Tissoires @ 2016-03-15 7:59 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Mika Westerberg, Andrew Duggan, Alexander Potapenko,
linux-input, linux-kernel
On Mar 14 2016 or thereabouts, Dmitry Torokhov wrote:
> From: Dmitry Torokhov <dtor@chromium.org>
>
> Even though hid_hw_* checks that passed in data_len is less than
> HID_MAX_BUFFER_SIZE it is not enough, as i2c-hid does not necessarily
> allocate buffers of HID_MAX_BUFFER_SIZE but rather checks all device
> reports and select largest size. In-kernel users normally just send as much
> data as report needs, so there is no problem, but hidraw users can do
> whatever they please:
>
> BUG: KASAN: slab-out-of-bounds in memcpy+0x34/0x54 at addr ffffffc07135ea80
> Write of size 4101 by task syz-executor/8747
> CPU: 2 PID: 8747 Comm: syz-executor Tainted: G BU 3.18.0 #37
> Hardware name: Google Tegra210 Smaug Rev 1,3+ (DT)
> Call trace:
> [<ffffffc00020ebcc>] dump_backtrace+0x0/0x258 arch/arm64/kernel/traps.c:83
> [<ffffffc00020ee40>] show_stack+0x1c/0x2c arch/arm64/kernel/traps.c:172
> [< inline >] __dump_stack lib/dump_stack.c:15
> [<ffffffc001958114>] dump_stack+0x90/0x140 lib/dump_stack.c:50
> [< inline >] print_error_description mm/kasan/report.c:97
> [< inline >] kasan_report_error mm/kasan/report.c:278
> [<ffffffc0004597dc>] kasan_report+0x268/0x530 mm/kasan/report.c:305
> [<ffffffc0004592e8>] __asan_storeN+0x20/0x150 mm/kasan/kasan.c:718
> [<ffffffc0004594e0>] memcpy+0x30/0x54 mm/kasan/kasan.c:299
> [<ffffffc001306354>] __i2c_hid_command+0x2b0/0x7b4 drivers/hid/i2c-hid/i2c-hid.c:178
> [< inline >] i2c_hid_set_or_send_report drivers/hid/i2c-hid/i2c-hid.c:321
> [<ffffffc0013079a0>] i2c_hid_output_raw_report.isra.2+0x3d4/0x4b8 drivers/hid/i2c-hid/i2c-hid.c:589
> [<ffffffc001307ad8>] i2c_hid_output_report+0x54/0x68 drivers/hid/i2c-hid/i2c-hid.c:602
> [< inline >] hid_hw_output_report include/linux/hid.h:1039
> [<ffffffc0012cc7a0>] hidraw_send_report+0x400/0x414 drivers/hid/hidraw.c:154
> [<ffffffc0012cc7f4>] hidraw_write+0x40/0x64 drivers/hid/hidraw.c:177
> [<ffffffc0004681dc>] vfs_write+0x1d4/0x3cc fs/read_write.c:534
> [< inline >] SYSC_pwrite64 fs/read_write.c:627
> [<ffffffc000468984>] SyS_pwrite64+0xec/0x144 fs/read_write.c:614
> Object at ffffffc07135ea80, in cache kmalloc-512
> Object allocated with size 268 bytes.
>
> Let's check data length against the buffer size before attempting to copy
> data over.
>
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
> ---
>
> This should probably go to stable as well, leaving it to Jiri/Benjamin
> to decide.
Works for me:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I also think this should go to stable. Jiri, can you add the tag when
committing?
Cheers,
Benjamin
>
>
> drivers/hid/i2c-hid/i2c-hid.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index b921693..bb89749 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -283,17 +283,21 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
> u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
> u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
> + u16 size;
> + int args_len;
> + int index = 0;
> +
> + i2c_hid_dbg(ihid, "%s\n", __func__);
> +
> + if (data_len > ihid->bufsize)
> + return -EINVAL;
>
> - /* hid_hw_* already checked that data_len < HID_MAX_BUFFER_SIZE */
> - u16 size = 2 /* size */ +
> + size = 2 /* size */ +
> (reportID ? 1 : 0) /* reportID */ +
> data_len /* buf */;
> - int args_len = (reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
> + args_len = (reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
> 2 /* dataRegister */ +
> size /* args */;
> - int index = 0;
> -
> - i2c_hid_dbg(ihid, "%s\n", __func__);
>
> if (!use_data && maxOutputLength == 0)
> return -ENOSYS;
> --
> 2.7.0.rc3.207.g0ac5344
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: i2c-hid: fix OOB write in i2c_hid_set_or_send_report()
2016-03-15 7:59 ` Benjamin Tissoires
@ 2016-03-15 13:34 ` Jiri Kosina
0 siblings, 0 replies; 3+ messages in thread
From: Jiri Kosina @ 2016-03-15 13:34 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Mika Westerberg, Andrew Duggan,
Alexander Potapenko, linux-input, linux-kernel
On Tue, 15 Mar 2016, Benjamin Tissoires wrote:
> > BUG: KASAN: slab-out-of-bounds in memcpy+0x34/0x54 at addr ffffffc07135ea80
> > Write of size 4101 by task syz-executor/8747
> > CPU: 2 PID: 8747 Comm: syz-executor Tainted: G BU 3.18.0 #37
> > Hardware name: Google Tegra210 Smaug Rev 1,3+ (DT)
> > Call trace:
> > [<ffffffc00020ebcc>] dump_backtrace+0x0/0x258 arch/arm64/kernel/traps.c:83
> > [<ffffffc00020ee40>] show_stack+0x1c/0x2c arch/arm64/kernel/traps.c:172
> > [< inline >] __dump_stack lib/dump_stack.c:15
> > [<ffffffc001958114>] dump_stack+0x90/0x140 lib/dump_stack.c:50
> > [< inline >] print_error_description mm/kasan/report.c:97
> > [< inline >] kasan_report_error mm/kasan/report.c:278
> > [<ffffffc0004597dc>] kasan_report+0x268/0x530 mm/kasan/report.c:305
> > [<ffffffc0004592e8>] __asan_storeN+0x20/0x150 mm/kasan/kasan.c:718
> > [<ffffffc0004594e0>] memcpy+0x30/0x54 mm/kasan/kasan.c:299
> > [<ffffffc001306354>] __i2c_hid_command+0x2b0/0x7b4 drivers/hid/i2c-hid/i2c-hid.c:178
> > [< inline >] i2c_hid_set_or_send_report drivers/hid/i2c-hid/i2c-hid.c:321
> > [<ffffffc0013079a0>] i2c_hid_output_raw_report.isra.2+0x3d4/0x4b8 drivers/hid/i2c-hid/i2c-hid.c:589
> > [<ffffffc001307ad8>] i2c_hid_output_report+0x54/0x68 drivers/hid/i2c-hid/i2c-hid.c:602
> > [< inline >] hid_hw_output_report include/linux/hid.h:1039
> > [<ffffffc0012cc7a0>] hidraw_send_report+0x400/0x414 drivers/hid/hidraw.c:154
> > [<ffffffc0012cc7f4>] hidraw_write+0x40/0x64 drivers/hid/hidraw.c:177
> > [<ffffffc0004681dc>] vfs_write+0x1d4/0x3cc fs/read_write.c:534
> > [< inline >] SYSC_pwrite64 fs/read_write.c:627
> > [<ffffffc000468984>] SyS_pwrite64+0xec/0x144 fs/read_write.c:614
> > Object at ffffffc07135ea80, in cache kmalloc-512
> > Object allocated with size 268 bytes.
> >
> > Let's check data length against the buffer size before attempting to copy
> > data over.
> >
> > Reported-by: Alexander Potapenko <glider@google.com>
> > Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
> > ---
> >
> > This should probably go to stable as well, leaving it to Jiri/Benjamin
> > to decide.
>
> Works for me:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> I also think this should go to stable. Jiri, can you add the tag when
> committing?
Makes sense, thanks to both of you.
Added the Cc: stable and applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-15 13:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-14 22:21 [PATCH] HID: i2c-hid: fix OOB write in i2c_hid_set_or_send_report() Dmitry Torokhov
2016-03-15 7:59 ` Benjamin Tissoires
2016-03-15 13:34 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).