KASAN: slab-out-of-bounds Read in technisat_usb2_rc_query

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: 9a33b369 usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan/tree/usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=1441219f200000
kernel config: https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15
dashboard link: https://syzkaller.appspot.com/bug?extid=eaaaf38a95427be88f4b
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=174a67bb200000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17aa2023200000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com

rc rc1: Technisat SkyStar USB HD (DVB-S/S2) as
/devices/platform/dummy_hcd.0/usb1/1-1/rc/rc1
input: Technisat SkyStar USB HD (DVB-S/S2) as
/devices/platform/dummy_hcd.0/usb1/1-1/rc/rc1/input10
rc rc1: lirc_dev: driver technisat-usb2 registered at minor = 1, raw IR
receiver, no transmitter
dvb-usb: schedule remote query interval to 100 msecs.
==================================================================
BUG: KASAN: slab-out-of-bounds in technisat_usb2_get_ir
drivers/media/usb/dvb-usb/technisat-usb2.c:664 [inline]
BUG: KASAN: slab-out-of-bounds in technisat_usb2_rc_query+0x5fa/0x660
drivers/media/usb/dvb-usb/technisat-usb2.c:679
Read of size 1 at addr ffff8880a8791ea8 by task kworker/0:1/12

CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc4-319354-g9a33b36 #3
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: events dvb_usb_read_remote_control
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xe8/0x16e lib/dump_stack.c:113
print_address_description+0x6c/0x236 mm/kasan/report.c:187
kasan_report.cold+0x1a/0x3c mm/kasan/report.c:317
technisat_usb2_get_ir drivers/media/usb/dvb-usb/technisat-usb2.c:664
[inline]
technisat_usb2_rc_query+0x5fa/0x660
drivers/media/usb/dvb-usb/technisat-usb2.c:679
dvb_usb_read_remote_control
drivers/media/usb/dvb-usb-v2/dvb_usb_core.c:128 [inline]
dvb_usb_read_remote_control+0xe5/0x1c0
drivers/media/usb/dvb-usb-v2/dvb_usb_core.c:105
process_one_work+0x90f/0x1580 kernel/workqueue.c:2269
worker_thread+0x9b/0xe20 kernel/workqueue.c:2415
kthread+0x313/0x420 kernel/kthread.c:253
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352

Allocated by task 615:
set_track mm/kasan/common.c:87 [inline]
__kasan_kmalloc mm/kasan/common.c:497 [inline]
__kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:470
dvb_usb_init drivers/media/usb/dvb-usb/dvb-usb-init.c:152 [inline]
dvb_usb_device_init.cold+0x317/0x10b3
drivers/media/usb/dvb-usb/dvb-usb-init.c:277
technisat_usb2_probe+0x82/0x2d0
drivers/media/usb/dvb-usb/technisat-usb2.c:763
usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361
really_probe+0x2da/0xb10 drivers/base/dd.c:509
driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
__device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
__device_attach+0x223/0x3a0 drivers/base/dd.c:844
bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
device_add+0xad2/0x16e0 drivers/base/core.c:2106
usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021
generic_probe+0xa2/0xda drivers/usb/core/generic.c:210
usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266
really_probe+0x2da/0xb10 drivers/base/dd.c:509
driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
__device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
__device_attach+0x223/0x3a0 drivers/base/dd.c:844
bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
device_add+0xad2/0x16e0 drivers/base/core.c:2106
usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534
hub_port_connect drivers/usb/core/hub.c:5089 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5204 [inline]
port_event drivers/usb/core/hub.c:5350 [inline]
hub_event+0x138e/0x3b00 drivers/usb/core/hub.c:5432
process_one_work+0x90f/0x1580 kernel/workqueue.c:2269
worker_thread+0x9b/0xe20 kernel/workqueue.c:2415
kthread+0x313/0x420 kernel/kthread.c:253
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352

