Re: KASAN: use-after-free Read in usblp_bulk_read
On Wed, 6 May 2020, Pete Zaitcev wrote: > On Wed, 06 May 2020 11:14:42 +0200 > Oliver Neukum wrote: > > > Very well. We are not going to find it without exceptional luck. Yet > > there may be a real issue, too. We simply do not know. How about the > > attached patch? > > > usblp_unlink_urbs(usblp); > > mutex_unlock(>mut); > > + usb_poison_anchored_urbs(>urbs); > > > > if (!usblp->used) > > usblp_cleanup(usblp); > > This can't be right. Our URBs are freed by the callback, and this > technique is not compatible with poisoning, at least with how the > usb/core.c implements it. The usb_poison_urb() waits for URB > to complete, and if the callback frees it, it's a problem. That's not a problem. URBs are reference-counted, and usb_poison_anchored_urbs() does usb_get_urb() before and usb_put_urb() after calling usb_poison_urb(). Alan Stern
Re: KASAN: use-after-free Read in usblp_bulk_read
On Wed, 06 May 2020 11:14:42 +0200 Oliver Neukum wrote: > Very well. We are not going to find it without exceptional luck. Yet > there may be a real issue, too. We simply do not know. How about the > attached patch? > usblp_unlink_urbs(usblp); > mutex_unlock(>mut); > + usb_poison_anchored_urbs(>urbs); > > if (!usblp->used) > usblp_cleanup(usblp); This can't be right. Our URBs are freed by the callback, and this technique is not compatible with poisoning, at least with how the usb/core.c implements it. The usb_poison_urb() waits for URB to complete, and if the callback frees it, it's a problem. -- Pete
Re: KASAN: use-after-free Read in usblp_bulk_read
On Wed, 6 May 2020, Oliver Neukum wrote: > Am Donnerstag, den 30.04.2020, 11:11 -0400 schrieb Alan Stern: > > > KASAN is documented. The difficulty is that this race is obviously > > hard to trigger, and without the ability to reproduce it we can't run > > diagnostics to find the underlying cause. > > > > We can't even ask syzbot to try running tests for us; without a valid > > reproducer it won't agree to rerun the original test program. > > > Very well. We are not going to find it without exceptional luck. Yet > there may be a real issue, too. We simply do not know. How about the > attached patch? It's okay with me (apart from the typo in the patch description: "UB"). Alan Stern
Re: KASAN: use-after-free Read in usblp_bulk_read
Am Donnerstag, den 30.04.2020, 11:11 -0400 schrieb Alan Stern: > KASAN is documented. The difficulty is that this race is obviously > hard to trigger, and without the ability to reproduce it we can't run > diagnostics to find the underlying cause. > > We can't even ask syzbot to try running tests for us; without a valid > reproducer it won't agree to rerun the original test program. Very well. We are not going to find it without exceptional luck. Yet there may be a real issue, too. We simply do not know. How about the attached patch? Regards Oliver From 5ed23e0029cf10cf8dbdd790a190d7e2113560ae Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Wed, 6 May 2020 11:05:41 +0200 Subject: [PATCH] usblp: poison URBs upon disconnect syzkaller reported an UB that should have been killed to be active. We do not understand it, but this should fix the issue if it is real. Signed-off-by: Oliver Neukum --- drivers/usb/class/usblp.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c index 0d8e3f3804a3..084c48c5848f 100644 --- a/drivers/usb/class/usblp.c +++ b/drivers/usb/class/usblp.c @@ -468,7 +468,8 @@ static int usblp_release(struct inode *inode, struct file *file) usb_autopm_put_interface(usblp->intf); if (!usblp->present) /* finish cleanup from disconnect */ - usblp_cleanup(usblp); + usblp_cleanup(usblp); /* any URBs must be dead */ + mutex_unlock(_mutex); return 0; } @@ -1375,9 +1376,11 @@ static void usblp_disconnect(struct usb_interface *intf) usblp_unlink_urbs(usblp); mutex_unlock(>mut); + usb_poison_anchored_urbs(>urbs); if (!usblp->used) usblp_cleanup(usblp); + mutex_unlock(_mutex); } -- 2.16.4
Re: KASAN: use-after-free Read in usblp_bulk_read
On Thu, 30 Apr 2020, Oliver Neukum wrote: > Am Dienstag, den 21.04.2020, 08:35 -0700 schrieb syzbot: > > Hello, > > > > syzbot found the following crash on: > > > > HEAD commit:0fa84af8 Merge tag 'usb-serial-5.7-rc1' of https://git.ker.. > > git tree: https://github.com/google/kasan.git usb-fuzzer > > console output: https://syzkaller.appspot.com/x/log.txt?x=126f75d7e0 > > kernel config: https://syzkaller.appspot.com/x/.config?x=6b9c154b0c23aecf > > dashboard link: https://syzkaller.appspot.com/bug?extid=be5b5f86a162a6c281e6 > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > Reported-by: syzbot+be5b5f86a162a6c28...@syzkaller.appspotmail.com > > > > usblp0: nonzero read bulk status received: -71 > > OK, we have this report and nobody understands it. If I may summarize: > > 1. We do not conclusively know how the URB was submitted > 2. We are clear about which memory was freed and accessed > 3. We agree that the URB should have been unlinked Or should not have been submitted. > Do we agree on what we agree on? > > Theories: > > A. There is a race that would allow disconnect() and resume() to run > concurrently > > B. There is a race in usblp which affects 'used' > > C. There is a bug in the virtual driver that can make unlinking an URB > fail > > What do you think? How to investigate this further and is it worth it? > Do we have documentation on what KASAN does? KASAN is documented. The difficulty is that this race is obviously hard to trigger, and without the ability to reproduce it we can't run diagnostics to find the underlying cause. We can't even ask syzbot to try running tests for us; without a valid reproducer it won't agree to rerun the original test program. Alan Stern
Re: KASAN: use-after-free Read in usblp_bulk_read
Am Dienstag, den 21.04.2020, 08:35 -0700 schrieb syzbot: > Hello, > > syzbot found the following crash on: > > HEAD commit:0fa84af8 Merge tag 'usb-serial-5.7-rc1' of https://git.ker.. > git tree: https://github.com/google/kasan.git usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=126f75d7e0 > kernel config: https://syzkaller.appspot.com/x/.config?x=6b9c154b0c23aecf > dashboard link: https://syzkaller.appspot.com/bug?extid=be5b5f86a162a6c281e6 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+be5b5f86a162a6c28...@syzkaller.appspotmail.com > > usblp0: nonzero read bulk status received: -71 OK, we have this report and nobody understands it. If I may summarize: 1. We do not conclusively know how the URB was submitted 2. We are clear about which memory was freed and accessed 3. We agree that the URB should have been unlinked Do we agree on what we agree on? Theories: A. There is a race that would allow disconnect() and resume() to run concurrently B. There is a race in usblp which affects 'used' C. There is a bug in the virtual driver that can make unlinking an URB fail What do you think? How to investigate this further and is it worth it? Do we have documentation on what KASAN does? Regards Oliver
