On Thu, Aug 01, 2024 at 10:07:39AM -0400, Josef Bacik wrote:
> On Thu, Aug 01, 2024 at 02:07:45PM +0200, Wojciech Gładysz wrote:
> > Test case: thread mounts NOEXEC fuse to a file being executed.
> > WARN_ON_ONCE is triggered yielding panic for some config.
> > Add a check to security_bprm_creds_for_exec(bprm).
> >
>
> Need more detail here, a script or something to describe the series of events
> that gets us here, I can't quite figure out how to do this.
>
> > Stack trace:
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 2736 at fs/exec.c:933 do_open_execat+0x311/0x710 fs/exec.c:932
> > Modules linked in:
> > CPU: 0 PID: 2736 Comm: syz-executor384 Not tainted 5.10.0-syzkaller #0
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
> > RIP: 0010:do_open_execat+0x311/0x710 fs/exec.c:932
> > Code: 89 de e8 02 b1 a1 ff 31 ff 89 de e8 f9 b0 a1 ff 45 84 ff 75 2e 45 85 ed 0f 8f ed 03 00 00 e8 56 ae a1 ff eb bd e8 4f ae a1 ff <0f> 0b 48 c7 c3 f3 ff ff ff 4c 89 f7 e8 9e cb fe ff 49 89 de e9 2d
> > RSP: 0018:ffffc90008e07c20 EFLAGS: 00010293
> > RAX: ffffffff82131ac6 RBX: 0000000000000004 RCX: ffff88801a6611c0
> > RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000000
> > RBP: ffffc90008e07cf0 R08: ffffffff8213173f R09: ffffc90008e07aa0
> > R10: 0000000000000000 R11: dffffc0000000001 R12: ffff8880115810e0
> > R13: dffffc0000000000 R14: ffff88801122c040 R15: ffffc90008e07c60
> > FS: 00007f9e283ce6c0(0000) GS:ffff888058a00000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007f9e2848600a CR3: 00000000139de000 CR4: 0000000000352ef0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> > bprm_execve+0x60b/0x1c40 fs/exec.c:1939
> > do_execveat_common+0x5a6/0x770 fs/exec.c:2077
> > do_execve fs/exec.c:2147 [inline]
> > __do_sys_execve fs/exec.c:2223 [inline]
> > __se_sys_execve fs/exec.c:2218 [inline]
> > __x64_sys_execve+0x92/0xb0 fs/exec.c:2218
> > do_syscall_64+0x6d/0xa0 arch/x86/entry/common.c:62
> > entry_SYSCALL_64_after_hwframe+0x61/0xcb
> > RIP: 0033:0x7f9e2842f299
> > Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 b1 18 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
> > RSP: 002b:00007f9e283ce218 EFLAGS: 00000246 ORIG_RAX: 000000000000003b
> > RAX: ffffffffffffffda RBX: 00007f9e284bd3f8 RCX: 00007f9e2842f299
> > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000020000400
> > RBP: 00007f9e284bd3f0 R08: 0000000000000000 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 00007f9e2848a134
> > R13: 0030656c69662f2e R14: 00007ffc819a23d0 R15: 00007f9e28488130
> >
> > Signed-off-by: Wojciech Gładysz <wojciech.gladysz@xxxxxxxxxxxx>
> > ---
> > fs/exec.c | 42 +++++++++++++++++++-----------------------
> > 1 file changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index a126e3d1cacb..0cc6a7d033a1 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -953,8 +953,6 @@ EXPORT_SYMBOL(transfer_args_to_stack);
> > */
> > static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > {
> > - struct file *file;
> > - int err;
> > struct open_flags open_exec_flags = {
> > .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> > .acc_mode = MAY_EXEC,
> > @@ -969,26 +967,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > if (flags & AT_EMPTY_PATH)
> > open_exec_flags.lookup_flags |= LOOKUP_EMPTY;
> >
> > - file = do_filp_open(fd, name, &open_exec_flags);
> > - if (IS_ERR(file))
> > - goto out;
> > -
> > - /*
> > - * may_open() has already checked for this, so it should be
> > - * impossible to trip now. But we need to be extra cautious
> > - * and check again at the very end too.
> > - */
> > - err = -EACCES;
> > - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
> > - path_noexec(&file->f_path)))
> > - goto exit;
> > -
>
> This still needs to be left here to catch any bad actors in the future. Thanks,
>
This check is fundamentally racy.
path_noexec expands to the following:
return (path->mnt->mnt_flags & MNT_NOEXEC) ||
(path->mnt->mnt_sb->s_iflags & SB_I_NOEXEC);
An exec racing against remount setting the noexec flag can correctly
conclude the file can be execed and then trip over the check later if
the flag showed up in the meantime.
This is not fuse-specific and I disagree with the posted patch as well.
The snippet here tries to validate that permissions were correctly checked
at some point, but it fails that goal in 2 ways:
- the inode + fs combo might just happen to be fine for exec, even if
may_open *was not issued*
- there is the aforementioned race
If this thing here is supposed to stay, it instead needs to be
reimplemented with may_open setting a marker "checking for exec was
performed and execing is allowed" somewhere in struct file.
I'm not confident this is particularly valuable, but if it is, it
probably should hide behind some debug flags.
