reiserfs: update reiserfs_xattrs_initialized() condition

Commit Message

Tetsuo Handa March 21, 2021, 2:37 p.m. UTC

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 <[e-mail]>
Signed-off-by: Tetsuo Handa <[e-mail]>
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 [e-mail] to [e-mail] , for
no response from reiserfs developers for one month. If still no response from fsdevel
people, I would have to directly send to Linus...

Comments

Linus Torvalds March 21, 2021, 7:20 p.m. UTC | #1

On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
<[e-mail]> 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

Jan Kara March 22, 2021, 3:43 p.m. UTC | #2

On Sun 21-03-21 12:20:21, Linus Torvalds wrote:
> On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
> <[e-mail]> 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

Tetsuo Handa March 30, 2021, 2:47 p.m. UTC | #3

On 2021/03/22 4:20, Linus Torvalds wrote:
> On Sun, Mar 21, 2021 at 7:37 AM Tetsuo Handa
> <[e-mail]> 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/[e-mail]
and Jeff says "Tetsuo's patch is fine" at https://lkml.kernel.org/m/[e-mail]
and I'm waiting for Jan/Jeff's reply to "why you think that my patch is incomplete" at
https://lkml.kernel.org/m/[e-mail] .
Since Jan/Jeff seems to be busy, applying as-is will let syzkaller answer to my question.

Jeff Mahoney March 30, 2021, 2:51 p.m. UTC | #4

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
>> <[e-mail]> 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/[e-mail]
> and Jeff says "Tetsuo's patch is fine" at https://lkml.kernel.org/m/[e-mail]
> and I'm waiting for Jan/Jeff's reply to "why you think that my patch is incomplete" at
> https://lkml.kernel.org/m/[e-mail] .
> 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

Jan Kara March 30, 2021, 2:53 p.m. UTC | #5

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
> > <[e-mail]> 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/[e-mail]
> and Jeff says "Tetsuo's patch is fine" at https://lkml.kernel.org/m/[e-mail]
> and I'm waiting for Jan/Jeff's reply to "why you think that my patch is incomplete" at
> https://lkml.kernel.org/m/[e-mail] .
> Since Jan/Jeff seems to be busy, applying as-is will let syzkaller answer to my question.

Patch

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