Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <000000000000d03eea0571adfe83@google.com> <000000000000a8e163057b2f2ddf@google.com>
 <CAOQ4uxhAwdaZnPLX6+qYWaNL6NHQDMptx5TMyzo3B7CCcgWTjg@mail.gmail.com> <20181121200114.yqis7fvn5x7nsw6e@merlin>
In-Reply-To: <20181121200114.yqis7fvn5x7nsw6e@merlin>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Wed, 21 Nov 2018 22:22:26 +0200
Message-ID: <CAOQ4uxgzxmpY6TLR6FGZRvSyCYfED9bM-5oKjWi9VnL9d9kF2w@mail.gmail.com>
Subject: Re: possible deadlock in mnt_want_write
To:     Goldwyn Rodrigues <rgoldwyn@suse.de>
Cc:     syzbot+ae82084b07d0297e566b@syzkaller.appspotmail.com,
        Mimi Zohar <zohar@linux.vnet.ibm.com>,
        Miklos Szeredi <mszeredi@redhat.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        syzkaller-bugs@googlegroups.com, Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 21, 2018 at 10:04 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> On 20:57 21/11, Amir Goldstein wrote:
> > On Wed, Nov 21, 2018 at 8:33 PM syzbot
> > <syzbot+ae82084b07d0297e566b@syzkaller.appspotmail.com> wrote:
> > >
> > > syzbot has found a reproducer for the following crash on:
> > >
> > > HEAD commit:    442b8cea2477 Add linux-next specific files for 20181109
> > > git tree:       linux-next
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=11a1426d400000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=2f72bdb11df9fbe8
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=ae82084b07d0297e566b
> > > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1632326d400000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17a16ed5400000
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+ae82084b07d0297e566b@syzkaller.appspotmail.com
> > >
...
> > >         percpu_down_read include/linux/percpu-rwsem.h:59 [inline]
> > >         __sb_start_write+0x214/0x370 fs/super.c:1564
> > >         sb_start_write include/linux/fs.h:1607 [inline]
> > >         mnt_want_write+0x3f/0xc0 fs/namespace.c:359
> > >         ovl_want_write+0x76/0xa0 fs/overlayfs/util.c:24
> > >         ovl_open_maybe_copy_up+0x12c/0x190 fs/overlayfs/copy_up.c:888
> > >         ovl_open+0xb3/0x260 fs/overlayfs/file.c:123
> > >         do_dentry_open+0x499/0x1250 fs/open.c:771
> > >         vfs_open fs/open.c:880 [inline]
> > >         dentry_open+0x143/0x1d0 fs/open.c:896
> > >         ima_calc_file_hash+0x324/0x570
> >
> > I suppose ima_calc_file_hash opens the file with write flags
> > and cause overlay to try to copy up which takes mnt_want_write().
> > Why does IMA need to open the file with write flags?
> >
> > Isn't this commit supposed to prevent that:
> > a408e4a86b36 ima: open a new file instance if no read permissions
> >
>
> Not write, read flags. This patch re-opens the files in O_RDONLY for
> files opened with O_WRONLY and cannot be read, so that the hash can be
> calculated. IOW, the user opened the file in overlayfs with write flags.
>

My point is:
ovl_open_need_copy_up() -> ovl_open_flags_need_copy_up()
returns false for O_RDONLY flags and never gets to ovl_want_write(),
so how is the stack trace below possible when ima_calc_file_hash()
removes all "write" flags before opening the file?

  ovl_want_write+0x76/0xa0 fs/overlayfs/util.c:24
  ovl_open_maybe_copy_up+0x12c/0x190 fs/overlayfs/copy_up.c:888
  ovl_open+0xb3/0x260 fs/overlayfs/file.c:123
  do_dentry_open+0x499/0x1250 fs/open.c:771
  vfs_open fs/open.c:880 [inline]
  dentry_open+0x143/0x1d0 fs/open.c:896
  ima_calc_file_hash+0x324/0x570 security/integrity/ima/ima_crypto.c:427

The answer is found in the zysbot repro: it opens the file with
open(O_WRONLY|O_RDWR) (0x3)
Not nice, but apparently possible.

How about adding O_RDWR to the masked flags in ima_calc_file_hash()?
Since syzbot has a reproducer, you can send it a patch to verify the fix.

Thanks,
Amir.