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

Re: general protection fault in reiserfs_security_init

[慕冬亮]

On Monday, September 21, 2020 at 5:32:22 PM UTC+8 syzbot wrote:
>
> 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+690cb1...@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


int reiserfs_xattr_init(struct super_block *s, int mount_flags)
{
        int err = 0;
        struct dentry *privroot = REISERFS_SB(s)->priv_root;

        err = xattr_mount_check(s);
        if (err)
                goto error;

        if (d_really_is_negative(privroot) && !(mount_flags & SB_RDONLY)) {
                inode_lock(d_inode(s->s_root));
                err = create_privroot(REISERFS_SB(s)->priv_root);
                inode_unlock(d_inode(s->s_root));
        }

        if (d_really_is_positive(privroot)) {
                inode_lock(d_inode(privroot));
                if (!REISERFS_SB(s)->xattr_root) {
                        struct dentry *dentry;

                        dentry = lookup_one_len(XAROOT_NAME, privroot,
                                                strlen(XAROOT_NAME));
                        if (!IS_ERR(dentry)) {
                                pr_alert("assign xattr_root with
dentry = 0x%lx", dentry);
                                REISERFS_SB(s)->xattr_root = dentry;
                        }else
                                err = PTR_ERR(dentry);
                }
                inode_unlock(d_inode(privroot));
        }
        ......
}
From the implementation of reiserfs_xattr_init, only when
d_really_is_positive(privroot) is true, xattr_root could be assigned
with a dentry obtained from lookup_one_len. In other words,
create_privroot is executed with REISERFS_SB(s)->xattr_root as NULL
pointer. With improper implementation of mkdir operation in reiserfs
filesystem or accessing the xattr_root in reiserfs_mkdir , it can lead
to NULL pointer dereference. If you remove the red code in
reiserfs_xattr_jcreate_nblocks, the crash never occurs, but it may
affect nblocks calculation in the reiserfs filesystem.

static inline size_t reiserfs_xattr_jcreate_nblocks(struct inode *inode)
{
        size_t nblocks = JOURNAL_BLOCKS_PER_OBJECT(inode->i_sb);

        pr_alert("5: inode = 0x%lx", inode);
        if ((REISERFS_I(inode)->i_flags & i_has_xattr_dir) == 0) {
                nblocks += JOURNAL_BLOCKS_PER_OBJECT(inode->i_sb);
                if (d_really_is_negative(REISERFS_SB(inode->i_sb)->xattr_root))
                      nblocks += JOURNAL_BLOCKS_PER_OBJECT(inode->i_sb);
        }

        return nblocks;
}

--
My best regards to you.

     No System Is Safe!
     Dongliang Mu

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

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