general protection fault in reiserfs_security_init

general protection fault in reiserfs_security_init

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    325d0eab Merge branch 'akpm' (patches from Andrew)
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1671c0e3900000
kernel config:  https://syzkaller.appspot.com/x/.config?x=b12e84189082991c
dashboard link: https://syzkaller.appspot.com/bug?extid=690cb1e51970435f9775
compiler:       gcc (GCC) 10.1.0-syz 20200507
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15705a3d900000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=117b3281900000

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

REISERFS (device loop0): journal params: device loop0, size 15748, journal first block 18, max trans len 256, max batch 225, max commit age 30, max trans age 30
REISERFS (device loop0): checking transaction log (loop0)
REISERFS (device loop0): Using tea hash to sort names
REISERFS (device loop0): using 3.5.x disk format
general protection fault, probably for non-canonical address 0xdffffc000000000d: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000068-0x000000000000006f]
CPU: 0 PID: 6874 Comm: syz-executor834 Not tainted 5.9.0-rc5-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:d_really_is_negative include/linux/dcache.h:472 [inline]
RIP: 0010:reiserfs_xattr_jcreate_nblocks fs/reiserfs/xattr.h:78 [inline]
RIP: 0010:reiserfs_security_init+0x285/0x4d0 fs/reiserfs/xattr_security.c:70
Code: 48 c1 ea 03 80 3c 02 00 0f 85 2b 02 00 00 4d 8b ad a0 05 00 00 48 b8 00 00 00 00 00 fc ff df 49 8d 7d 68 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 23 02 00 00 49 83 7d 68 00 0f 84 62 01 00 00 48
RSP: 0018:ffffc90005827980 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 0000000000000036 RCX: 000000000000006c
RDX: 000000000000000d RSI: ffffffff82009dd3 RDI: 0000000000000068
RBP: ffff88807d8441d0 R08: ffffc90005827a10 R09: ffffc90005827a18
R10: 0000000000000000 R11: 0000000000000000 R12: 00000000000005fa
R13: 0000000000000000 R14: ffff888094e60000 R15: 0000000000000000
FS:  0000000001036880(0000) GS:ffff8880ae400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f5a6fb90ab4 CR3: 000000009a1ab000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 reiserfs_mkdir+0x2c9/0x980 fs/reiserfs/namei.c:821
 create_privroot fs/reiserfs/xattr.c:882 [inline]
 reiserfs_xattr_init+0x4de/0xb52 fs/reiserfs/xattr.c:1004
 reiserfs_fill_super+0x215d/0x2df3 fs/reiserfs/super.c:2177
 mount_bdev+0x32e/0x3f0 fs/super.c:1417
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1547
 do_new_mount fs/namespace.c:2875 [inline]
 path_mount+0x1387/0x20a0 fs/namespace.c:3192
 do_mount fs/namespace.c:3205 [inline]
 __do_sys_mount fs/namespace.c:3413 [inline]
 __se_sys_mount fs/namespace.c:3390 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3390
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x447d9a
Code: b8 08 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 7d a3 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 5a a3 fb ff c3 66 0f 1f 84 00 00 00 00 00
RSP: 002b:00007fffe558f5c8 EFLAGS: 00000297 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007fffe558f620 RCX: 0000000000447d9a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007fffe558f5e0
RBP: 00007fffe558f5e0 R08: 00007fffe558f620 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000297 R12: 0000000000000006
R13: 0000000000000004 R14: 0000000000000003 R15: 0000000000000003
Modules linked in:
---[ end trace e6a0a9f4ee2cea86 ]---
RIP: 0010:d_really_is_negative include/linux/dcache.h:472 [inline]
RIP: 0010:reiserfs_xattr_jcreate_nblocks fs/reiserfs/xattr.h:78 [inline]
RIP: 0010:reiserfs_security_init+0x285/0x4d0 fs/reiserfs/xattr_security.c:70
Code: 48 c1 ea 03 80 3c 02 00 0f 85 2b 02 00 00 4d 8b ad a0 05 00 00 48 b8 00 00 00 00 00 fc ff df 49 8d 7d 68 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 23 02 00 00 49 83 7d 68 00 0f 84 62 01 00 00 48
RSP: 0018:ffffc90005827980 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 0000000000000036 RCX: 000000000000006c
RDX: 000000000000000d RSI: ffffffff82009dd3 RDI: 0000000000000068
RBP: ffff88807d8441d0 R08: ffffc90005827a10 R09: ffffc90005827a18
R10: 0000000000000000 R11: 0000000000000000 R12: 00000000000005fa
R13: 0000000000000000 R14: ffff888094e60000 R15: 0000000000000000
FS:  0000000001036880(0000) GS:ffff8880ae400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ff7d575b000 CR3: 000000009a1ab000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
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

