Re: suspicious capability check in ovl_ioctl_set_flags

From: Miklos Szeredi <miklos@szeredi.hu>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: overlayfs <linux-unionfs@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Alexander Potapenko <glider@google.com>,
	Merna Zakaria <mernazakaria@google.com>,
	kasan-dev <kasan-dev@googlegroups.com>
Subject: Re: suspicious capability check in ovl_ioctl_set_flags
Date: Tue, 17 Nov 2020 09:58:18 +0100	[thread overview]
Message-ID: <CAJfpegvoiGb5R1Y2a+_rNgTXgfJD=kFrkXBn7zSZDHKxwe992Q@mail.gmail.com> (raw)
In-Reply-To: <CACT4Y+bUfavwMVv2SEMve5pabE_AwsDO0YsRBGZtYqX59a77vA@mail.gmail.com>

On Tue, Nov 17, 2020 at 8:56 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> Hi Miklos,
>
> We've detected a suspicious double-fetch of user-space data in
> ovl_ioctl_set_flags using a prototype tool (see report below [1]).
>
> It points to ovl_ioctl_set_flags that does a capability check using
> flags, but then the real ioctl double-fetches flags and uses
> potentially different value:
>
> static long ovl_ioctl_set_flags(struct file *file, unsigned int cmd,
>                 unsigned long arg, unsigned int flags)
> {
> ...
>     /* Check the capability before cred override */
>     oldflags = ovl_iflags_to_fsflags(READ_ONCE(inode->i_flags));
>     ret = vfs_ioc_setflags_prepare(inode, oldflags, flags);
>     if (ret)
>         goto unlock;
> ...
>     ret = ovl_real_ioctl(file, cmd, arg);
>
> All fs impls call vfs_ioc_setflags_prepare again, so the capability is
> checked again.
>
> But I think this makes the vfs_ioc_setflags_prepare check in overlayfs
> pointless (?) and the "Check the capability before cred override"
> comment misleading, user can skip this check by presenting benign
> flags first and then overwriting them to non-benign flags. Or, if this
> check is still needed... it is wrong (?). The code would need to
> arrange for both ioctl's to operate on the same data then.
> Does it make any sense?

Yes, looks like an oversight.

The only way to fix this properly, AFAICS is to add i_op->setflags.

Will look into this.

Thanks,
Miklos

> Thanks
>
> [1] BUG: multi-read in __x64_sys_ioctl  between ovl_ioctl and ext4_ioctl
> ======= First Address Range Stack =======
>  df_save_stack+0x33/0x70 lib/df-detection.c:208
>  add_address+0x2ac/0x352 lib/df-detection.c:47
>  ovl_ioctl_set_fsflags fs/overlayfs/file.c:607 [inline]
>  ovl_ioctl+0x7d/0x290 fs/overlayfs/file.c:654
>  vfs_ioctl fs/ioctl.c:48 [inline]
>  __do_sys_ioctl fs/ioctl.c:753 [inline]
>  __se_sys_ioctl fs/ioctl.c:739 [inline]
>  __x64_sys_ioctl+0xfc/0x140 fs/ioctl.c:739
>  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ======= Second Address Range Stack =======
>  df_save_stack+0x33/0x70 lib/df-detection.c:208
>  add_address+0x2ac/0x352 lib/df-detection.c:47
>  ext4_ioctl+0x13b1/0x27f0 fs/ext4/ioctl.c:833
>  vfs_ioctl+0x30/0x80 fs/ioctl.c:48
>  ovl_real_ioctl+0xed/0x100 fs/overlayfs/file.c:539
>  ovl_ioctl_set_flags+0x11d/0x180 fs/overlayfs/file.c:574
>  ovl_ioctl_set_fsflags fs/overlayfs/file.c:610 [inline]
>  ovl_ioctl+0x11e/0x290 fs/overlayfs/file.c:654
>  vfs_ioctl fs/ioctl.c:48 [inline]
>  __do_sys_ioctl fs/ioctl.c:753 [inline]
>  __se_sys_ioctl fs/ioctl.c:739 [inline]
>  __x64_sys_ioctl+0xfc/0x140 fs/ioctl.c:739
>  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> syscall number 16  System Call: __x64_sys_ioctl+0x0/0x140 fs/ioctl.c:800
> First 0000000020000000 len 4 Caller vfs_ioctl fs/ioctl.c:48 [inline]
> First 0000000020000000 len 4 Caller __do_sys_ioctl fs/ioctl.c:753 [inline]
> First 0000000020000000 len 4 Caller __se_sys_ioctl fs/ioctl.c:739 [inline]
> First 0000000020000000 len 4 Caller __x64_sys_ioctl+0xfc/0x140 fs/ioctl.c:739
> Second 0000000020000000 len 4 Caller vfs_ioctl+0x30/0x80 fs/ioctl.c:48
> ==================================================================