Freed by task 1:
set_track mm/kasan/common.c:87 [inline]
__kasan_slab_free+0x130/0x180 mm/kasan/common.c:459
slab_free_hook mm/slub.c:1429 [inline]
slab_free_freelist_hook+0x5e/0x140 mm/slub.c:1456
slab_free mm/slub.c:3003 [inline]
kfree+0xce/0x290 mm/slub.c:3958
krealloc+0x7d/0xc0 mm/slab_common.c:1570
add_sysfs_param.isra.0+0xcd/0x930 kernel/params.c:633
kernel_add_sysfs_param kernel/params.c:794 [inline]
param_sysfs_builtin kernel/params.c:833 [inline]
param_sysfs_init+0x364/0x435 kernel/params.c:954
do_one_initcall+0xde/0x597 init/main.c:901
do_initcall_level init/main.c:969 [inline]
do_initcalls init/main.c:977 [inline]
do_basic_setup init/main.c:995 [inline]
kernel_init_freeable+0x4da/0x5c7 init/main.c:1150
kernel_init+0x12/0x1ca init/main.c:1068
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352

The buggy address belongs to the object at ffff8880a8791dc0
which belongs to the cache kmalloc-256 of size 256
The buggy address is located 232 bytes inside of
256-byte region [ffff8880a8791dc0, ffff8880a8791ec0)
The buggy address belongs to the page:
page:ffffea0002a1e440 count:1 mapcount:0 mapping:ffff88812c3f4e00 index:0x0
flags: 0xfff00000000200(slab)
raw: 00fff00000000200 dead000000000100 dead000000000200 ffff88812c3f4e00
raw: 0000000000000000 00000000000c000c 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff8880a8791d80: fc fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00
ffff8880a8791e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ffff8880a8791e80: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
^
ffff8880a8791f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff8880a8791f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Phong Tran]

The buffer will be overflow in case of the while loop can not break.
Add the checking buffer condition in while loop for avoiding
overlooping index.

This issue was reported by syzbot

Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com

Tested by:
https://groups.google.com/d/msg/syzkaller-bugs/CySBCKuUOOs/0hKq1CdjCwAJ

Signed-off-by: Phong Tran <tranma...@gmail.com>
---
drivers/media/usb/dvb-usb/technisat-usb2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
index c659e18b358b..4e0b6185666a 100644
--- a/drivers/media/usb/dvb-usb/technisat-usb2.c
+++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
@@ -655,7 +655,7 @@ static int technisat_usb2_get_ir(struct dvb_usb_device *d)
#endif

