linux-kernel - Re: possible deadlock in mon_bin_vma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191120111235.7d306f23@suzdal.zaitcev.lan>
Date:   Wed, 20 Nov 2019 11:12:35 -0600
From:   Pete Zaitcev <zaitcev@...hat.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     syzbot <syzbot+56f9673bb4cdcbeb0e92@...kaller.appspotmail.com>,
        arnd@...db.de, <gregkh@...uxfoundation.org>,
        <jrdr.linux@...il.com>, <keescook@...omium.org>,
        <kstewart@...uxfoundation.org>,
        Kernel development list <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        <syzkaller-bugs@...glegroups.com>, <tglx@...utronix.de>,
        <viro@...iv.linux.org.uk>, zaitcev@...hat.com
Subject: Re: possible deadlock in mon_bin_vma_fault

On Wed, 20 Nov 2019 11:14:05 -0500 (EST)
Alan Stern <stern@...land.harvard.edu> wrote:

> As it happens, I spent a little time investigating this bug report just
> yesterday.  It seems to me that the easiest fix would be to disallow
> resizing the buffer while it is mapped by any users.  (Besides,
> allowing that seems like a bad idea in any case.)
> 
> Pete, does that seem reasonable to you?

Yes, it does seem reasonable.

I think I understand it now. My fallacy was thinking that since everything
is nailed down as long as fetch_lock is held, it was okay to grab whatever
page from our pagemap. What happens later is an attempt to get pages of the
new buffer while looking at them through the old VMA, in mon_bin_vma_fault.

It seems to me that the use counter, mmap_active, is correct and sufficient
to check in the ioctl.

-- Pete

P.S. One thing that vaguely bothers me on this is that the bot
bisected to the commit that clearly fixed worse issues.

P.P.S. Like this?

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..e27d99606adb 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1020,6 +1020,9 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
                int size;
                struct mon_pgmap *vec;
 
+               if (rp->mmap_active)
+                       return -EBUSY;
+
                if (arg < BUFF_MIN || arg > BUFF_MAX)
                        return -EINVAL;