* [syzbot] KMSAN: uninit-value in tomoyo_path_chown @ 2022-09-19 8:10 syzbot 2022-09-19 11:05 ` [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() Tetsuo Handa 0 siblings, 1 reply; 8+ messages in thread From: syzbot @ 2022-09-19 8:10 UTC (permalink / raw) To: glider, jmorris, linux-kernel, linux-security-module, paul, penguin-kernel, serge, syzkaller-bugs, takedakn 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+541e21dcc32c4046cba9@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 syzkaller@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() 2022-09-19 8:10 [syzbot] KMSAN: uninit-value in tomoyo_path_chown syzbot @ 2022-09-19 11:05 ` Tetsuo Handa 2022-09-19 15:12 ` Christian Brauner 0 siblings, 1 reply; 8+ messages in thread From: Tetsuo Handa @ 2022-09-19 11:05 UTC (permalink / raw) To: linux-fsdevel, Christian Brauner, Alexander Viro Cc: linux-security-module, syzbot, syzkaller-bugs 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+541e21dcc32c4046cba9@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() 2022-09-19 11:05 ` [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() Tetsuo Handa @ 2022-09-19 15:12 ` Christian Brauner 2022-09-19 15:14 ` Christian Brauner 0 siblings, 1 reply; 8+ messages in thread From: Christian Brauner @ 2022-09-19 15:12 UTC (permalink / raw) To: Tetsuo Handa Cc: linux-fsdevel, Alexander Viro, linux-security-module, syzbot, syzkaller-bugs 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+541e21dcc32c4046cba9@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() 2022-09-19 15:12 ` Christian Brauner @ 2022-09-19 15:14 ` Christian Brauner 2022-09-19 23:41 ` Serge E. Hallyn 0 siblings, 1 reply; 8+ messages in thread From: Christian Brauner @ 2022-09-19 15:14 UTC (permalink / raw) To: Tetsuo Handa Cc: linux-fsdevel, Alexander Viro, linux-security-module, syzbot, syzkaller-bugs, Seth Forshee On Mon, Sep 19, 2022 at 05:12:25PM +0200, Christian Brauner wrote: > 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+541e21dcc32c4046cba9@syzkaller.appspotmail.com> > > Signed-off-by: Tetsuo Handa <penguin-kernel@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. Fyi, Seth. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() 2022-09-19 15:14 ` Christian Brauner @ 2022-09-19 23:41 ` Serge E. Hallyn 2022-09-20 0:25 ` Seth Forshee 0 siblings, 1 reply; 8+ messages in thread From: Serge E. Hallyn @ 2022-09-19 23:41 UTC (permalink / raw) To: Christian Brauner Cc: Tetsuo Handa, linux-fsdevel, Alexander Viro, linux-security-module, syzbot, syzkaller-bugs, Seth Forshee On Mon, Sep 19, 2022 at 05:14:14PM +0200, Christian Brauner wrote: > On Mon, Sep 19, 2022 at 05:12:25PM +0200, Christian Brauner wrote: > > 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+541e21dcc32c4046cba9@syzkaller.appspotmail.com> > > > Signed-off-by: Tetsuo Handa <penguin-kernel@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. > > Fyi, Seth. Because the modules are ignoring ia_valid & ATTR_XID? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() 2022-09-19 23:41 ` Serge E. Hallyn @ 2022-09-20 0:25 ` Seth Forshee 2022-09-20 0:29 ` Seth Forshee 0 siblings, 1 reply; 8+ messages in thread From: Seth Forshee @ 2022-09-20 0:25 UTC (permalink / raw) To: Serge E. Hallyn Cc: Christian Brauner, Tetsuo Handa, linux-fsdevel, Alexander Viro, linux-security-module, syzbot, syzkaller-bugs On Mon, Sep 19, 2022 at 06:41:02PM -0500, Serge E. Hallyn wrote: > On Mon, Sep 19, 2022 at 05:14:14PM +0200, Christian Brauner wrote: > > On Mon, Sep 19, 2022 at 05:12:25PM +0200, Christian Brauner wrote: > > > 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+541e21dcc32c4046cba9@syzkaller.appspotmail.com> > > > > Signed-off-by: Tetsuo Handa <penguin-kernel@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. > > > > 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. Seth ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() 2022-09-20 0:25 ` Seth Forshee @ 2022-09-20 0:29 ` Seth Forshee 2022-09-20 0:45 ` Tetsuo Handa 0 siblings, 1 reply; 8+ messages in thread From: Seth Forshee @ 2022-09-20 0:29 UTC (permalink / raw) To: Serge E. Hallyn Cc: Christian Brauner, Tetsuo Handa, linux-fsdevel, Alexander Viro, linux-security-module, syzbot, syzkaller-bugs On Mon, Sep 19, 2022 at 07:25:39PM -0500, Seth Forshee wrote: > On Mon, Sep 19, 2022 at 06:41:02PM -0500, Serge E. Hallyn wrote: > > On Mon, Sep 19, 2022 at 05:14:14PM +0200, Christian Brauner wrote: > > > On Mon, Sep 19, 2022 at 05:12:25PM +0200, Christian Brauner wrote: > > > > 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+541e21dcc32c4046cba9@syzkaller.appspotmail.com> > > > > > Signed-off-by: Tetsuo Handa <penguin-kernel@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. > > > > > > 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() 2022-09-20 0:29 ` Seth Forshee @ 2022-09-20 0:45 ` Tetsuo Handa 0 siblings, 0 replies; 8+ messages in thread From: Tetsuo Handa @ 2022-09-20 0:45 UTC (permalink / raw) To: Seth Forshee, Serge E. Hallyn Cc: Christian Brauner, linux-fsdevel, Alexander Viro, linux-security-module, syzbot, syzkaller-bugs 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-20 0:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-19 8:10 [syzbot] KMSAN: uninit-value in tomoyo_path_chown syzbot 2022-09-19 11:05 ` [PATCH (urgent)] vfs: fix uninitialized uid/gid in chown_common() Tetsuo Handa 2022-09-19 15:12 ` Christian Brauner 2022-09-19 15:14 ` Christian Brauner 2022-09-19 23:41 ` Serge E. Hallyn 2022-09-20 0:25 ` Seth Forshee 2022-09-20 0:29 ` Seth Forshee 2022-09-20 0:45 ` Tetsuo Handa
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.