| Message ID | 20160314222104.GA20569@dtor-ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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.
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;