Re: general protection fault in reiserfs_security_init

[syzbot]

syzbot has bisected this issue to:

commit 1592c4b9935fa8a3b7c297955bb872a357e5a3b6
Author: Linhua Xu <linhua.xu@unisoc.com>
Date:   Wed Mar 25 08:25:28 2020 +0000

    pinctrl: sprd: Add pin high impedance mode support

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=111050d3900000
start commit:   325d0eab Merge branch 'akpm' (patches from Andrew)
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=131050d3900000
console output: https://syzkaller.appspot.com/x/log.txt?x=151050d3900000
kernel config:  https://syzkaller.appspot.com/x/.config?x=b12e84189082991c
dashboard link: https://syzkaller.appspot.com/bug?extid=690cb1e51970435f9775
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15705a3d900000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=117b3281900000

Reported-by: syzbot+690cb1e51970435f9775@syzkaller.appspotmail.com
Fixes: 1592c4b9935f ("pinctrl: sprd: Add pin high impedance mode support")

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

[PATCH] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

syzbot is reporting NULL pointer dereference at reiserfs_security_init()
[1], for commit ab17c4f02156c4f7 ("reiserfs: fixup xattr_root caching") is
assuming that REISERFS_SB(s)->xattr_root != NULL in
reiserfs_xattr_jcreate_nblocks() despite that commit made
REISERFS_SB(sb)->priv_root != NULL && REISERFS_SB(s)->xattr_root == NULL
case possible.

I guess that commit 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating
privroot with selinux enabled") wanted to check xattr_root != NULL before
reiserfs_xattr_jcreate_nblocks(), for the changelog is talking about the
xattr root.

 The issue is that while creating the privroot during mount
 reiserfs_security_init calls reiserfs_xattr_jcreate_nblocks which
 dereferences the xattr root.  The xattr root doesn't exist, so we get an
 oops.

Therefore, update reiserfs_xattrs_initialized() to check both the privroot
and the xattr root.

[1] https://syzkaller.appspot.com/bug?id=8abaedbdeb32c861dc5340544284167dd0e46cde

Reported-and-tested-by: syzbot <syzbot+690cb1e51970435f9775@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating privroot with selinux enabled")
---
 fs/reiserfs/xattr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/reiserfs/xattr.h b/fs/reiserfs/xattr.h
index c764352447ba..81bec2c80b25 100644
--- a/fs/reiserfs/xattr.h
+++ b/fs/reiserfs/xattr.h
@@ -43,7 +43,7 @@ void reiserfs_security_free(struct reiserfs_security_handle *sec);
 
 static inline int reiserfs_xattrs_initialized(struct super_block *sb)
 {
-	return REISERFS_SB(sb)->priv_root != NULL;
+	return REISERFS_SB(sb)->priv_root && REISERFS_SB(sb)->xattr_root;
 }
 
 #define xattr_size(size) ((size) + sizeof(struct reiserfs_xattr_header))
-- 
2.18.4

[PATCH (resend)] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

syzbot is reporting NULL pointer dereference at reiserfs_security_init()
[1], for commit ab17c4f02156c4f7 ("reiserfs: fixup xattr_root caching") is
assuming that REISERFS_SB(s)->xattr_root != NULL in
reiserfs_xattr_jcreate_nblocks() despite that commit made
REISERFS_SB(sb)->priv_root != NULL && REISERFS_SB(s)->xattr_root == NULL
case possible.

