[syzbot] [ntfs?] WARNING in do_open_execat

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 16931859a650 Merge tag 'nfsd-6.5-4' of git://git.kernel.or..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13e2673da80000
kernel config: https://syzkaller.appspot.com/x/.config?x=aa796b6080b04102
dashboard link: https://syzkaller.appspot.com/bug?extid=6ec38f7a8db3b3fb1002
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17cdbc65a80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1262d8cfa80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/eecc010800b4/disk-16931859.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/f45ae06377a7/vmlinux-16931859.xz
kernel image: https://storage.googleapis.com/syzbot-assets/68891896edba/bzImage-16931859.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/4b6ab78b223a/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+6ec38f...@syzkaller.appspotmail.com

ntfs: volume version 3.1.
process 'syz-executor300' launched './file1' with NULL argv: empty string added
------------[ cut here ]------------
WARNING: CPU: 0 PID: 5020 at fs/exec.c:933 do_open_execat+0x18f/0x3f0 fs/exec.c:933
Modules linked in:
CPU: 0 PID: 5020 Comm: syz-executor300 Not tainted 6.5.0-rc6-syzkaller-00038-g16931859a650 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
RIP: 0010:do_open_execat+0x18f/0x3f0 fs/exec.c:933
Code: 8e 46 02 00 00 41 0f b7 1e bf 00 80 ff ff 66 81 e3 00 f0 89 de e8 b1 67 9b ff 66 81 fb 00 80 0f 84 8b 00 00 00 e8 51 6c 9b ff <0f> 0b 48 c7 c3 f3 ff ff ff e8 43 6c 9b ff 4c 89 e7 e8 4b c9 fe ff
RSP: 0018:ffffc90003b0fd10 EFLAGS: 00010293
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff888028401dc0 RSI: ffffffff81ea9c4f RDI: 0000000000000003
RBP: 1ffff92000761fa2 R08: 0000000000000003 R09: 0000000000008000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff88802bf18780
R13: ffff888075d70000 R14: ffff8880742776a0 R15: 0000000000000001
FS: 000055555706b380(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffe0f1d3ff8 CR3: 0000000015f97000 CR4: 0000000000350ef0
Call Trace:
<TASK>
bprm_execve fs/exec.c:1830 [inline]
bprm_execve+0x49d/0x1a50 fs/exec.c:1811
do_execveat_common.isra.0+0x5d3/0x740 fs/exec.c:1963
do_execve fs/exec.c:2037 [inline]
__do_sys_execve fs/exec.c:2113 [inline]
__se_sys_execve fs/exec.c:2108 [inline]
__x64_sys_execve+0x8c/0xb0 fs/exec.c:2108
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fee7ec27b39
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 61 17 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 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe6c369d28 EFLAGS: 00000246 ORIG_RAX: 000000000000003b
RAX: ffffffffffffffda RBX: 0031656c69662f2e RCX: 00007fee7ec27b39
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000020000480
RBP: 00007fee7ec7004b R08: 000000000001ee3b R09: 0000000000000000
R10: 00007ffe6c369bf0 R11: 0000000000000246 R12: 00007fee7ec70055
R13: 00007ffe6c369f08 R14: 0000000000000001 R15: 0000000000000001
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Eric W. Biederman]

syzbot <syzbot+6ec38f...@syzkaller.appspotmail.com> writes:

> Hello,
>
> syzbot found the following issue on:

Not an issue.
Nothing to do with ntfs.

The code is working as designed and intended.

syzbot generated a malformed exec and the kernel made it
well formed and warned about it.

Human beings who run syzbot please mark this as not an issue in your
system. The directions don't have a way to say that the code is working
as expected and designed.

Eric

[Kees Cook]

On Fri, Aug 18, 2023 at 11:26:51AM -0500, Eric W. Biederman wrote:
> syzbot <syzbot+6ec38f...@syzkaller.appspotmail.com> writes:
>
> > Hello,
> >
> > syzbot found the following issue on:
>
> Not an issue.
> Nothing to do with ntfs.
>
> The code is working as designed and intended.
>
> syzbot generated a malformed exec and the kernel made it
> well formed and warned about it.
>
> Human beings who run syzbot please mark this as not an issue in your
> system. The directions don't have a way to say that the code is working
> as expected and designed.

WARN and BUG should not be reachable from userspace, so if this can be
tripped we should take a closer look and likely fix it...

> > HEAD commit: 16931859a650 Merge tag 'nfsd-6.5-4' of git://git.kernel.or..
> > git tree: upstream
> > console+strace: https://syzkaller.appspot.com/x/log.txt?x=13e2673da80000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=aa796b6080b04102
> > dashboard link: https://syzkaller.appspot.com/bug?extid=6ec38f7a8db3b3fb1002
> > compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17cdbc65a80000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1262d8cfa80000
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/eecc010800b4/disk-16931859.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/f45ae06377a7/vmlinux-16931859.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/68891896edba/bzImage-16931859.xz
> > mounted in repro: https://storage.googleapis.com/syzbot-assets/4b6ab78b223a/mount_0.gz
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+6ec38f...@syzkaller.appspotmail.com
> >
> > ntfs: volume version 3.1.
> > process 'syz-executor300' launched './file1' with NULL argv: empty string added
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 5020 at fs/exec.c:933 do_open_execat+0x18f/0x3f0 fs/exec.c:933

