On Fri, 2023-06-02 at 09:20 +0200, Roberto Sassu wrote:
> On Thu, 2023-06-01 at 17:22 -0400, Jeff Mahoney wrote:
> > On 5/31/23 05:49, Roberto Sassu wrote:
> > > On 5/5/2023 11:36 PM, Paul Moore wrote:
> > > > On Fri, May 5, 2023 at 4:51 PM syzbot
> > > > <syzbot+8fb64a61fdd96b50f3b8@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > > > syzbot has bisected this issue to:
> > > > >
> > > > > commit d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > > > > Author: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > > Date: Fri Mar 31 12:32:18 2023 +0000
> > > > >
> > > > > reiserfs: Add security prefix to xattr name in
> > > > > reiserfs_security_write()
> > > > >
> > > > > bisection log:
> > > > > https://syzkaller.appspot.com/x/bisect.txt?x=14403182280000
> > > > > start commit: 3c4aa4434377 Merge tag 'ceph-for-6.4-rc1' of
> > > > > https://githu..
> > > > > git tree: upstream
> > > > > final oops:
> > > > > https://syzkaller.appspot.com/x/report.txt?x=16403182280000
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12403182280000
> > > > > kernel config:
> > > > > https://syzkaller.appspot.com/x/.config?x=73a06f6ef2d5b492
> > > > > dashboard link:
> > > > > https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
> > > > > syz repro:
> > > > > https://syzkaller.appspot.com/x/repro.syz?x=12442414280000
> > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=176a7318280000
> > > > >
> > > > > Reported-by: syzbot+8fb64a61fdd96b50f3b8@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > > Fixes: d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in
> > > > > reiserfs_security_write()")
> > > > >
> > > > > For information about bisection process see:
> > > > > https://goo.gl/tpsmEJ#bisection
> > > >
> > > > I don't think Roberto's patch identified above is the actual root
> > > > cause of this problem as reiserfs_xattr_set_handle() is called in
> > > > reiserfs_security_write() both before and after the patch. However,
> > > > due to some bad logic in reiserfs_security_write() which Roberto
> > > > corrected, I'm thinking that it is possible this code is being
> > > > exercised for the first time and syzbot is starting to trigger a
> > > > locking issue in the reiserfs code ... ?
> > >
> > > + Jan, Jeff (which basically restructured the lock)
> > >
> > > + Petr, Ingo, Will
>
> Peter, clearly (sorry!)
>
> > I involve the lockdep experts, to get a bit of help on this.
> >
> > Yep, looks like that's been broken since it was added in 2009. Since
> > there can't be any users of it, it'd make sense to drop the security
> > xattr support from reiserfs entirely.
>
> Thanks, Jeff. Will make a patch to implement your suggestion.
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git next
From a31f5b09e39e5a964457b0a52aa9c437a0bf7f63 Mon Sep 17 00:00:00 2001
From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
Date: Fri, 2 Jun 2023 10:10:28 +0200
Subject: [PATCH] reiserfs: Disable security xattr initialization since it
never worked
Commit d82dcd9e21b7 ("reiserfs: Add security prefix to xattr name in
reiserfs_security_write()"), while fixed the security xattr initialization,
it also revealed a circular locking dependency between the reiserfs write
lock and the inode lock.
Since the bug in security xattr initialization was introduced since the
beginning, there cannot be any user of this feature. Instead of trying to
fix the locking dependency, which was already challenging to convert from
BLK, just disable the feature.
However, still keep the security xattr handler, since it was introduced
earlier, and users might have manually added xattrs.
Reported-by: syzbot+8fb64a61fdd96b50f3b8@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=8fb64a61fdd96b50f3b8
Suggested-by: Jeff Mahoney <jeffm@xxxxxxxx>
Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
---
fs/reiserfs/super.c | 2 ++
fs/reiserfs/xattr_security.c | 3 +++
2 files changed, 5 insertions(+)
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 929acce6e73..262041b87cd 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -1654,6 +1654,8 @@ static int read_super_block(struct super_block *s, int offset)
reiserfs_warning(NULL, "", "reiserfs filesystem is deprecated and "
"scheduled to be removed from the kernel in 2025");
+ reiserfs_warning(NULL, "", "initializing security xattrs never worked, disable it");
+
SB_BUFFER_WITH_SB(s) = bh;
SB_DISK_SUPER_BLOCK(s) = rs;
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
index 078dd8cc312..4037a998dbf 100644
--- a/fs/reiserfs/xattr_security.c
+++ b/fs/reiserfs/xattr_security.c
@@ -69,6 +69,9 @@ int reiserfs_security_init(struct inode *dir, struct inode *inode,
sec->value = NULL;
sec->length = 0;
+ /* See warning in read_super_block(). */
+ return 0;
+
/* Don't add selinux attributes on xattrs - they'll never get used */
if (IS_PRIVATE(dir))
return 0;
--
2.25.1