I guess that commit 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating
privroot with selinux enabled") wanted to check xattr_root != NULL before
reiserfs_xattr_jcreate_nblocks(), for the changelog is talking about the
xattr root.

 The issue is that while creating the privroot during mount
 reiserfs_security_init calls reiserfs_xattr_jcreate_nblocks which
 dereferences the xattr root.  The xattr root doesn't exist, so we get an
 oops.

Therefore, update reiserfs_xattrs_initialized() to check both the privroot
and the xattr root.

[1] https://syzkaller.appspot.com/bug?id=8abaedbdeb32c861dc5340544284167dd0e46cde

Reported-and-tested-by: syzbot <syzbot+690cb1e51970435f9775@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating privroot with selinux enabled")
---
 fs/reiserfs/xattr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/reiserfs/xattr.h b/fs/reiserfs/xattr.h
index c764352447ba..81bec2c80b25 100644
--- a/fs/reiserfs/xattr.h
+++ b/fs/reiserfs/xattr.h
@@ -43,7 +43,7 @@ void reiserfs_security_free(struct reiserfs_security_handle *sec);
 
 static inline int reiserfs_xattrs_initialized(struct super_block *sb)
 {
-	return REISERFS_SB(sb)->priv_root != NULL;
+	return REISERFS_SB(sb)->priv_root && REISERFS_SB(sb)->xattr_root;
 }
 
 #define xattr_size(size) ((size) + sizeof(struct reiserfs_xattr_header))
-- 
2.18.4

Re: [PATCH (resend)] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

Can somebody review and pick up this trivial patch?
This bug is currently 6th top crasher for syzbot.

On 2021/03/05 15:31, Tetsuo Handa wrote:
> syzbot is reporting NULL pointer dereference at reiserfs_security_init()
> [1], for commit ab17c4f02156c4f7 ("reiserfs: fixup xattr_root caching") is
> assuming that REISERFS_SB(s)->xattr_root != NULL in
> reiserfs_xattr_jcreate_nblocks() despite that commit made
> REISERFS_SB(sb)->priv_root != NULL && REISERFS_SB(s)->xattr_root == NULL
> case possible.
> 
> I guess that commit 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating
> privroot with selinux enabled") wanted to check xattr_root != NULL before
> reiserfs_xattr_jcreate_nblocks(), for the changelog is talking about the
> xattr root.
> 
>  The issue is that while creating the privroot during mount
>  reiserfs_security_init calls reiserfs_xattr_jcreate_nblocks which
>  dereferences the xattr root.  The xattr root doesn't exist, so we get an
>  oops.
> 
> Therefore, update reiserfs_xattrs_initialized() to check both the privroot
> and the xattr root.
> 
> [1] https://syzkaller.appspot.com/bug?id=8abaedbdeb32c861dc5340544284167dd0e46cde
> 
> Reported-and-tested-by: syzbot <syzbot+690cb1e51970435f9775@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating privroot with selinux enabled")
> ---
>  fs/reiserfs/xattr.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/reiserfs/xattr.h b/fs/reiserfs/xattr.h
> index c764352447ba..81bec2c80b25 100644
> --- a/fs/reiserfs/xattr.h
> +++ b/fs/reiserfs/xattr.h
> @@ -43,7 +43,7 @@ void reiserfs_security_free(struct reiserfs_security_handle *sec);
>  
>  static inline int reiserfs_xattrs_initialized(struct super_block *sb)
>  {
> -	return REISERFS_SB(sb)->priv_root != NULL;
> +	return REISERFS_SB(sb)->priv_root && REISERFS_SB(sb)->xattr_root;
>  }
>  
>  #define xattr_size(size) ((size) + sizeof(struct reiserfs_xattr_header))
> 

[PATCH] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

