* [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa_root()
@ 2020-10-01 9:05 ` Anmol Karn
0 siblings, 0 replies; 6+ messages in thread
From: Anmol Karn @ 2020-10-01 9:05 UTC (permalink / raw)
To: jack, jeffm
Cc: linux-kernel, reiserfs-devel, linux-kernel-mentees,
syzkaller-bugs, anmol.karan123, syzbot+9b33c9b118d77ff59b6f
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@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=9b33c9b118d77ff59b6f
Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
---
fs/reiserfs/xattr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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);
+ }
inode_lock_nested(d_inode(privroot), I_MUTEX_XATTR);
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa_root()
@ 2020-10-01 9:05 ` Anmol Karn
0 siblings, 0 replies; 6+ messages in thread
From: Anmol Karn @ 2020-10-01 9:05 UTC (permalink / raw)
To: jack, jeffm
Cc: syzkaller-bugs, syzbot+9b33c9b118d77ff59b6f, linux-kernel,
reiserfs-devel, linux-kernel-mentees
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@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=9b33c9b118d77ff59b6f
Signed-off-by: Anmol Karn <anmol.karan123@gmail.com>
---
fs/reiserfs/xattr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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);
+ }
inode_lock_nested(d_inode(privroot), I_MUTEX_XATTR);
--
2.28.0
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa_root()
2020-10-01 9:05 ` Anmol Karn
@ 2020-10-01 9:28 ` Jan Kara
-1 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2020-10-01 9:28 UTC (permalink / raw)
To: Anmol Karn
Cc: jack, jeffm, linux-kernel, reiserfs-devel, linux-kernel-mentees,
syzkaller-bugs, syzbot+9b33c9b118d77ff59b6f
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@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=9b33c9b118d77ff59b6f
> Signed-off-by: Anmol Karn <anmol.karan123@gmail.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
> 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().
> + }
There's no need for additional braces in this 'if'.
>
> inode_lock_nested(d_inode(privroot), I_MUTEX_XATTR);
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa_root()
@ 2020-10-01 9:28 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2020-10-01 9:28 UTC (permalink / raw)
To: Anmol Karn
Cc: jack, syzbot+9b33c9b118d77ff59b6f, jeffm, syzkaller-bugs,
linux-kernel, reiserfs-devel, linux-kernel-mentees
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@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=9b33c9b118d77ff59b6f
> Signed-off-by: Anmol Karn <anmol.karan123@gmail.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
> 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().
> + }
There's no need for additional braces in this 'if'.
>
> inode_lock_nested(d_inode(privroot), I_MUTEX_XATTR);
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa_root()
2020-10-01 9:28 ` Jan Kara
@ 2020-10-01 10:44 ` Anmol karn
-1 siblings, 0 replies; 6+ messages in thread
From: Anmol karn @ 2020-10-01 10:44 UTC (permalink / raw)
To: Jan Kara
Cc: jeffm, open list, reiserfs-devel, linux-kernel-mentees,
syzkaller-bugs, syzbot+9b33c9b118d77ff59b6f
On Thu, Oct 1, 2020 at 2:58 PM Jan Kara <jack@suse.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@syzkaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=9b33c9b118d77ff59b6f
> > Signed-off-by: Anmol Karn <anmol.karan123@gmail.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@suse.com>
> SUSE Labs, CR
Anmol
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa_root()
@ 2020-10-01 10:44 ` Anmol karn
0 siblings, 0 replies; 6+ messages in thread
From: Anmol karn @ 2020-10-01 10:44 UTC (permalink / raw)
To: Jan Kara
Cc: syzkaller-bugs, jeffm, syzbot+9b33c9b118d77ff59b6f, open list,
reiserfs-devel, linux-kernel-mentees
On Thu, Oct 1, 2020 at 2:58 PM Jan Kara <jack@suse.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@syzkaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=9b33c9b118d77ff59b6f
> > Signed-off-by: Anmol Karn <anmol.karan123@gmail.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@suse.com>
> SUSE Labs, CR
Anmol
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-01 10:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01 9:05 [Linux-kernel-mentees] [PATCH] fs: reiserfs: xattr: Fix null pointer derefernce in open_xa_root() Anmol Karn
2020-10-01 9:05 ` Anmol Karn
2020-10-01 9:28 ` Jan Kara
2020-10-01 9:28 ` Jan Kara
2020-10-01 10:44 ` Anmol karn
2020-10-01 10:44 ` Anmol karn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.