On Tue, Nov 17, 2020 at 8:56 AM Dmitry Vyukov <dvyukov@xxxxxxxxxx> 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
> ==================================================================
