linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* suspicious capability check in ovl_ioctl_set_flags
@ 2020-11-17  7:56 Dmitry Vyukov
  2020-11-17  8:58 ` Miklos Szeredi
  0 siblings, 1 reply; 2+ messages in thread
From: Dmitry Vyukov @ 2020-11-17  7:56 UTC (permalink / raw)
  To: Miklos Szeredi, overlayfs, LKML
  Cc: Alexander Potapenko, Merna Zakaria, kasan-dev

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?
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
==================================================================

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: suspicious capability check in ovl_ioctl_set_flags
  2020-11-17  7:56 suspicious capability check in ovl_ioctl_set_flags Dmitry Vyukov
@ 2020-11-17  8:58 ` Miklos Szeredi
  0 siblings, 0 replies; 2+ messages in thread
From: Miklos Szeredi @ 2020-11-17  8:58 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: overlayfs, LKML, Alexander Potapenko, Merna Zakaria, kasan-dev

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
> ==================================================================

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2020-11-17  8:58 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17  7:56 suspicious capability check in ovl_ioctl_set_flags Dmitry Vyukov
2020-11-17  8:58 ` Miklos Szeredi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).