lkml.org 
[lkml]   [2020]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: suspicious capability check in ovl_ioctl_set_flags
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
> ==================================================================

\
 
 \ /
  Last update: 2020-11-17 10:01    [W:0.029 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site