linux-kernel - Re: [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAC+yH-Z3243vRB6Nr7Y1FrfsQcmyjM0qH8D1FKSzSPEi=xa0uw@mail.gmail.com>
Date:   Thu, 1 Oct 2020 16:14:23 +0530
From:   Anmol karn <anmol.karan123@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     jeffm@...e.com, open list <linux-kernel@...r.kernel.org>,
        reiserfs-devel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        syzkaller-bugs@...glegroups.com,
        syzbot+9b33c9b118d77ff59b6f@...kaller.appspotmail.com
Subject: Re: [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null
 pointer derefernce in open_xa_root()

On Thu, Oct 1, 2020 at 2:58 PM Jan Kara <jack@...e.cz> wrote:
>
> On Thu 01-10-20 14:35:47, Anmol Karn wrote:
> > d_really_is_negative() checks for the dentry->d_inode whether it's NULL
> > or not, but in open_xa_root(), when it checks 'privroot->d_inode', it
> > doesn't check whether privroot is NULL or not, this leads to a null
> > pointer dereference while calling it from open_xa_dir() while
> > initializing xaroot.
> >
> > - fs/reiserfs/xattr.c
> > The bug seems to get triggered at this line:
> >
> > if (d_really_is_negative(privroot))
> >               return ERR_PTR(-EOPNOTSUPP);
> >
> > Fix it by adding a NULL check for privroot.
> >
> > Reported-and-tested-by: syzbot+9b33c9b118d77ff59b6f@...kaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=9b33c9b118d77ff59b6f
> > Signed-off-by: Anmol Karn <anmol.karan123@...il.com>
>
> Thanks for the patch! I've already fixed the problem myself (slightly
> differently) but I'll comment about your patch below for educational
> purposes :). See
> https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git/commit/?h=for_next&id=c2bb80b8bdd04dfe32364b78b61b6a47f717af52

Ah, no worries, I am glad that, the bug is fixed :).
I will mark this as fixed on syzbot.

>
> > diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
> > index 28b241cd6987..a75480d0ee7e 100644
> > --- a/fs/reiserfs/xattr.c
> > +++ b/fs/reiserfs/xattr.c
> > @@ -121,8 +121,9 @@ static struct dentry *open_xa_root(struct super_block *sb, int flags)
> >       struct dentry *privroot = REISERFS_SB(sb)->priv_root;
> >       struct dentry *xaroot;
> >
> > -     if (d_really_is_negative(privroot))
> > +     if (!privroot || d_really_is_negative(privroot)) {
> >               return ERR_PTR(-EOPNOTSUPP);
>
> I don't think EOPNOTSUPP is correct return code for !privroot case. AFAICS
> it would propagate out of reiserfs xattr code and would result in denying
> access to lookup_one_len() so xattr dir could never be initialized for such
> filesystem. So we need to return 0 (success, no xattrs present) in this
> case and because this is just a special case when we are initializing xattr
> dir and recurse back into xattr code, I've decided to perform this check
> directly in reiserfs_xattr_get().

Thanks for the review and information, sir.

>
> > +     }
>
> There's no need for additional braces in this 'if'.
> >
> >       inode_lock_nested(d_inode(privroot), I_MUTEX_XATTR);
>
>                                                                 Honza
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR

Anmol