This is a double-check I left in place, since it shouldn't have been reachable:

/*
* 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;

So yes, let's figure this out...

--
Kees Cook

[Matthew Wilcox]

When trying to figure it out, remember that ntfs corrupts random memory,
so all reports from syzbot that have "ntfs" in them should be discarded.
I tried to tell them that all this work they're doing testing ntfs3 is
pointless, but they won't listen.

[Kees Cook]

!! Oh. Well, then yeah, that's not great. :(

-Kees

> so all reports from syzbot that have "ntfs" in them should be discarded.
> I tried to tell them that all this work they're doing testing ntfs3 is
> pointless, but they won't listen.

--
Kees Cook

[Mateusz Guzik]

On Fri, Aug 18, 2023 at 11:26:51AM -0500, Eric W. Biederman wrote:

> syzbot <syzbot+6ec38f...@syzkaller.appspotmail.com> writes:
>
> > Hello,
> >
> > syzbot found the following issue on:
>
> Not an issue.
> Nothing to do with ntfs.
>
> The code is working as designed and intended.
>
> syzbot generated a malformed exec and the kernel made it
> well formed and warned about it.
>

There is definitely an issue here.

The warn on comes from:

/*
* 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;

Where path_noexec is:
return (path->mnt->mnt_flags & MNT_NOEXEC) ||
(path->mnt->mnt_sb->s_iflags & SB_I_NOEXEC);

So as is I think this can race with mount update *adding* noexec after
may_open() returned but before the code gets to recheck.

I suspect i_mode can also change in the time window.

However, the reproducer does not try to race anything so this is not why
the warn.

From my reading the kernel always lands in may_open() (as in, not
fs-specific) so as is I'm puzzled as to what happened. Maybe I'll try to
repro later.

[Mateusz Guzik]

On Fri, Aug 18, 2023 at 10:33:26AM -0700, Kees Cook wrote:

> This is a double-check I left in place, since it shouldn't have been reachable:
>
> /*
> * 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;
>

As I mentioned in my other e-mail, the check is racy -- an unlucky
enough remounting with noexec should trip over it, and probably a chmod
too.

However, that's not what triggers the warn in this case.

The ntfs image used here is intentionally corrupted and the inode at
hand has a mode of 777 (as in type not specified).

Then the type check in may_open():
switch (inode->i_mode & S_IFMT) {

fails to match anything.

This debug printk:
diff --git a/fs/namei.c b/fs/namei.c
index e56ff39a79bc..05652e8a1069 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3259,6 +3259,10 @@ static int may_open(struct mnt_idmap *idmap, const struct path *path,
if ((acc_mode & MAY_EXEC) && path_noexec(path))
return -EACCES;
break;
+ default:
+ /* bogus mode! */
+ printk(KERN_EMERG "got bogus mode inode!\n");
+ return -EACCES;
}

error = inode_permission(idmap, inode, MAY_OPEN | acc_mode);

catches it.

All that said, I think adding a WARN_ONCE here is prudent, but I
don't know if denying literally all opts is the way to go.

Do other filesystems have provisions to prevent inodes like this from
getting here?

[Eric W. Biederman]

Mateusz Guzik <mjg...@gmail.com> writes:

> On Fri, Aug 18, 2023 at 11:26:51AM -0500, Eric W. Biederman wrote:
>> syzbot <syzbot+6ec38f...@syzkaller.appspotmail.com> writes:
>>
>> > Hello,
>> >
>> > syzbot found the following issue on:
>>
>> Not an issue.
>> Nothing to do with ntfs.
>>
>> The code is working as designed and intended.
>>
>> syzbot generated a malformed exec and the kernel made it
>> well formed and warned about it.
>>
>
> There is definitely an issue here.
>
> The warn on comes from:
> /*
> * 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;
>
> Where path_noexec is:
> return (path->mnt->mnt_flags & MNT_NOEXEC) ||
> (path->mnt->mnt_sb->s_iflags & SB_I_NOEXEC);

My confusion.

I was seeing the message from
if (retval == 0)
pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n",
current->comm, bprm->filename);

I made the mistake of assuming that that was generating the backtrace.
The lack of args to execveat appears to be working fine.

I see you tracked this down to a non-exhaustive check in may_open.
Apologies for the noise.

Eric

[Christian Brauner]

Bugs reported against the VFS from ntfs/ntfs3 are to be treated with
extreme caution. Frankly, if it isn't reproducible without a corrupted
ntfs/ntfs3 image it is to be dismissed until further notice.

In this case it simply seems that ntfs is failing at ensuring that its
own inodes it reads from disk have a well-defined type.

If ntfs fails to validate that its own inodes it puts into the icache
are correctly initialized then the vfs doesn't need to try and taper
over this.

If ntfs fails at that, there's no guarantee that it doesn't also fail at
setting the correct i_ops for that inode. At which point we can check
the type in may_open() but we already used bogus i_ops the whole time on
some other inodes.

We're not here to make up for silly bugs like this. That WARN belongs
into ntfs not the vfs.

[Theodore Ts'o]

On Fri, Aug 18, 2023 at 09:12:39PM +0200, Mateusz Guzik wrote:
>

> The ntfs image used here is intentionally corrupted and the inode at
> hand has a mode of 777 (as in type not specified).
>
> Then the type check in may_open():
> switch (inode->i_mode & S_IFMT) {
>
> fails to match anything.

> ...

>
> Do other filesystems have provisions to prevent inodes like this from
> getting here?

Well, what ext4 does is that we do a bunch of basic validity checks in
ext4_iget(), and if the inode is bad (for example the type is not
specified), the following gets executed:

} else {
ret = -EFSCORRUPTED;
ext4_error_inode(inode, function, line, 0,
"iget: bogus i_mode (%o)", inode->i_mode);
goto bad_inode;
...

bad_inode:
brelse(iloc.bh);
iget_failed(inode);
return ERR_PTR(ret);

iget_failed() takes the inode under construction (returned by
iget_locked), and marks it as a bad/"dead" inode. So subsequent
attempts to do anything with the inode, including opening it, will
fail at the VFS level, and you never get to the file system's open
function.

The ext4_error_inode() function is reponsible for logging the error,
and if userspace is using fsnotify and are subscribed FS_ERROR,
notifying user space that the file system is corrupted. Depending on
the file system settings, we may also remount the file system
read-only, or force a panic to reboot the system (so that a failover
backup server can take over), or just log the message and continuing.

- Ted

[Mateusz Guzik]

Given the triggered WARN_ON it seemed to me this would be the
operating procedure, I am happy it is not ;)

Per your description and the one provided by Theodore I take it
filesystems must not ship botched inodes like this one.

While in this case this is a clear-cut bug in ntfs, I would argue the
entire ordeal exposes a deficiency in VFS -- it should have a
debug-only mechanism which catches cases like this early on. For
example there could be a mandatory function to call when the
filesystem claims it constructed the inode to assert a bunch on it --
it would not catch all possible problems, but would definitely catch
this one (and VFS would have to detect the call was not made).

Perhaps I should write a separate e-mail about this, but I'm surprised
there is no debug-only (as in not present in production kernels)
support for asserting the state. To give one example which makes me
itchy see inode destruction. There are few checks in clear_inode, but
past that there is almost nothing.

Similarly there are quite a few comments how the caller is required to
hold a given lock, which should have been converted to lockdep asserts
years ago.

I'm going to write something up later.

--
Mateusz Guzik <mjguzik gmail.com>

[syzbot]

syzbot suspects this issue was fixed by commit:

commit 6f861765464f43a71462d52026fbddfc858239a5
Author: Jan Kara <ja...@suse.cz>
Date: Wed Nov 1 17:43:10 2023 +0000

fs: Block writes to mounted block devices

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=17e3f58e180000
start commit: eb3479bc23fa Merge tag 'kbuild-fixes-v6.7' of git://git.ke..
git tree: upstream
kernel config: https://syzkaller.appspot.com/x/.config?x=bdf178b2f20f99b0
dashboard link: https://syzkaller.appspot.com/bug?extid=6ec38f7a8db3b3fb1002
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15073fd4e80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17b20b8f680000

If the result looks correct, please mark the issue as fixed by replying with:

#syz fix: fs: Block writes to mounted block devices

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[Jan Kara]

Honza
--
Jan Kara <ja...@suse.com>
SUSE Labs, CR

[Mateusz Guzik]

I don't think that's correct.

The bug is ntfs instantiating an inode with bogus type (based on an
intentionally corrupted filesystem), violating the api contract with
vfs, which in turn results in the warning way later.

It may be someone sorted out ntfs doing this in the meantime, I have
not checked.

With this in mind I don't believe your patch fixed it, at best it
happened to neuter the reproducer.

vfs could definitely be patched to catch this when I_NEW is getting
cleared (only when running with debug), not in the spot which
generates the warn.

[Jan Kara]

OK, I didn't dig deep into the bug. I've just seen there are no working
reproducers and given this is ntfs3 which doesn't really have great
maintenance effort put into it, I've opted for closing the bug. If there's
a way to tickle the bug without writing to mounted block device, syzbot
should eventually find it and create a new issue... But if you want to look
into this feel free to :) Thanks for sharing the info.

[Mateusz Guzik]

Maybe I'll get around to future-proofing by adding validation before
the inode escapes the filesystem code, but I'm definitely NOT patching
ntfs. 8->