syzbot is reporting NULL pointer dereference at reiserfs_security_init()
[1], for commit ab17c4f02156c4f7 ("reiserfs: fixup xattr_root caching") is
assuming that REISERFS_SB(s)->xattr_root != NULL in
reiserfs_xattr_jcreate_nblocks() despite that commit made
REISERFS_SB(sb)->priv_root != NULL && REISERFS_SB(s)->xattr_root == NULL
case possible.

I guess that commit 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating
privroot with selinux enabled") wanted to check xattr_root != NULL before
reiserfs_xattr_jcreate_nblocks(), for the changelog is talking about the
xattr root.

 The issue is that while creating the privroot during mount
 reiserfs_security_init calls reiserfs_xattr_jcreate_nblocks which
 dereferences the xattr root.  The xattr root doesn't exist, so we get an
 oops.

Therefore, update reiserfs_xattrs_initialized() to check both the privroot
and the xattr root.

[1] https://syzkaller.appspot.com/bug?id=8abaedbdeb32c861dc5340544284167dd0e46cde

Reported-and-tested-by: syzbot <syzbot+690cb1e51970435f9775@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating privroot with selinux enabled")
---
 fs/reiserfs/xattr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Escalating from reiserfs-devel@vger.kernel.org to linux-fsdevel@vger.kernel.org , for
no response from reiserfs developers for one month. If still no response from fsdevel
people, I would have to directly send to Linus...

diff --git a/fs/reiserfs/xattr.h b/fs/reiserfs/xattr.h
index c764352447ba..81bec2c80b25 100644
--- a/fs/reiserfs/xattr.h
+++ b/fs/reiserfs/xattr.h
@@ -43,7 +43,7 @@ void reiserfs_security_free(struct reiserfs_security_handle *sec);
 
 static inline int reiserfs_xattrs_initialized(struct super_block *sb)
 {
-	return REISERFS_SB(sb)->priv_root != NULL;
+	return REISERFS_SB(sb)->priv_root && REISERFS_SB(sb)->xattr_root;
 }
 
 #define xattr_size(size) ((size) + sizeof(struct reiserfs_xattr_header))
-- 
2.18.4

Re: [PATCH] reiserfs: update reiserfs_xattrs_initialized() condition

[Linus Torvalds]

On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> syzbot is reporting NULL pointer dereference at reiserfs_security_init()

Whee. Both of the mentioned commits go back over a decade.

I guess I could just take this directly, but let's add Jeff Mahoney
and Jan Kara to the participants in case they didn't see it on the
fsdevel list. I think they might want to be kept in the loop.

I'll forward the original in a separate email to them.

Jeff/Jan - just let me know if I should just apply this as-is.
Otherwise I'd expect it to (eventually) come in through Jan's random
fs tree, which is how I think most of these things have come in ..

           Linus

Re: [PATCH (resend)] reiserfs: update reiserfs_xattrs_initialized() condition

[Jan Kara]

On Fri 05-03-21 15:31:26, Tetsuo Handa wrote:
> syzbot is reporting NULL pointer dereference at reiserfs_security_init()
> [1], for commit ab17c4f02156c4f7 ("reiserfs: fixup xattr_root caching") is
> assuming that REISERFS_SB(s)->xattr_root != NULL in
> reiserfs_xattr_jcreate_nblocks() despite that commit made
> REISERFS_SB(sb)->priv_root != NULL && REISERFS_SB(s)->xattr_root == NULL
> case possible.
> 
> I guess that commit 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating
> privroot with selinux enabled") wanted to check xattr_root != NULL before
> reiserfs_xattr_jcreate_nblocks(), for the changelog is talking about the
> xattr root.
> 
>  The issue is that while creating the privroot during mount
>  reiserfs_security_init calls reiserfs_xattr_jcreate_nblocks which
>  dereferences the xattr root.  The xattr root doesn't exist, so we get an
>  oops.
> 
> Therefore, update reiserfs_xattrs_initialized() to check both the privroot
> and the xattr root.
> 
> [1] https://syzkaller.appspot.com/bug?id=8abaedbdeb32c861dc5340544284167dd0e46cde
> 
> Reported-and-tested-by: syzbot <syzbot+690cb1e51970435f9775@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: 6cb4aff0a77cc0e6 ("reiserfs: fix oops while creating privroot with selinux enabled")

