[syzbot] KMSAN: uninit-value in tomoyo_path_chown

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 3a2b6b904ea7 x86: kmsan: enable KMSAN builds for x86
git tree: https://github.com/google/kmsan.git master
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13719145080000
kernel config: https://syzkaller.appspot.com/x/.config?x=8e64bc5364a1307e
dashboard link: https://syzkaller.appspot.com/bug?extid=541e21dcc32c4046cba9
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project.git 610139d2d9ce6746b3c617fb3e2f7886272d26ff), GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10cac995080000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16816c33080000

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

=====================================================
BUG: KMSAN: uninit-value in tomoyo_path_chown+0x121/0x240 security/tomoyo/tomoyo.c:366
tomoyo_path_chown+0x121/0x240 security/tomoyo/tomoyo.c:366
security_path_chown+0x17d/0x260 security/security.c:1224
chown_common+0x9f2/0xef0 fs/open.c:729
vfs_fchown fs/open.c:802 [inline]
ksys_fchown+0x229/0x360 fs/open.c:813
__do_sys_fchown fs/open.c:821 [inline]
__se_sys_fchown fs/open.c:819 [inline]
__x64_sys_fchown+0x8a/0xe0 fs/open.c:819
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

Local variable newattrs created at:
chown_common+0xd1/0xef0 fs/open.c:708
vfs_fchown fs/open.c:802 [inline]
ksys_fchown+0x229/0x360 fs/open.c:813

CPU: 0 PID: 3490 Comm: syz-executor426 Not tainted 6.0.0-rc2-syzkaller-47460-g3a2b6b904ea7 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
=====================================================


---
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.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Tetsuo Handa]

syzbot is reporting uninit-value in tomoyo_path_chown() [1], for
chown_common() is by error passing uninitialized newattrs.ia_vfsuid to
security_path_chown() via from_vfsuid() when user == -1 is passed.
We must initialize newattrs.ia_vfs{u,g}id fields in order to make
from_vfs{u,g}id() work.

Link: https://syzkaller.appspot.com/bug?extid=541e21dcc32c4046cba9 [1]
Reported-by: syzbot <syzbot+541e21...@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
---
fs/open.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/fs/open.c b/fs/open.c
index 8a813fa5ca56..0550efb7b53a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -709,6 +709,8 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
kuid_t uid;
kgid_t gid;

+ newattrs.ia_vfsuid = VFSUIDT_INIT(KUIDT_INIT(-1));
+ newattrs.ia_vfsgid = VFSGIDT_INIT(KGIDT_INIT(-1));
uid = make_kuid(current_user_ns(), user);
gid = make_kgid(current_user_ns(), group);

--
2.34.1

[Christian Brauner]

On Mon, Sep 19, 2022 at 08:05:12PM +0900, Tetsuo Handa wrote:
> syzbot is reporting uninit-value in tomoyo_path_chown() [1], for
> chown_common() is by error passing uninitialized newattrs.ia_vfsuid to
> security_path_chown() via from_vfsuid() when user == -1 is passed.
> We must initialize newattrs.ia_vfs{u,g}id fields in order to make
> from_vfs{u,g}id() work.
>
> Link: https://syzkaller.appspot.com/bug?extid=541e21dcc32c4046cba9 [1]
> Reported-by: syzbot <syzbot+541e21...@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
> ---

Odd that we didn't get any of the reports. Thanks for relying this.
I'll massage this a tiny bit, apply and will test with syzbot.

[Christian Brauner]

Fyi, Seth.

[Serge E. Hallyn]

Because the modules are ignoring ia_valid & ATTR_XID?

[Seth Forshee]

security_path_chown() takes ids as arguments, not struct iattr.

int security_path_chown(const struct path *path, kuid_t uid, kgid_t gid)

The ones being passed are now taken from iattr and thus potentially not
initialized. Even if we change it to only call security_path_chown()
when one of ATTR_{U,G}ID is set the other might not be set, so
initializing iattr.ia_vfs{u,g}id makes sense to me and should match the
old behavior of passing invalid ids in this situation.

What I don't understand is why security_path_chown() is even necessary
when we also have security_inode_setattr(), which also gets called
during chown and gets the full iattr struct. Maybe there's a good
reason, but at first glance it seems like it could do any checks that
security_path_chown() is doing.

Seth

[Seth Forshee]

Maybe the important difference is that one takes the path as an argument
and the other only takes the dentry? I guess that might be it, though it
still feels kind of redundant.

[Tetsuo Handa]

On 2022/09/20 9:29, Seth Forshee wrote:
>>>>> Odd that we didn't get any of the reports. Thanks for relying this.
>>>>> I'll massage this a tiny bit, apply and will test with syzbot.

Since KMSAN is not yet upstreamed, uninit-value reports can come only after bugs
reached upstream kernels. Also, only LSMs which implement security_path_chown()
hook and checks uid/gid values (i.e. TOMOYO) would catch this bug.

