* KASAN: use-after-free Read in appledisplay_bl_get_brightness
@ 2019-09-23 14:31 syzbot
2019-09-23 14:53 ` Alexander Theißen
2019-11-05 23:36 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
0 siblings, 2 replies; 9+ messages in thread
From: syzbot @ 2019-09-23 14:31 UTC (permalink / raw)
To: 2pi, alex.theissen, andreyknvl, gregkh, linux-kernel, linux-usb,
syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: e0bd8d79 usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=13714ad9600000
kernel config: https://syzkaller.appspot.com/x/.config?x=8847e5384a16f66a
dashboard link: https://syzkaller.appspot.com/bug?extid=495dab1f175edc9c2f13
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1653d945600000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15176945600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in appledisplay_bl_get_brightness+0x1ac/0x1c0
drivers/usb/misc/appledisplay.c:167
Read of size 8 at addr ffff8881cfc576a0 by task kworker/1:0/17
CPU: 1 PID: 17 Comm: kworker/1:0 Not tainted 5.3.0+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: events appledisplay_work
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xca/0x13e lib/dump_stack.c:113
print_address_description+0x6a/0x32c mm/kasan/report.c:351
__kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
kasan_report+0xe/0x12 mm/kasan/common.c:618
appledisplay_bl_get_brightness+0x1ac/0x1c0
drivers/usb/misc/appledisplay.c:167
appledisplay_work+0x36/0x160 drivers/usb/misc/appledisplay.c:187
process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
worker_thread+0x96/0xe20 kernel/workqueue.c:2415
kthread+0x318/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Allocated by task 83:
save_stack+0x1b/0x80 mm/kasan/common.c:69
set_track mm/kasan/common.c:77 [inline]
__kasan_kmalloc mm/kasan/common.c:493 [inline]
__kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:466
kmalloc include/linux/slab.h:552 [inline]
kzalloc include/linux/slab.h:748 [inline]
appledisplay_probe+0x15a/0xb37 drivers/usb/misc/appledisplay.c:218
usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
really_probe+0x281/0x6d0 drivers/base/dd.c:548
driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
__device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
__device_attach+0x217/0x360 drivers/base/dd.c:894
bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
device_add+0xae6/0x16f0 drivers/base/core.c:2201
usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
really_probe+0x281/0x6d0 drivers/base/dd.c:548
driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
__device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
__device_attach+0x217/0x360 drivers/base/dd.c:894
bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
device_add+0xae6/0x16f0 drivers/base/core.c:2201
usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
hub_port_connect drivers/usb/core/hub.c:5098 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
port_event drivers/usb/core/hub.c:5359 [inline]
hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
worker_thread+0x96/0xe20 kernel/workqueue.c:2415
kthread+0x318/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Freed by task 83:
save_stack+0x1b/0x80 mm/kasan/common.c:69
set_track mm/kasan/common.c:77 [inline]
__kasan_slab_free+0x130/0x180 mm/kasan/common.c:455
slab_free_hook mm/slub.c:1423 [inline]
slab_free_freelist_hook mm/slub.c:1474 [inline]
slab_free mm/slub.c:3016 [inline]
kfree+0xe4/0x2f0 mm/slub.c:3957
appledisplay_probe+0x772/0xb37 drivers/usb/misc/appledisplay.c:312
usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
really_probe+0x281/0x6d0 drivers/base/dd.c:548
driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
__device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
__device_attach+0x217/0x360 drivers/base/dd.c:894
bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
device_add+0xae6/0x16f0 drivers/base/core.c:2201
usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
really_probe+0x281/0x6d0 drivers/base/dd.c:548
driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
__device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
__device_attach+0x217/0x360 drivers/base/dd.c:894
bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
device_add+0xae6/0x16f0 drivers/base/core.c:2201
usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
hub_port_connect drivers/usb/core/hub.c:5098 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
port_event drivers/usb/core/hub.c:5359 [inline]
hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
worker_thread+0x96/0xe20 kernel/workqueue.c:2415
kthread+0x318/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
The buggy address belongs to the object at ffff8881cfc57680
which belongs to the cache kmalloc-512 of size 512
The buggy address is located 32 bytes inside of
512-byte region [ffff8881cfc57680, ffff8881cfc57880)
The buggy address belongs to the page:
page:ffffea00073f1580 refcount:1 mapcount:0 mapping:ffff8881da002500
index:0x0 compound_mapcount: 0
flags: 0x200000000010200(slab|head)
raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881da002500
raw: 0000000000000000 00000000000c000c 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff8881cfc57580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8881cfc57600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff8881cfc57680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8881cfc57700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8881cfc57780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
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 syzkaller@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: KASAN: use-after-free Read in appledisplay_bl_get_brightness
2019-09-23 14:31 KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
@ 2019-09-23 14:53 ` Alexander Theißen
2019-11-05 23:36 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
1 sibling, 0 replies; 9+ messages in thread
From: Alexander Theißen @ 2019-09-23 14:53 UTC (permalink / raw)
To: syzbot; +Cc: 2pi, andreyknvl, gregkh, linux-usb, syzkaller-bugs
Hello,
just by looking at appledisplay_probe() I would say that the problem here is that usb_submit_urb() is called while the probe function can still fail. If it fails after submitting the urb the work item could already been scheduled from appledisplay_complete() and therefore operates on freed data.
Regards,
Alexander
> On 23. Sep 2019, at 16:31, syzbot <syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e0bd8d79 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=13714ad9600000
> kernel config: https://syzkaller.appspot.com/x/.config?x=8847e5384a16f66a
> dashboard link: https://syzkaller.appspot.com/bug?extid=495dab1f175edc9c2f13
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1653d945600000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15176945600000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: use-after-free in appledisplay_bl_get_brightness+0x1ac/0x1c0 drivers/usb/misc/appledisplay.c:167
> Read of size 8 at addr ffff8881cfc576a0 by task kworker/1:0/17
>
> CPU: 1 PID: 17 Comm: kworker/1:0 Not tainted 5.3.0+ #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: events appledisplay_work
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> print_address_description+0x6a/0x32c mm/kasan/report.c:351
> __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
> kasan_report+0xe/0x12 mm/kasan/common.c:618
> appledisplay_bl_get_brightness+0x1ac/0x1c0 drivers/usb/misc/appledisplay.c:167
> appledisplay_work+0x36/0x160 drivers/usb/misc/appledisplay.c:187
> process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> kthread+0x318/0x420 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
>
> Allocated by task 83:
> save_stack+0x1b/0x80 mm/kasan/common.c:69
> set_track mm/kasan/common.c:77 [inline]
> __kasan_kmalloc mm/kasan/common.c:493 [inline]
> __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:466
> kmalloc include/linux/slab.h:552 [inline]
> kzalloc include/linux/slab.h:748 [inline]
> appledisplay_probe+0x15a/0xb37 drivers/usb/misc/appledisplay.c:218
> usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
> really_probe+0x281/0x6d0 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
> bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
> __device_attach+0x217/0x360 drivers/base/dd.c:894
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
> device_add+0xae6/0x16f0 drivers/base/core.c:2201
> usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
> generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
> usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
> really_probe+0x281/0x6d0 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
> bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
> __device_attach+0x217/0x360 drivers/base/dd.c:894
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
> device_add+0xae6/0x16f0 drivers/base/core.c:2201
> usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
> hub_port_connect drivers/usb/core/hub.c:5098 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
> port_event drivers/usb/core/hub.c:5359 [inline]
> hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
> process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> kthread+0x318/0x420 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
>
> Freed by task 83:
> save_stack+0x1b/0x80 mm/kasan/common.c:69
> set_track mm/kasan/common.c:77 [inline]
> __kasan_slab_free+0x130/0x180 mm/kasan/common.c:455
> slab_free_hook mm/slub.c:1423 [inline]
> slab_free_freelist_hook mm/slub.c:1474 [inline]
> slab_free mm/slub.c:3016 [inline]
> kfree+0xe4/0x2f0 mm/slub.c:3957
> appledisplay_probe+0x772/0xb37 drivers/usb/misc/appledisplay.c:312
> usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
> really_probe+0x281/0x6d0 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
> bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
> __device_attach+0x217/0x360 drivers/base/dd.c:894
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
> device_add+0xae6/0x16f0 drivers/base/core.c:2201
> usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
> generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
> usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
> really_probe+0x281/0x6d0 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:721
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:828
> bus_for_each_drv+0x162/0x1e0 drivers/base/bus.c:430
> __device_attach+0x217/0x360 drivers/base/dd.c:894
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:490
> device_add+0xae6/0x16f0 drivers/base/core.c:2201
> usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
> hub_port_connect drivers/usb/core/hub.c:5098 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
> port_event drivers/usb/core/hub.c:5359 [inline]
> hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
> process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> kthread+0x318/0x420 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
>
> The buggy address belongs to the object at ffff8881cfc57680
> which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 32 bytes inside of
> 512-byte region [ffff8881cfc57680, ffff8881cfc57880)
> The buggy address belongs to the page:
> page:ffffea00073f1580 refcount:1 mapcount:0 mapping:ffff8881da002500 index:0x0 compound_mapcount: 0
> flags: 0x200000000010200(slab|head)
> raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881da002500
> raw: 0000000000000000 00000000000c000c 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8881cfc57580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8881cfc57600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> ffff8881cfc57680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff8881cfc57700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8881cfc57780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
>
> ---
> 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 syzkaller@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
2019-09-23 14:31 KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
2019-09-23 14:53 ` Alexander Theißen
@ 2019-11-05 23:36 ` Phong Tran
2019-11-06 11:42 ` Oliver Neukum
2019-11-06 12:10 ` Andrey Konovalov
1 sibling, 2 replies; 9+ messages in thread
From: Phong Tran @ 2019-11-05 23:36 UTC (permalink / raw)
To: syzbot+495dab1f175edc9c2f13
Cc: 2pi, alex.theissen, andreyknvl, gregkh, linux-kernel, linux-usb,
syzkaller-bugs, linux-kernel-mentees, Phong Tran
In context of USB disconnect, the delaywork trigger and calling
appledisplay_bl_get_brightness() and the msgdata was freed.
add the checking return value of usb_control_msg() and only update the
data while the retval is valid.
Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
Reported-and-tested-by:
syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
https://groups.google.com/d/msg/syzkaller-bugs/dRmkh2UYusY/l2a6Mg3FAQAJ
Signed-off-by: Phong Tran <tranmanphong@gmail.com>
---
drivers/usb/misc/appledisplay.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index ac92725458b5..3e3dfa5a3954 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -164,7 +164,8 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
0,
pdata->msgdata, 2,
ACD_USB_TIMEOUT);
- brightness = pdata->msgdata[1];
+ if (retval >= 0)
+ brightness = pdata->msgdata[1];
mutex_unlock(&pdata->sysfslock);
if (retval < 0)
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
2019-11-05 23:36 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
@ 2019-11-06 11:42 ` Oliver Neukum
2019-11-06 12:26 ` KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
2019-11-06 14:22 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
2019-11-06 12:10 ` Andrey Konovalov
1 sibling, 2 replies; 9+ messages in thread
From: Oliver Neukum @ 2019-11-06 11:42 UTC (permalink / raw)
To: Phong Tran, syzbot+495dab1f175edc9c2f13
Cc: 2pi, alex.theissen, andreyknvl, gregkh, linux-kernel, linux-usb,
syzkaller-bugs, linux-kernel-mentees
Am Mittwoch, den 06.11.2019, 06:36 +0700 schrieb Phong Tran:
> In context of USB disconnect, the delaywork trigger and calling
> appledisplay_bl_get_brightness() and the msgdata was freed.
>
> add the checking return value of usb_control_msg() and only update the
> data while the retval is valid.
Hi,
I am afraid there are some issues with your patch. First, let me stress
that you found the right place to fix an issue and you partially fixed
an issue. But the the fix you applied is incomplete and left another
issue open.
Your patch still allows doing IO to a device that may already be bound
to another driver. That is bad, especially as the buffer is already
free. Yes, if IO is failing, you have fixed that narrow issue.
But it need not fail.
If you look into appledisplay_probe() you will see that it can fail
because backlight_device_register() fails. The error handling will
thereupon kill the URB and free memory. But it will not kill an already
scheduled work. The scheduled work will then call usb_control_msg()
on pdata->msgdata, which has been freed. If that IO fails, all is well.
If not, the issue still exists.
Secondly, your error check is off by 2. You are checking only for
usb_control_msg() failing. But it can return only one byte instead
of two. If that happens, the value you return is stale, although
the buffer is correctly allocated.
Regards
Oliver
The correct fix for both issues would be:
#syz test: https://github.com/google/kasan.git e0bd8d79
From 2497a62bdbeb9bd94f690c22d96dd1b8cf22861f Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 6 Nov 2019 12:36:28 +0100
Subject: [PATCH] appledisplay: fix error handling in the scheduled work
The work item can operate on
1. stale memory left over from the last transfer
the actual length of the data transfered needs to be checked
2. memory already freed
the error handling in appledisplay_probe() needs
to cancel the work in that case
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/misc/appledisplay.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index ac92725458b5..ba1eaabc7796 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -164,7 +164,12 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
0,
pdata->msgdata, 2,
ACD_USB_TIMEOUT);
- brightness = pdata->msgdata[1];
+ if (retval < 2) {
+ if (retval >= 0)
+ retval = -EMSGSIZE;
+ } else {
+ brightness = pdata->msgdata[1];
+ }
mutex_unlock(&pdata->sysfslock);
if (retval < 0)
@@ -299,6 +304,7 @@ static int appledisplay_probe(struct usb_interface *iface,
if (pdata) {
if (pdata->urb) {
usb_kill_urb(pdata->urb);
+ cancel_delayed_work_sync(&pdata->work);
if (pdata->urbdata)
usb_free_coherent(pdata->udev, ACD_URB_BUFFER_LEN,
pdata->urbdata, pdata->urb->transfer_dma);
--
2.16.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
2019-11-05 23:36 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
2019-11-06 11:42 ` Oliver Neukum
@ 2019-11-06 12:10 ` Andrey Konovalov
2019-11-06 12:11 ` Andrey Konovalov
1 sibling, 1 reply; 9+ messages in thread
From: Andrey Konovalov @ 2019-11-06 12:10 UTC (permalink / raw)
To: Phong Tran
Cc: syzbot+495dab1f175edc9c2f13, 2pi, alex.theissen,
Greg Kroah-Hartman, LKML, USB list, syzkaller-bugs,
linux-kernel-mentees
On Wed, Nov 6, 2019 at 12:37 AM Phong Tran <tranmanphong@gmail.com> wrote:
>
> In context of USB disconnect, the delaywork trigger and calling
> appledisplay_bl_get_brightness() and the msgdata was freed.
>
> add the checking return value of usb_control_msg() and only update the
> data while the retval is valid.
>
> Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
> Reported-and-tested-by:
> syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
>
> https://groups.google.com/d/msg/syzkaller-bugs/dRmkh2UYusY/l2a6Mg3FAQAJ
Hi Phong,
FYI, when testing patches with the usb-fuzzer instance, you need to
provide the same kernel commit id as the one where the bug was
triggered. Please see here for details:
>
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
> drivers/usb/misc/appledisplay.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> index ac92725458b5..3e3dfa5a3954 100644
> --- a/drivers/usb/misc/appledisplay.c
> +++ b/drivers/usb/misc/appledisplay.c
> @@ -164,7 +164,8 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
> 0,
> pdata->msgdata, 2,
> ACD_USB_TIMEOUT);
> - brightness = pdata->msgdata[1];
> + if (retval >= 0)
> + brightness = pdata->msgdata[1];
> mutex_unlock(&pdata->sysfslock);
>
> if (retval < 0)
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
2019-11-06 12:10 ` Andrey Konovalov
@ 2019-11-06 12:11 ` Andrey Konovalov
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Konovalov @ 2019-11-06 12:11 UTC (permalink / raw)
To: Phong Tran
Cc: syzbot+495dab1f175edc9c2f13, 2pi, alex.theissen,
Greg Kroah-Hartman, LKML, USB list, syzkaller-bugs,
linux-kernel-mentees
On Wed, Nov 6, 2019 at 1:10 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Wed, Nov 6, 2019 at 12:37 AM Phong Tran <tranmanphong@gmail.com> wrote:
> >
> > In context of USB disconnect, the delaywork trigger and calling
> > appledisplay_bl_get_brightness() and the msgdata was freed.
> >
> > add the checking return value of usb_control_msg() and only update the
> > data while the retval is valid.
> >
> > Reported-by: syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
> > Reported-and-tested-by:
> > syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
> >
> > https://groups.google.com/d/msg/syzkaller-bugs/dRmkh2UYusY/l2a6Mg3FAQAJ
>
> Hi Phong,
>
> FYI, when testing patches with the usb-fuzzer instance, you need to
> provide the same kernel commit id as the one where the bug was
> triggered. Please see here for details:
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#usb-bugs
>
> >
> > Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> > ---
> > drivers/usb/misc/appledisplay.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> > index ac92725458b5..3e3dfa5a3954 100644
> > --- a/drivers/usb/misc/appledisplay.c
> > +++ b/drivers/usb/misc/appledisplay.c
> > @@ -164,7 +164,8 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
> > 0,
> > pdata->msgdata, 2,
> > ACD_USB_TIMEOUT);
> > - brightness = pdata->msgdata[1];
> > + if (retval >= 0)
> > + brightness = pdata->msgdata[1];
> > mutex_unlock(&pdata->sysfslock);
> >
> > if (retval < 0)
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: KASAN: use-after-free Read in appledisplay_bl_get_brightness
2019-11-06 11:42 ` Oliver Neukum
@ 2019-11-06 12:26 ` syzbot
2019-11-06 14:22 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
1 sibling, 0 replies; 9+ messages in thread
From: syzbot @ 2019-11-06 12:26 UTC (permalink / raw)
To: 2pi, alex.theissen, andreyknvl, gregkh, linux-kernel-mentees,
linux-kernel, linux-usb, oneukum, syzkaller-bugs, tranmanphong
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger
crash:
Reported-and-tested-by:
syzbot+495dab1f175edc9c2f13@syzkaller.appspotmail.com
Tested on:
commit: e0bd8d79 usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git
kernel config: https://syzkaller.appspot.com/x/.config?x=8847e5384a16f66a
dashboard link: https://syzkaller.appspot.com/bug?extid=495dab1f175edc9c2f13
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
patch: https://syzkaller.appspot.com/x/patch.diff?x=14d463b2e00000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
2019-11-06 11:42 ` Oliver Neukum
2019-11-06 12:26 ` KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
@ 2019-11-06 14:22 ` Phong Tran
2019-11-07 8:50 ` Oliver Neukum
1 sibling, 1 reply; 9+ messages in thread
From: Phong Tran @ 2019-11-06 14:22 UTC (permalink / raw)
To: Oliver Neukum, syzbot+495dab1f175edc9c2f13
Cc: tranmanphong, 2pi, alex.theissen, andreyknvl, gregkh,
linux-kernel, linux-usb, syzkaller-bugs, linux-kernel-mentees
On 11/6/19 6:42 PM, Oliver Neukum wrote:
> Am Mittwoch, den 06.11.2019, 06:36 +0700 schrieb Phong Tran:
>> In context of USB disconnect, the delaywork trigger and calling
>> appledisplay_bl_get_brightness() and the msgdata was freed.
>>
>> add the checking return value of usb_control_msg() and only update the
>> data while the retval is valid.
>
> Hi,
>
> I am afraid there are some issues with your patch. First, let me stress
> that you found the right place to fix an issue and you partially fixed
> an issue. But the the fix you applied is incomplete and left another
> issue open.
>
> Your patch still allows doing IO to a device that may already be bound
> to another driver. That is bad, especially as the buffer is already
> free. Yes, if IO is failing, you have fixed that narrow issue.
> But it need not fail.
>
> If you look into appledisplay_probe() you will see that it can fail
> because backlight_device_register() fails. The error handling will
> thereupon kill the URB and free memory. But it will not kill an already
> scheduled work. The scheduled work will then call usb_control_msg()
> on pdata->msgdata, which has been freed. If that IO fails, all is well.
> If not, the issue still exists.
>
Hello Oliver,
argee, there need a cancel workqueue in case error of probe().
> Secondly, your error check is off by 2. You are checking only for
> usb_control_msg() failing. But it can return only one byte instead
> of two. If that happens, the value you return is stale, although
> the buffer is correctly allocated.
>
> Regards
> Oliver
>
> The correct fix for both issues would be:
>
> #syz test: https://github.com/google/kasan.git e0bd8d79
>
> From 2497a62bdbeb9bd94f690c22d96dd1b8cf22861f Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Wed, 6 Nov 2019 12:36:28 +0100
> Subject: [PATCH] appledisplay: fix error handling in the scheduled work
>
> The work item can operate on
>
> 1. stale memory left over from the last transfer
> the actual length of the data transfered needs to be checked
> 2. memory already freed
> the error handling in appledisplay_probe() needs
> to cancel the work in that case
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
> drivers/usb/misc/appledisplay.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> index ac92725458b5..ba1eaabc7796 100644
> --- a/drivers/usb/misc/appledisplay.c
> +++ b/drivers/usb/misc/appledisplay.c
> @@ -164,7 +164,12 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
> 0,
> pdata->msgdata, 2,
> ACD_USB_TIMEOUT);
> - brightness = pdata->msgdata[1];
> + if (retval < 2) {
> + if (retval >= 0)
> + retval = -EMSGSIZE;
> + } else {
> + brightness = pdata->msgdata[1];
> + }
compare with message size (2) can be considered.
if (retval == 2) {
brightness = pdata->msgdata[1];
} else {
retval = -EMSGSIZE;
}
Regards,
Phong.
> mutex_unlock(&pdata->sysfslock);
>
> if (retval < 0)
> @@ -299,6 +304,7 @@ static int appledisplay_probe(struct usb_interface *iface,
> if (pdata) {
> if (pdata->urb) {
> usb_kill_urb(pdata->urb);
> + cancel_delayed_work_sync(&pdata->work);
> if (pdata->urbdata)
> usb_free_coherent(pdata->udev, ACD_URB_BUFFER_LEN,
> pdata->urbdata, pdata->urb->transfer_dma);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness
2019-11-06 14:22 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
@ 2019-11-07 8:50 ` Oliver Neukum
0 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2019-11-07 8:50 UTC (permalink / raw)
To: Phong Tran, syzbot+495dab1f175edc9c2f13
Cc: 2pi, alex.theissen, andreyknvl, gregkh, linux-kernel, linux-usb,
syzkaller-bugs, linux-kernel-mentees
Am Mittwoch, den 06.11.2019, 21:22 +0700 schrieb Phong Tran:
> compare with message size (2) can be considered.
>
> if (retval == 2) {
> brightness = pdata->msgdata[1];
> } else {
> retval = -EMSGSIZE;
> }
If you do it that way, the error reporting will be distorted.
Hence I did it a bit more complexly.
Regards
Oliver
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-07 9:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-23 14:31 KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
2019-09-23 14:53 ` Alexander Theißen
2019-11-05 23:36 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
2019-11-06 11:42 ` Oliver Neukum
2019-11-06 12:26 ` KASAN: use-after-free Read in appledisplay_bl_get_brightness syzbot
2019-11-06 14:22 ` [PATCH] usb: appledisplay: fix use-after-free in bl_get_brightness Phong Tran
2019-11-07 8:50 ` Oliver Neukum
2019-11-06 12:10 ` Andrey Konovalov
2019-11-06 12:11 ` Andrey Konovalov
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).