Thanks for the patch Tetsuo! I'd prefer if Jeff had a look since he has
written this code back then. But let me provide my view: I agree that for a
corrupted filesystem it can happen that xattr_root remains NULL although
priv_root is set. So your change makes sense. But then
reiserfs_xattrs_initialized() seems to be used really minimally? Only once
in fs/reiserfs/xattr_security.c and e.g. reiserfs_xattr_set() is prone to
the same problem? Do I miss something?

								Honza

> diff --git a/fs/reiserfs/xattr.h b/fs/reiserfs/xattr.h
> index c764352447ba..81bec2c80b25 100644
> --- a/fs/reiserfs/xattr.h
> +++ b/fs/reiserfs/xattr.h
> @@ -43,7 +43,7 @@ void reiserfs_security_free(struct reiserfs_security_handle *sec);
>  
>  static inline int reiserfs_xattrs_initialized(struct super_block *sb)
>  {
> -	return REISERFS_SB(sb)->priv_root != NULL;
> +	return REISERFS_SB(sb)->priv_root && REISERFS_SB(sb)->xattr_root;
>  }
>  
>  #define xattr_size(size) ((size) + sizeof(struct reiserfs_xattr_header))
> -- 
> 2.18.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] reiserfs: update reiserfs_xattrs_initialized() condition

[Jan Kara]

On Sun 21-03-21 12:20:21, Linus Torvalds wrote:
> On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >
> > syzbot is reporting NULL pointer dereference at reiserfs_security_init()
> 
> Whee. Both of the mentioned commits go back over a decade.
> 
> I guess I could just take this directly, but let's add Jeff Mahoney
> and Jan Kara to the participants in case they didn't see it on the
> fsdevel list. I think they might want to be kept in the loop.
> 
> I'll forward the original in a separate email to them.
> 
> Jeff/Jan - just let me know if I should just apply this as-is.
> Otherwise I'd expect it to (eventually) come in through Jan's random
> fs tree, which is how I think most of these things have come in ..

Thanks Linus. I've replied to Tetsuo's patch. Honestly, I've seen the patch
when Tetsuo sent it but reiserfs fuzzing bugs are not high on my priority
list so I forgot about it before I found time to understand what's going on
there. I think his patch is safe but I'm not sure it is a complete solution
so let's wait a bit what he has to say.

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

Re: [PATCH (resend)] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

On 2021/03/23 0:31, Jan Kara wrote:
> Thanks for the patch Tetsuo! I'd prefer if Jeff had a look since he has
> written this code back then. But let me provide my view: I agree that for a
> corrupted filesystem it can happen that xattr_root remains NULL although
> priv_root is set. So your change makes sense. But then
> reiserfs_xattrs_initialized() seems to be used really minimally? Only once
> in fs/reiserfs/xattr_security.c and e.g. reiserfs_xattr_set() is prone to
> the same problem? Do I miss something?