>>>>
>>>> Fyi, Seth.
>>>
>>> Because the modules are ignoring ia_valid & ATTR_XID?
>>
>> security_path_chown() takes ids as arguments, not struct iattr.
>>
>> int security_path_chown(const struct path *path, kuid_t uid, kgid_t gid)
>>
>> The ones being passed are now taken from iattr and thus potentially not
>> initialized. Even if we change it to only call security_path_chown()
>> when one of ATTR_{U,G}ID is set the other might not be set, so
>> initializing iattr.ia_vfs{u,g}id makes sense to me and should match the
>> old behavior of passing invalid ids in this situation.
>>
>> What I don't understand is why security_path_chown() is even necessary
>> when we also have security_inode_setattr(), which also gets called
>> during chown and gets the full iattr struct. Maybe there's a good
>> reason, but at first glance it seems like it could do any checks that
>> security_path_chown() is doing.
>
> Maybe the important difference is that one takes the path as an argument
> and the other only takes the dentry? I guess that might be it, though it
> still feels kind of redundant.

security_path_*() hooks are used by LSMs which check absolute pathnames.
Thus, these hooks are not redundant.

[Christian Brauner]

#syz test: https://github.com/google/kmsan.git master

From 76e8832fa00b9e7e80052bceae3ab8481c428bb1 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
Date: Mon, 19 Sep 2022 20:05:12 +0900
Subject: [PATCH] open: always initialize ownership fields

Beginning of the merge window we introduced the vfs{g,u}id_t types in
b27c82e12965 ("attr: port attribute changes to new types") and changed
various codepaths over including chown_common().

During that change we forgot to account for the case were the passed
ownership value is -1. In this case the ownership fields in struct iattr
aren't initialized but we rely on them being initialized by the time we
generate the ownership to pass down to the LSMs. All the major LSMs
don't care about the ownership values at all. Only Tomoyo uses them and
so it took a while for syzbot to unearth this issue.

Fix this by initializing the ownership fields and do it within the
retry_deleg block. While notify_change() doesn't alter the ownership
fields currently we shouldn't rely on it.

Since no kernel has been released with these changes this does not
needed to be backported to any stable kernels.

[Christian Brauner (Microsoft) <bra...@kernel.org>]
* rewrote commit message
* use INVALID_VFS{G,U}ID macros

Fixes: b27c82e12965 ("attr: port attribute changes to new types") # mainline only

Reported-by: syzbot <syzbot+541e21...@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>

Reviewed-by: Seth Forshee (DigitalOcean) <sfor...@kernel.org>
Signed-off-by: Christian Brauner (Microsoft) <bra...@kernel.org>

---
fs/open.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/fs/open.c b/fs/open.c

index 8a813fa5ca56..cf7e5c350a54 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -716,6 +716,8 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
fs_userns = i_user_ns(inode);

retry_deleg:
+ newattrs.ia_vfsuid = INVALID_VFSUID;
+ newattrs.ia_vfsgid = INVALID_VFSGID;
newattrs.ia_valid = ATTR_CTIME;
if ((user != (uid_t)-1) && !setattr_vfsuid(&newattrs, uid))
return -EINVAL;
--
2.34.1

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+541e21...@syzkaller.appspotmail.com

Tested on:

commit: 8f4ae27d Revert "Revert "crypto: kmsan: disable accele..

git tree: https://github.com/google/kmsan.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=126545d5080000
kernel config: https://syzkaller.appspot.com/x/.config?x=3553796fd72c7a2e

dashboard link: https://syzkaller.appspot.com/bug?extid=541e21dcc32c4046cba9
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project.git 610139d2d9ce6746b3c617fb3e2f7886272d26ff), GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=120de737080000

Note: testing is done by a robot and is best-effort only.

[Christian Brauner]

Ok, now that KMSAN confirmed that this fixes it and xfstests pass I'll
route it upstream:

tree: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git
branch: fs.chown_common.fix
commit: c73aaa634ec1 ("open: always initialize ownership fields")

Thanks!
Christian

[Alexander Potapenko]

Thanks for the quick turnaround! I'll pick the patch to the KMSAN tree
right now, so that syzbot can make further progress without hitting
this issue so often.

--
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[Christian Brauner]

On Tue, Sep 20, 2022 at 11:53:36AM +0200, Alexander Potapenko wrote:
> On Tue, Sep 20, 2022 at 11:50 AM Christian Brauner <bra...@kernel.org> wrote:
> >
> > On Tue, Sep 20, 2022 at 02:36:21AM -0700, syzbot wrote:
> > > Hello,
> > >
> > > syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> > >
> > > Reported-and-tested-by: syzbot+541e21...@syzkaller.appspotmail.com
> > >
> > > Tested on:
> > >
> > > commit: 8f4ae27d Revert "Revert "crypto: kmsan: disable accele..
> > > git tree: https://github.com/google/kmsan.git master
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=126545d5080000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=3553796fd72c7a2e
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=541e21dcc32c4046cba9
> > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project.git 610139d2d9ce6746b3c617fb3e2f7886272d26ff), GNU ld (GNU Binutils for Debian) 2.35.2
> > > patch: https://syzkaller.appspot.com/x/patch.diff?x=120de737080000
> > >
> > > Note: testing is done by a robot and is best-effort only.
> >
> > Ok, now that KMSAN confirmed that this fixes it and xfstests pass I'll
> > route it upstream:
> >
> > tree: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git
> > branch: fs.chown_common.fix
> > commit: c73aaa634ec1 ("open: always initialize ownership fields")
> >
> > Thanks!
> > Christian
>
> Thanks for the quick turnaround! I'll pick the patch to the KMSAN tree

Of course!

> right now, so that syzbot can make further progress without hitting
> this issue so often.

Fwiw, I just had to force-push because I added the tested-by from syzbot
so:

f52d74b190f8d10ec01cd5774eca77c2186c8ab7

is the new hash.