[PATCH] fs: hfsplus: fix uninit-value bug in hfsplus_listxattr

[PATCH] fs: hfsplus: fix uninit-value bug in hfsplus_listxattr

[houweitao]

BUG: KMSAN: uninit-value in strncmp+0x11e/0x180 lib/string.c:307
 strncmp+0x11e/0x180 lib/string.c:307
 is_known_namespace fs/hfsplus/xattr.c:45 [inline]
 name_len fs/hfsplus/xattr.c:397 [inline]
 hfsplus_listxattr+0xe61/0x1aa0 fs/hfsplus/xattr.c:746
 vfs_listxattr fs/xattr.c:473 [inline]
 listxattr+0x700/0x780 fs/xattr.c:820
 path_listxattr fs/xattr.c:844 [inline]
 __do_sys_llistxattr fs/xattr.c:862 [inline]
 __se_sys_llistxattr fs/xattr.c:859 [inline]
 __ia32_sys_llistxattr+0x171/0x300 fs/xattr.c:859
 do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
 __do_fast_syscall_32+0xa2/0x100 arch/x86/entry/common.c:178
 do_fast_syscall_32+0x37/0x80 arch/x86/entry/common.c:203
 do_SYSENTER_32+0x1f/0x30 arch/x86/entry/common.c:246
 entry_SYSENTER_compat_after_hwframe+0x70/0x82

Reported-by: syzbot <syzbot+92ef9ee419803871020e@syzkaller.appspotmail.com>
Link: https://syzkaller.appspot.com/bug?extid=92ef9ee419803871020e
Signed-off-by: houweitao <houweitao@didiglobal.com>
---
 fs/hfsplus/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 58021e73c00b..f7f9d0889df3 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -698,7 +698,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		return err;
 	}
 
-	strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
+	strbuf = kzalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
 			XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
 	if (!strbuf) {
 		res = -ENOMEM;
-- 
2.17.1

Re: [PATCH] fs: hfsplus: fix uninit-value bug in hfsplus_listxattr

[Al Viro]

On Wed, May 10, 2023 at 10:25:15AM +0800, houweitao wrote:
> BUG: KMSAN: uninit-value in strncmp+0x11e/0x180 lib/string.c:307
>  strncmp+0x11e/0x180 lib/string.c:307
>  is_known_namespace fs/hfsplus/xattr.c:45 [inline]
>  name_len fs/hfsplus/xattr.c:397 [inline]
>  hfsplus_listxattr+0xe61/0x1aa0 fs/hfsplus/xattr.c:746
>  vfs_listxattr fs/xattr.c:473 [inline]
>  listxattr+0x700/0x780 fs/xattr.c:820
>  path_listxattr fs/xattr.c:844 [inline]
>  __do_sys_llistxattr fs/xattr.c:862 [inline]
>  __se_sys_llistxattr fs/xattr.c:859 [inline]
>  __ia32_sys_llistxattr+0x171/0x300 fs/xattr.c:859
>  do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
>  __do_fast_syscall_32+0xa2/0x100 arch/x86/entry/common.c:178
>  do_fast_syscall_32+0x37/0x80 arch/x86/entry/common.c:203
>  do_SYSENTER_32+0x1f/0x30 arch/x86/entry/common.c:246
>  entry_SYSENTER_compat_after_hwframe+0x70/0x82
> 
> Reported-by: syzbot <syzbot+92ef9ee419803871020e@syzkaller.appspotmail.com>
> Link: https://syzkaller.appspot.com/bug?extid=92ef9ee419803871020e
> Signed-off-by: houweitao <houweitao@didiglobal.com>

Why does it actually fix anything?  Other than making KMSAN STFU, that is...

"Fill it with zeroes" might or might not be a fix in this particular case,
but it really needs more detailed proof.

You might have figured it out, but how do I (or anybody else) even begin
to reason about the correctness of that fix?  By redoing the analysis from
scratch, starting with "in some conditions this stack trace might end up
reading uninitialized data in strbuf"?

NAK.  *IF* you have an explanation of what's going on and why this change
really fixes things, please repost with useful commit message.

RE: [PATCH] fs: hfsplus: fix uninit-value bug in hfsplus_listxattr

[侯伟桃 Vincent Hou]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2224 bytes --]

Sorry, I will update patch V2 with new comment later.

-----Original Message-----
From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
Sent: Thursday, May 11, 2023 12:36 PM
To: ºîΰÌÒ Vincent Hou <houweitao@didiglobal.com>
Cc: akpm@linux-foudation.org; xupengfei@nfschina.com; brauner@kernel.org; dchinner@redhat.com; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; Àî«hâù Roy Li <royliyueyi@didiglobal.com>
Subject: Re: [PATCH] fs: hfsplus: fix uninit-value bug in hfsplus_listxattr

On Wed, May 10, 2023 at 10:25:15AM +0800, houweitao wrote:
> BUG: KMSAN: uninit-value in strncmp+0x11e/0x180 lib/string.c:307
>  strncmp+0x11e/0x180 lib/string.c:307
>  is_known_namespace fs/hfsplus/xattr.c:45 [inline]  name_len 
> fs/hfsplus/xattr.c:397 [inline]
>  hfsplus_listxattr+0xe61/0x1aa0 fs/hfsplus/xattr.c:746  vfs_listxattr 
> fs/xattr.c:473 [inline]
>  listxattr+0x700/0x780 fs/xattr.c:820
>  path_listxattr fs/xattr.c:844 [inline]  __do_sys_llistxattr 
> fs/xattr.c:862 [inline]  __se_sys_llistxattr fs/xattr.c:859 [inline]
>  __ia32_sys_llistxattr+0x171/0x300 fs/xattr.c:859  
> do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
>  __do_fast_syscall_32+0xa2/0x100 arch/x86/entry/common.c:178
>  do_fast_syscall_32+0x37/0x80 arch/x86/entry/common.c:203
>  do_SYSENTER_32+0x1f/0x30 arch/x86/entry/common.c:246
>  entry_SYSENTER_compat_after_hwframe+0x70/0x82
> 
> Reported-by: syzbot 
> <syzbot+92ef9ee419803871020e@syzkaller.appspotmail.com>
> Link: https://syzkaller.appspot.com/bug?extid=92ef9ee419803871020e
> Signed-off-by: houweitao <houweitao@didiglobal.com>

Why does it actually fix anything?  Other than making KMSAN STFU, that is...

"Fill it with zeroes" might or might not be a fix in this particular case, but it really needs more detailed proof.

You might have figured it out, but how do I (or anybody else) even begin to reason about the correctness of that fix?  By redoing the analysis from scratch, starting with "in some conditions this stack trace might end up reading uninitialized data in strbuf"?

NAK.  *IF* you have an explanation of what's going on and why this change really fixes things, please repost with useful commit message.