As far as tested with assertion patch
( https://syzkaller.appspot.com/text?tag=Patch&x=13186fe6d00000 ) applied,
syzbot did not trigger the BUG_ON() added by this patch, which means that
reiserfs_fill_super() always fails if reiserfs_xattrs_initialized() returned false.

And console log ( https://syzkaller.appspot.com/text?tag=CrashLog&x=177b30bad00000 ) contains

  jdm-20006 create_privroot: xattrs/ACLs enabled and couldn't find/create .reiserfs_priv. Failing mount.

messages, which means that e.g. reiserfs_xattr_set() will not be called on
this corrupted filesystem image because mount operation itself fails.

Despite there are other bugs remaining, I think that applying this patch as-is is OK.

Re: [PATCH (resend)] reiserfs: update reiserfs_xattrs_initialized() condition

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 1748 bytes --]

On 3/24/21 10:47 AM, Tetsuo Handa wrote:
> On 2021/03/23 0:31, Jan Kara wrote:
>> Thanks for the patch Tetsuo! I'd prefer if Jeff had a look since he has
>> written this code back then. But let me provide my view: I agree that for a
>> corrupted filesystem it can happen that xattr_root remains NULL although
>> priv_root is set. So your change makes sense. But then
>> reiserfs_xattrs_initialized() seems to be used really minimally? Only once
>> in fs/reiserfs/xattr_security.c and e.g. reiserfs_xattr_set() is prone to
>> the same problem? Do I miss something?
> 
> As far as tested with assertion patch
> ( https://syzkaller.appspot.com/text?tag=Patch&x=13186fe6d00000 ) applied,
> syzbot did not trigger the BUG_ON() added by this patch, which means that
> reiserfs_fill_super() always fails if reiserfs_xattrs_initialized() returned false.
> 
> And console log ( https://syzkaller.appspot.com/text?tag=CrashLog&x=177b30bad00000 ) contains
> 
>   jdm-20006 create_privroot: xattrs/ACLs enabled and couldn't find/create .reiserfs_priv. Failing mount.
> 
> messages, which means that e.g. reiserfs_xattr_set() will not be called on
> this corrupted filesystem image because mount operation itself fails.
> 
> Despite there are other bugs remaining, I think that applying this patch as-is is OK.
> 

Ever wish you had a time machine and could go back and prevent your
former self from making a mistake?  The reiserfs xattr code would be my
first destination.

Tetsuo's patch is fine but it needs a similar fix in reiserfs_xattr_set,
as you noted.  Whether it's required is another question.  ReiserFS is
absolutely loaded with fuzzer bugs.

-Jeff


-- 
Jeff Mahoney
Director, SUSE Labs Data & Performance


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH (resend)] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

On 2021/03/25 0:20, Jeff Mahoney wrote:
> Tetsuo's patch is fine but it needs a similar fix in reiserfs_xattr_set,
> as you noted.  Whether it's required is another question.  ReiserFS is
> absolutely loaded with fuzzer bugs.

Can we apply this patch as-is? Since this is currently 5th top crasher,
applying this patch as soon as possible helps utilizing syzbot's resource
for finding further bugs.

Re: [PATCH (resend)] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

On 2021/03/25 15:19, Tetsuo Handa wrote:
> On 2021/03/25 0:20, Jeff Mahoney wrote:
>> Tetsuo's patch is fine but it needs a similar fix in reiserfs_xattr_set,
>> as you noted.  Whether it's required is another question.  ReiserFS is
>> absolutely loaded with fuzzer bugs.
> 
> Can we apply this patch as-is? Since this is currently 5th top crasher,
> applying this patch as soon as possible helps utilizing syzbot's resource
> for finding further bugs.
> 

Will you explain why we need a similar fix in reiserfs_xattr_set() ?

Debug print patch ( https://syzkaller.appspot.com/x/patch.diff?x=1112d621d00000 ) and
console output ( https://syzkaller.appspot.com/x/log.txt?x=13e76921d00000 ) says
"reiserfs_xattr_init returns -95" which indicates that reiserfs_fill_super() from
mount attempts for such crafted filesystem images fails with -EOPNOTSUPP error.

Given that such crafted filesystem images cannot be mounted,
how can reiserfs_xattr_set() be called and cause problems?

Re: [PATCH] reiserfs: update reiserfs_xattrs_initialized() condition

[Tetsuo Handa]

On 2021/03/22 4:20, Linus Torvalds wrote:
> On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> syzbot is reporting NULL pointer dereference at reiserfs_security_init()
> 
> Whee. Both of the mentioned commits go back over a decade.
> 
> I guess I could just take this directly, but let's add Jeff Mahoney
> and Jan Kara to the participants in case they didn't see it on the
> fsdevel list. I think they might want to be kept in the loop.
> 
> I'll forward the original in a separate email to them.
> 
> Jeff/Jan - just let me know if I should just apply this as-is.
> Otherwise I'd expect it to (eventually) come in through Jan's random
> fs tree, which is how I think most of these things have come in ..
> 

Linus, please just apply this as-is.

Jan says "your change makes sense" at https://lkml.kernel.org/m/20210322153142.GF31783@quack2.suse.cz
and Jeff says "Tetsuo's patch is fine" at https://lkml.kernel.org/m/7d7a884a-5a94-5b0e-3cf5-82d12e1b0992@suse.com
and I'm waiting for Jan/Jeff's reply to "why you think that my patch is incomplete" at
https://lkml.kernel.org/m/fa9f373a-a878-6551-abf1-903865a9d21f@i-love.sakura.ne.jp .
Since Jan/Jeff seems to be busy, applying as-is will let syzkaller answer to my question.

Re: [PATCH] reiserfs: update reiserfs_xattrs_initialized() condition

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 1608 bytes --]

On 3/30/21 10:47 AM, Tetsuo Handa wrote:
> On 2021/03/22 4:20, Linus Torvalds wrote:
>> On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>
>>> syzbot is reporting NULL pointer dereference at reiserfs_security_init()
>>
>> Whee. Both of the mentioned commits go back over a decade.
>>
>> I guess I could just take this directly, but let's add Jeff Mahoney
>> and Jan Kara to the participants in case they didn't see it on the
>> fsdevel list. I think they might want to be kept in the loop.
>>
>> I'll forward the original in a separate email to them.
>>
>> Jeff/Jan - just let me know if I should just apply this as-is.
>> Otherwise I'd expect it to (eventually) come in through Jan's random
>> fs tree, which is how I think most of these things have come in ..
>>
> 
> Linus, please just apply this as-is.
> 
> Jan says "your change makes sense" at https://lkml.kernel.org/m/20210322153142.GF31783@quack2.suse.cz
> and Jeff says "Tetsuo's patch is fine" at https://lkml.kernel.org/m/7d7a884a-5a94-5b0e-3cf5-82d12e1b0992@suse.com
> and I'm waiting for Jan/Jeff's reply to "why you think that my patch is incomplete" at
> https://lkml.kernel.org/m/fa9f373a-a878-6551-abf1-903865a9d21f@i-love.sakura.ne.jp .
> Since Jan/Jeff seems to be busy, applying as-is will let syzkaller answer to my question.

Hi Tetsuo -

You're right.  The other call site in reiserfs_xattr_set is fine because
of the privroot check before it.

The patch is fine as-is.

Thanks,

-Jeff


-- 
Jeff Mahoney
Director, SUSE Labs Data & Performance


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] reiserfs: update reiserfs_xattrs_initialized() condition

[Jan Kara]

On Tue 30-03-21 23:47:11, Tetsuo Handa wrote:
> On 2021/03/22 4:20, Linus Torvalds wrote:
> > On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
> > <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >>
> >> syzbot is reporting NULL pointer dereference at reiserfs_security_init()
> > 
> > Whee. Both of the mentioned commits go back over a decade.
> > 
> > I guess I could just take this directly, but let's add Jeff Mahoney
> > and Jan Kara to the participants in case they didn't see it on the
> > fsdevel list. I think they might want to be kept in the loop.
> > 
> > I'll forward the original in a separate email to them.
> > 
> > Jeff/Jan - just let me know if I should just apply this as-is.
> > Otherwise I'd expect it to (eventually) come in through Jan's random
> > fs tree, which is how I think most of these things have come in ..
> > 
> 
> Linus, please just apply this as-is.

Yes, feel free to. I just wanted to do that today and send you the pull
request anyway.

								Honza


> 
> Jan says "your change makes sense" at https://lkml.kernel.org/m/20210322153142.GF31783@quack2.suse.cz
> and Jeff says "Tetsuo's patch is fine" at https://lkml.kernel.org/m/7d7a884a-5a94-5b0e-3cf5-82d12e1b0992@suse.com
> and I'm waiting for Jan/Jeff's reply to "why you think that my patch is incomplete" at
> https://lkml.kernel.org/m/fa9f373a-a878-6551-abf1-903865a9d21f@i-love.sakura.ne.jp .
> Since Jan/Jeff seems to be busy, applying as-is will let syzkaller answer to my question.
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR