Re: possible deadlock in mnt_want_write

[Amir Goldstein]

On Wed, Nov 21, 2018 at 10:04 PM Goldwyn Rodrigues <rgoldwyn@xxxxxxx> wrote:
>
> On 20:57 21/11, Amir Goldstein wrote:
> > On Wed, Nov 21, 2018 at 8:33 PM syzbot
> > <syzbot+ae82084b07d0297e566b@xxxxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > >
...
> > >         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.