* [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read
@ 2019-11-26 6:44 Pete Zaitcev
2019-11-26 15:20 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Pete Zaitcev @ 2019-11-26 6:44 UTC (permalink / raw)
To: linux-usb; +Cc: Pete Zaitcev
The problem arises because our read() function grabs a lock of the
circular buffer, finds something of interest, then invokes copy_to_user()
straight from the buffer, which in turn takes mm->mmap_sem. In the same
time, the callback mon_bin_vma_fault() is invoked under mm->mmap_sem.
It attempts to take the fetch lock and deadlocks.
This patch does away with protecting of our page list with any
semaphores, and instead relies on the kernel not close the device
while mmap is active in a process.
In addition, we prohibit re-sizing of a buffer while mmap is active.
This way, when (now unlocked) fault is processed, it works with the
page that is intended to be mapped-in, and not some other random page.
Note that this may have an ABI impact, but hopefully no legitimate
program is this wrong.
Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com
---
drivers/usb/mon/mon_bin.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..f48a23adbc35 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
mutex_lock(&rp->fetch_lock);
spin_lock_irqsave(&rp->b_lock, flags);
- mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
- kfree(rp->b_vec);
- rp->b_vec = vec;
- rp->b_size = size;
- rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
- rp->cnt_lost = 0;
+ if (rp->mmap_active) {
+ mon_free_buff(vec, size/CHUNK_SIZE);
+ kfree(vec);
+ ret = -EBUSY;
+ } else {
+ mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+ kfree(rp->b_vec);
+ rp->b_vec = vec;
+ rp->b_size = size;
+ rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+ rp->cnt_lost = 0;
+ }
spin_unlock_irqrestore(&rp->b_lock, flags);
mutex_unlock(&rp->fetch_lock);
}
@@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
static void mon_bin_vma_open(struct vm_area_struct *vma)
{
struct mon_reader_bin *rp = vma->vm_private_data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&rp->b_lock, flags);
rp->mmap_active++;
+ spin_unlock_irqrestore(&rp->b_lock, flags);
}
static void mon_bin_vma_close(struct vm_area_struct *vma)
{
+ unsigned long flags;
+
struct mon_reader_bin *rp = vma->vm_private_data;
+ spin_lock_irqsave(&rp->b_lock, flags);
rp->mmap_active--;
+ spin_unlock_irqrestore(&rp->b_lock, flags);
}
/*
@@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
unsigned long offset, chunk_idx;
struct page *pageptr;
- mutex_lock(&rp->fetch_lock);
offset = vmf->pgoff << PAGE_SHIFT;
- if (offset >= rp->b_size) {
- mutex_unlock(&rp->fetch_lock);
+ if (offset >= rp->b_size)
return VM_FAULT_SIGBUS;
- }
chunk_idx = offset / CHUNK_SIZE;
pageptr = rp->b_vec[chunk_idx].pg;
get_page(pageptr);
- mutex_unlock(&rp->fetch_lock);
vmf->page = pageptr;
return 0;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read
2019-11-26 6:44 [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read Pete Zaitcev
@ 2019-11-26 15:20 ` Alan Stern
2019-11-27 4:35 ` Pete Zaitcev
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2019-11-26 15:20 UTC (permalink / raw)
To: Greg KH, Pete Zaitcev; +Cc: USB list
On Tue, 26 Nov 2019, Pete Zaitcev wrote:
> The problem arises because our read() function grabs a lock of the
> circular buffer, finds something of interest, then invokes copy_to_user()
> straight from the buffer, which in turn takes mm->mmap_sem. In the same
> time, the callback mon_bin_vma_fault() is invoked under mm->mmap_sem.
> It attempts to take the fetch lock and deadlocks.
>
> This patch does away with protecting of our page list with any
> semaphores, and instead relies on the kernel not close the device
> while mmap is active in a process.
>
> In addition, we prohibit re-sizing of a buffer while mmap is active.
> This way, when (now unlocked) fault is processed, it works with the
> page that is intended to be mapped-in, and not some other random page.
> Note that this may have an ABI impact, but hopefully no legitimate
> program is this wrong.
>
> Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
> Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Also this should have:
Fixes: 46eb14a6e158 ("USB: fix usbmon BUG trigger")
CC: <stable@vger.kernel.org>
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read
2019-11-26 15:20 ` Alan Stern
@ 2019-11-27 4:35 ` Pete Zaitcev
2019-11-27 6:55 ` Greg KH
2019-11-27 15:07 ` Alan Stern
0 siblings, 2 replies; 6+ messages in thread
From: Pete Zaitcev @ 2019-11-27 4:35 UTC (permalink / raw)
To: Alan Stern; +Cc: Greg KH, USB list
On Tue, 26 Nov 2019 10:20:14 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:
> > Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
> > Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com
>
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Thanks.
> Fixes: 46eb14a6e158 ("USB: fix usbmon BUG trigger")
Indeed... Either I didn't think that one through, or the copy_to_user
used not to take the mmap_sem.
> CC: <stable@vger.kernel.org>
Do we really need this? The problem was in the code for more than 10 years.
It's not like anyone is exploiting systems because of it.
If we do need it, I should cc: the submission to the same place too, right?
-- Pete
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read
2019-11-27 4:35 ` Pete Zaitcev
@ 2019-11-27 6:55 ` Greg KH
2019-11-27 15:07 ` Alan Stern
1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2019-11-27 6:55 UTC (permalink / raw)
To: Pete Zaitcev; +Cc: Alan Stern, USB list
On Tue, Nov 26, 2019 at 10:35:09PM -0600, Pete Zaitcev wrote:
> On Tue, 26 Nov 2019 10:20:14 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
>
> > > Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
> > > Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com
> >
> > Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
>
> Thanks.
>
> > Fixes: 46eb14a6e158 ("USB: fix usbmon BUG trigger")
>
> Indeed... Either I didn't think that one through, or the copy_to_user
> used not to take the mmap_sem.
>
> > CC: <stable@vger.kernel.org>
>
> Do we really need this? The problem was in the code for more than 10 years.
> It's not like anyone is exploiting systems because of it.
Well now we all have a simple reproducer for it, so yes, it should be
backported. I'm doing that for all of the syzbot stuff.
> If we do need it, I should cc: the submission to the same place too, right?
Nope, the tag is just fine, that's all that is needed. I'll add the
above to the patch when applying it to my trees.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read
2019-11-27 4:35 ` Pete Zaitcev
2019-11-27 6:55 ` Greg KH
@ 2019-11-27 15:07 ` Alan Stern
1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2019-11-27 15:07 UTC (permalink / raw)
To: Pete Zaitcev; +Cc: Greg KH, USB list
On Tue, 26 Nov 2019, Pete Zaitcev wrote:
> On Tue, 26 Nov 2019 10:20:14 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
>
> > > Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
> > > Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com
> >
> > Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
>
> Thanks.
>
> > Fixes: 46eb14a6e158 ("USB: fix usbmon BUG trigger")
>
> Indeed... Either I didn't think that one through, or the copy_to_user
> used not to take the mmap_sem.
copy_to_user doesn't, but the fault handler does (the core handler, not
the fault routine in mon_bin.c). After all, it doesn't want the memory
map to change while a page is being read in to satisfy the fault.
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read
@ 2019-12-05 2:39 Pete Zaitcev
0 siblings, 0 replies; 6+ messages in thread
From: Pete Zaitcev @ 2019-12-05 2:39 UTC (permalink / raw)
To: linux-usb; +Cc: Pete Zaitcev
The problem arises because our read() function grabs a lock of the
circular buffer, finds something of interest, then invokes copy_to_user()
straight from the buffer, which in turn takes mm->mmap_sem. In the same
time, the callback mon_bin_vma_fault() is invoked under mm->mmap_sem.
It attempts to take the fetch lock and deadlocks.
This patch does away with protecting of our page list with any
semaphores, and instead relies on the kernel not close the device
while mmap is active in a process.
In addition, we prohibit re-sizing of a buffer while mmap is active.
This way, when (now unlocked) fault is processed, it works with the
page that is intended to be mapped-in, and not some other random page.
Note that this may have an ABI impact, but hopefully no legitimate
program is this wrong.
Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Fixes: 46eb14a6e158 ("USB: fix usbmon BUG trigger")
Cc: <stable@vger.kernel.org>
---
drivers/usb/mon/mon_bin.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..f48a23adbc35 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
mutex_lock(&rp->fetch_lock);
spin_lock_irqsave(&rp->b_lock, flags);
- mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
- kfree(rp->b_vec);
- rp->b_vec = vec;
- rp->b_size = size;
- rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
- rp->cnt_lost = 0;
+ if (rp->mmap_active) {
+ mon_free_buff(vec, size/CHUNK_SIZE);
+ kfree(vec);
+ ret = -EBUSY;
+ } else {
+ mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+ kfree(rp->b_vec);
+ rp->b_vec = vec;
+ rp->b_size = size;
+ rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+ rp->cnt_lost = 0;
+ }
spin_unlock_irqrestore(&rp->b_lock, flags);
mutex_unlock(&rp->fetch_lock);
}
@@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
static void mon_bin_vma_open(struct vm_area_struct *vma)
{
struct mon_reader_bin *rp = vma->vm_private_data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&rp->b_lock, flags);
rp->mmap_active++;
+ spin_unlock_irqrestore(&rp->b_lock, flags);
}
static void mon_bin_vma_close(struct vm_area_struct *vma)
{
+ unsigned long flags;
+
struct mon_reader_bin *rp = vma->vm_private_data;
+ spin_lock_irqsave(&rp->b_lock, flags);
rp->mmap_active--;
+ spin_unlock_irqrestore(&rp->b_lock, flags);
}
/*
@@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
unsigned long offset, chunk_idx;
struct page *pageptr;
- mutex_lock(&rp->fetch_lock);
offset = vmf->pgoff << PAGE_SHIFT;
- if (offset >= rp->b_size) {
- mutex_unlock(&rp->fetch_lock);
+ if (offset >= rp->b_size)
return VM_FAULT_SIGBUS;
- }
chunk_idx = offset / CHUNK_SIZE;
pageptr = rp->b_vec[chunk_idx].pg;
get_page(pageptr);
- mutex_unlock(&rp->fetch_lock);
vmf->page = pageptr;
return 0;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-05 2:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-26 6:44 [PATCH] usb: mon: Fix a deadlock in usbmon between mmap and read Pete Zaitcev
2019-11-26 15:20 ` Alan Stern
2019-11-27 4:35 ` Pete Zaitcev
2019-11-27 6:55 ` Greg KH
2019-11-27 15:07 ` Alan Stern
2019-12-05 2:39 Pete Zaitcev
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).