ev.pulse = 0;
- while (1) {
+ while (b != (buf + 63)) {
ev.pulse = !ev.pulse;
ev.duration = (*b * FIRMWARE_CLOCK_DIVISOR * FIRMWARE_CLOCK_TICK) / 1000;
ir_raw_event_store(d->rc_dev, &ev);
--
2.11.0

[Alexander Potapenko]

On Tue, Jul 2, 2019 at 4:02 PM Phong Tran <tranma...@gmail.com> wrote:
>
> The buffer will be overflow in case of the while loop can not break.
> Add the checking buffer condition in while loop for avoiding
> overlooping index.
>
> This issue was reported by syzbot
>
> Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com
>
> Tested by:
> https://groups.google.com/d/msg/syzkaller-bugs/CySBCKuUOOs/0hKq1CdjCwAJ
>
> Signed-off-by: Phong Tran <tranma...@gmail.com>
> ---
> drivers/media/usb/dvb-usb/technisat-usb2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
> index c659e18b358b..4e0b6185666a 100644
> --- a/drivers/media/usb/dvb-usb/technisat-usb2.c
> +++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
> @@ -655,7 +655,7 @@ static int technisat_usb2_get_ir(struct dvb_usb_device *d)
> #endif
>
> ev.pulse = 0;
> - while (1) {
> + while (b != (buf + 63)) {

I think it won't hurt to either use ARRAY_SIZE here, or define some
magic constant for the buffer size in struct technisat_usb2_state.

> ev.pulse = !ev.pulse;
> ev.duration = (*b * FIRMWARE_CLOCK_DIVISOR * FIRMWARE_CLOCK_TICK) / 1000;
> ir_raw_event_store(d->rc_dev, &ev);
> --
> 2.11.0
>

> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20190702140211.28399-1-tranmanphong%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.



--
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[Kees Cook]

On Tue, Jul 02, 2019 at 09:02:11PM +0700, Phong Tran wrote:
> The buffer will be overflow in case of the while loop can not break.
> Add the checking buffer condition in while loop for avoiding
> overlooping index.
>
> This issue was reported by syzbot
>
> Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com
>
> Tested by:
> https://groups.google.com/d/msg/syzkaller-bugs/CySBCKuUOOs/0hKq1CdjCwAJ
>
> Signed-off-by: Phong Tran <tranma...@gmail.com>
> ---
> drivers/media/usb/dvb-usb/technisat-usb2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
> index c659e18b358b..4e0b6185666a 100644
> --- a/drivers/media/usb/dvb-usb/technisat-usb2.c
> +++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
> @@ -655,7 +655,7 @@ static int technisat_usb2_get_ir(struct dvb_usb_device *d)
> #endif
>
> ev.pulse = 0;
> - while (1) {
> + while (b != (buf + 63)) {

This matches the "62" from the earlier read -- instead of these literal
numbers, could you replace the "62"s with a named define for whatever
would make sense for this driver (maybe "IR_MAX_EVENTS"?), and then you
can make the above be something like:

while (b <= buf + IR_MAX_EVENTS) {

> ev.pulse = !ev.pulse;
> ev.duration = (*b * FIRMWARE_CLOCK_DIVISOR * FIRMWARE_CLOCK_TICK) / 1000;
> ir_raw_event_store(d->rc_dev, &ev);
> --
> 2.11.0
>

--
Kees Cook

[Phong Tran]

The buffer will be overflow in case of the while loop can not break.
Add the checking buffer condition in while loop for avoiding
overlooping index.

This issue was reported by syzbot

Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com

Tested-by:
https://groups.google.com/d/msg/syzkaller-bugs/CySBCKuUOOs/t3PvVheSAAAJ

Signed-off-by: Phong Tran <tranma...@gmail.com>
---

Change Log:
* V2: add IR_MAX_BUFFER_INDEX and adjust the while loop condition as comments
---
drivers/media/usb/dvb-usb/technisat-usb2.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
index c659e18b358b..cdabff97c1ea 100644
--- a/drivers/media/usb/dvb-usb/technisat-usb2.c
+++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
@@ -49,6 +49,7 @@ MODULE_PARM_DESC(disable_led_control,
"disable LED control of the device (default: 0 - LED control is active).");

/* device private data */
+#define IR_MAX_BUFFER_INDEX 63
struct technisat_usb2_state {
struct dvb_usb_device *dev;
struct delayed_work green_led_work;
@@ -56,7 +57,7 @@ struct technisat_usb2_state {

u16 last_scan_code;

- u8 buf[64];
+ u8 buf[IR_MAX_BUFFER_INDEX + 1];
};

/* debug print helpers */
@@ -655,7 +656,7 @@ static int technisat_usb2_get_ir(struct dvb_usb_device *d)

#endif

ev.pulse = 0;
- while (1) {

+ while (b <= (buf + IR_MAX_BUFFER_INDEX)) {

[Kees Cook]

On Wed, Jul 03, 2019 at 09:14:44AM +0700, Phong Tran wrote:
> The buffer will be overflow in case of the while loop can not break.
> Add the checking buffer condition in while loop for avoiding
> overlooping index.
>
> This issue was reported by syzbot
>
> Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com
>
> Tested-by:
> https://groups.google.com/d/msg/syzkaller-bugs/CySBCKuUOOs/t3PvVheSAAAJ
>

Avoid these blank lines please. (More below...)

> Signed-off-by: Phong Tran <tranma...@gmail.com>
> ---
> Change Log:
> * V2: add IR_MAX_BUFFER_INDEX and adjust the while loop condition as comments
> ---
> drivers/media/usb/dvb-usb/technisat-usb2.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
> index c659e18b358b..cdabff97c1ea 100644
> --- a/drivers/media/usb/dvb-usb/technisat-usb2.c
> +++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
> @@ -49,6 +49,7 @@ MODULE_PARM_DESC(disable_led_control,
> "disable LED control of the device (default: 0 - LED control is active).");
>
> /* device private data */
> +#define IR_MAX_BUFFER_INDEX 63

How does this map to the literal "62" used before the loop you're
fixing?

Otherwise, it's looking good; thanks!

-Kees

> struct technisat_usb2_state {
> struct dvb_usb_device *dev;
> struct delayed_work green_led_work;
> @@ -56,7 +57,7 @@ struct technisat_usb2_state {
>
> u16 last_scan_code;
>
> - u8 buf[64];
> + u8 buf[IR_MAX_BUFFER_INDEX + 1];
> };
>
> /* debug print helpers */
> @@ -655,7 +656,7 @@ static int technisat_usb2_get_ir(struct dvb_usb_device *d)
> #endif
>
> ev.pulse = 0;
> - while (1) {
> + while (b <= (buf + IR_MAX_BUFFER_INDEX)) {
> ev.pulse = !ev.pulse;
> ev.duration = (*b * FIRMWARE_CLOCK_DIVISOR * FIRMWARE_CLOCK_TICK) / 1000;
> ir_raw_event_store(d->rc_dev, &ev);
> --
> 2.11.0
>

--
Kees Cook

[Sean Young]

Ensure we do not access the buffer beyond the end if no 0xff byte
is encountered.

Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com
Signed-off-by: Sean Young <se...@mess.org>
---
drivers/media/usb/dvb-usb/technisat-usb2.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c
index c659e18b358b..676d233d46d5 100644
--- a/drivers/media/usb/dvb-usb/technisat-usb2.c
+++ b/drivers/media/usb/dvb-usb/technisat-usb2.c
@@ -608,10 +608,9 @@ static int technisat_usb2_frontend_attach(struct dvb_usb_adapter *a)

static int technisat_usb2_get_ir(struct dvb_usb_device *d)

{
struct technisat_usb2_state *state = d->priv;
- u8 *buf = state->buf;
- u8 *b;
- int ret;
struct ir_raw_event ev;
+ u8 *buf = state->buf;
+ int i, ret;

buf[0] = GET_IR_DATA_VENDOR_REQUEST;
buf[1] = 0x08;
@@ -647,26 +646,25 @@ static int technisat_usb2_get_ir(struct dvb_usb_device *d)
return 0; /* no key pressed */

/* decoding */
- b = buf+1;

#if 0
deb_rc("RC: %d ", ret);
- debug_dump(b, ret, deb_rc);
+ debug_dump(buf + 1, ret, deb_rc);

#endif

ev.pulse = 0;
- while (1) {

- ev.pulse = !ev.pulse;
- ev.duration = (*b * FIRMWARE_CLOCK_DIVISOR * FIRMWARE_CLOCK_TICK) / 1000;
- ir_raw_event_store(d->rc_dev, &ev);
-
- b++;
- if (*b == 0xff) {
+ for (i = 1; i < ARRAY_SIZE(state->buf); i++) {
+ if (buf[i] == 0xff) {
ev.pulse = 0;
ev.duration = 888888*2;
ir_raw_event_store(d->rc_dev, &ev);
break;
}
+
+ ev.pulse = !ev.pulse;
+ ev.duration = (buf[i] * FIRMWARE_CLOCK_DIVISOR *
+ FIRMWARE_CLOCK_TICK) / 1000;
+ ir_raw_event_store(d->rc_dev, &ev);
}

ir_raw_event_handle(d->rc_dev);
--
2.21.0

[Kees Cook]

On Wed, Jul 03, 2019 at 03:52:39PM +0100, Sean Young wrote:
> Ensure we do not access the buffer beyond the end if no 0xff byte
> is encountered.
>
> Reported-by: syzbot+eaaaf3...@syzkaller.appspotmail.com
> Signed-off-by: Sean Young <se...@mess.org>

Both you and Phong Tran appear to be working on fixing this. At a
glance, this patch appears to be more complete in that it makes the code
flow more sane too.

Reviewed-by: Kees Cook <kees...@chromium.org>

-Kees

--
Kees Cook