[PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

The KMSAN bug report for the bug indicates that there exists;
Local variable ----nd@do_file_open_root created at:
 do_file_open_root+0xa4/0xb40 fs/namei.c:3385
 do_file_open_root+0xa4/0xb40 fs/namei.c:3385

Initializing nd fixes this issue, and doesn't break anything else either

Fixes: https://syzkaller.appspot.com/bug?extid=4191a44ad556eacc1a7a
Reported-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
Tested-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
 fs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index e99e2a9da0f7..b27382586209 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3404,7 +3404,7 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
 struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
 		const char *name, const struct open_flags *op)
 {
-	struct nameidata nd;
+	struct nameidata nd = {};
 	struct file *file;
 	struct filename *filename;
 	int flags = op->lookup_flags | LOOKUP_ROOT;
-- 
2.25.1

[Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

The KMSAN bug report for the bug indicates that there exists;
Local variable ----nd@do_file_open_root created at:
 do_file_open_root+0xa4/0xb40 fs/namei.c:3385
 do_file_open_root+0xa4/0xb40 fs/namei.c:3385

Initializing nd fixes this issue, and doesn't break anything else either

Fixes: https://syzkaller.appspot.com/bug?extid=4191a44ad556eacc1a7a
Reported-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
Tested-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
 fs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index e99e2a9da0f7..b27382586209 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3404,7 +3404,7 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
 struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
 		const char *name, const struct open_flags *op)
 {
-	struct nameidata nd;
+	struct nameidata nd = {};
 	struct file *file;
 	struct filename *filename;
 	int flags = op->lookup_flags | LOOKUP_ROOT;
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Eric Biggers]

On Wed, Sep 16, 2020 at 10:56:56AM +0530, Anant Thazhemadam wrote:
> The KMSAN bug report for the bug indicates that there exists;
> Local variable ----nd@do_file_open_root created at:
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385
> 
> Initializing nd fixes this issue, and doesn't break anything else either
> 
> Fixes: https://syzkaller.appspot.com/bug?extid=4191a44ad556eacc1a7a
> Reported-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Tested-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
>  fs/namei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index e99e2a9da0f7..b27382586209 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3404,7 +3404,7 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
>  struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
>  		const char *name, const struct open_flags *op)
>  {
> -	struct nameidata nd;
> +	struct nameidata nd = {};
>  	struct file *file;
>  	struct filename *filename;
>  	int flags = op->lookup_flags | LOOKUP_ROOT;

Looking at the actual KMSAN report, it looks like it's nameidata::dir_mode or
nameidata::dir_uid that is uninitialized.  You need to figure out the correct
solution, not just blindly initialize with zeroes -- that could hide a bug.
Is there a bug that is preventing these fields from being initialized to the
correct values, are these fields being used when they shouldn't be, etc...

- Eric

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Eric Biggers]

On Wed, Sep 16, 2020 at 10:56:56AM +0530, Anant Thazhemadam wrote:
> The KMSAN bug report for the bug indicates that there exists;
> Local variable ----nd@do_file_open_root created at:
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385
> 
> Initializing nd fixes this issue, and doesn't break anything else either
> 
> Fixes: https://syzkaller.appspot.com/bug?extid=4191a44ad556eacc1a7a
> Reported-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Tested-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
>  fs/namei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index e99e2a9da0f7..b27382586209 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3404,7 +3404,7 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
>  struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
>  		const char *name, const struct open_flags *op)
>  {
> -	struct nameidata nd;
> +	struct nameidata nd = {};
>  	struct file *file;
>  	struct filename *filename;
>  	int flags = op->lookup_flags | LOOKUP_ROOT;

Looking at the actual KMSAN report, it looks like it's nameidata::dir_mode or
nameidata::dir_uid that is uninitialized.  You need to figure out the correct
solution, not just blindly initialize with zeroes -- that could hide a bug.
Is there a bug that is preventing these fields from being initialized to the
correct values, are these fields being used when they shouldn't be, etc...

- Eric
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Greg KH]

On Wed, Sep 16, 2020 at 10:56:56AM +0530, Anant Thazhemadam wrote:
> The KMSAN bug report for the bug indicates that there exists;
> Local variable ----nd@do_file_open_root created at:
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385

What does this "error" mean?

> Initializing nd fixes this issue, and doesn't break anything else either
> 
> Fixes: https://syzkaller.appspot.com/bug?extid=4191a44ad556eacc1a7a
> Reported-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Tested-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
>  fs/namei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index e99e2a9da0f7..b27382586209 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3404,7 +3404,7 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
>  struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
>  		const char *name, const struct open_flags *op)
>  {
> -	struct nameidata nd;
> +	struct nameidata nd = {};

What exactly does setting this structure to all 0 fix here that is
currently "broken"?

confused,

greg k-h

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Greg KH]

On Wed, Sep 16, 2020 at 10:56:56AM +0530, Anant Thazhemadam wrote:
> The KMSAN bug report for the bug indicates that there exists;
> Local variable ----nd@do_file_open_root created at:
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385
>  do_file_open_root+0xa4/0xb40 fs/namei.c:3385

What does this "error" mean?

> Initializing nd fixes this issue, and doesn't break anything else either
> 
> Fixes: https://syzkaller.appspot.com/bug?extid=4191a44ad556eacc1a7a
> Reported-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Tested-by: syzbot+4191a44ad556eacc1a7a@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
>  fs/namei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index e99e2a9da0f7..b27382586209 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3404,7 +3404,7 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
>  struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
>  		const char *name, const struct open_flags *op)
>  {
> -	struct nameidata nd;
> +	struct nameidata nd = {};

What exactly does setting this structure to all 0 fix here that is
currently "broken"?

confused,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Al Viro]

On Tue, Sep 15, 2020 at 10:41:57PM -0700, Eric Biggers wrote:

> Looking at the actual KMSAN report, it looks like it's nameidata::dir_mode or
> nameidata::dir_uid that is uninitialized.  You need to figure out the correct
> solution, not just blindly initialize with zeroes -- that could hide a bug.
> Is there a bug that is preventing these fields from being initialized to the
> correct values, are these fields being used when they shouldn't be, etc...

False positive, and this is the wrong place to shut it up.

->dir_uid and ->dir_mode are set when link_path_walk() resolves the pathname
to directory + final component.  They are used when deciding whether to reject
a trailing symlink (on fs.protected_symlinks setups) and whether to allow
creation in sticky directories (on fs.protected_regular and fs.protected_fifos
setups).  Both operations really need the results of successful link_path_walk().

I don't see how that could be not a false positive.  If we hit the use in
may_create_in_sticky(), we'd need the combination of
	* pathname that consists only of slashes (or it will be initialized)
	* LAST_NORM in nd->last_type, which is flat-out impossible, since
we are left with LAST_ROOT for such pathnames.  The same goes for
may_follow_link() use - we need WALK_TRAILING in flags to hit it in the
first place, which can come from two sources -
        return walk_component(nd, WALK_TRAILING);
in lookup_last() (and walk_component() won't go anywhere near the
call chain leading to may_follow_link() without LAST_NORM in nd->last_type)
and
        res = step_into(nd, WALK_TRAILING, dentry, inode, seq);
in open_last_lookups(), which also won't go anywhere near that line without
LAST_NORM in the nd->last_type.

IOW, unless we manage to call that without having called link_path_walk()
at all or after link_path_walk() returning an error, we shouldn't hit
that.  And if we *do* go there without link_path_walk() or with an error
from link_path_walk(), we have a much worse problem.

I want to see the details of reproducer.  If it's for real, we have a much
more serious problem; if it's a false positive, the right place to deal
with it would be elsewhere (perhaps on return from link_path_walk() with
a slashes-only pathname), but in any case it should only be done after we
manage to understand what's going on.

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Al Viro]

On Tue, Sep 15, 2020 at 10:41:57PM -0700, Eric Biggers wrote:

> Looking at the actual KMSAN report, it looks like it's nameidata::dir_mode or
> nameidata::dir_uid that is uninitialized.  You need to figure out the correct
> solution, not just blindly initialize with zeroes -- that could hide a bug.
> Is there a bug that is preventing these fields from being initialized to the
> correct values, are these fields being used when they shouldn't be, etc...

False positive, and this is the wrong place to shut it up.

->dir_uid and ->dir_mode are set when link_path_walk() resolves the pathname
to directory + final component.  They are used when deciding whether to reject
a trailing symlink (on fs.protected_symlinks setups) and whether to allow
creation in sticky directories (on fs.protected_regular and fs.protected_fifos
setups).  Both operations really need the results of successful link_path_walk().

I don't see how that could be not a false positive.  If we hit the use in
may_create_in_sticky(), we'd need the combination of
	* pathname that consists only of slashes (or it will be initialized)
	* LAST_NORM in nd->last_type, which is flat-out impossible, since
we are left with LAST_ROOT for such pathnames.  The same goes for
may_follow_link() use - we need WALK_TRAILING in flags to hit it in the
first place, which can come from two sources -
        return walk_component(nd, WALK_TRAILING);
in lookup_last() (and walk_component() won't go anywhere near the
call chain leading to may_follow_link() without LAST_NORM in nd->last_type)
and
        res = step_into(nd, WALK_TRAILING, dentry, inode, seq);
in open_last_lookups(), which also won't go anywhere near that line without
LAST_NORM in the nd->last_type.

IOW, unless we manage to call that without having called link_path_walk()
at all or after link_path_walk() returning an error, we shouldn't hit
that.  And if we *do* go there without link_path_walk() or with an error
from link_path_walk(), we have a much worse problem.

I want to see the details of reproducer.  If it's for real, we have a much
more serious problem; if it's a false positive, the right place to deal
with it would be elsewhere (perhaps on return from link_path_walk() with
a slashes-only pathname), but in any case it should only be done after we
manage to understand what's going on.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

On 19-09-2020 22:25, Al Viro wrote:
> On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
>
>> Lovely...  That would get an empty path and non-directory for a starting
>> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
>> not be able to reach the readers of those fields...  Which kernel had
>> that been on?
> Yecchhh...  I see what's going on; I suspect that this ought to be enough.
> Folks, could somebody test it on the original reproducer setup?

Sure. I can do that.

Thanks,
Anant

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

On 19-09-2020 22:25, Al Viro wrote:
> On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
>
>> Lovely...  That would get an empty path and non-directory for a starting
>> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
>> not be able to reach the readers of those fields...  Which kernel had
>> that been on?
> Yecchhh...  I see what's going on; I suspect that this ought to be enough.
> Folks, could somebody test it on the original reproducer setup?

Sure. I can do that.

Thanks,
Anant
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Greg KH]

On Thu, Sep 17, 2020 at 01:22:38AM +0100, Al Viro wrote:
> On Tue, Sep 15, 2020 at 10:41:57PM -0700, Eric Biggers wrote:
> 
> > Looking at the actual KMSAN report, it looks like it's nameidata::dir_mode or
> > nameidata::dir_uid that is uninitialized.  You need to figure out the correct
> > solution, not just blindly initialize with zeroes -- that could hide a bug.
> > Is there a bug that is preventing these fields from being initialized to the
> > correct values, are these fields being used when they shouldn't be, etc...
> 
> False positive, and this is the wrong place to shut it up.
> 
> ->dir_uid and ->dir_mode are set when link_path_walk() resolves the pathname
> to directory + final component.  They are used when deciding whether to reject
> a trailing symlink (on fs.protected_symlinks setups) and whether to allow
> creation in sticky directories (on fs.protected_regular and fs.protected_fifos
> setups).  Both operations really need the results of successful link_path_walk().
> 
> I don't see how that could be not a false positive.  If we hit the use in
> may_create_in_sticky(), we'd need the combination of
> 	* pathname that consists only of slashes (or it will be initialized)
> 	* LAST_NORM in nd->last_type, which is flat-out impossible, since
> we are left with LAST_ROOT for such pathnames.  The same goes for
> may_follow_link() use - we need WALK_TRAILING in flags to hit it in the
> first place, which can come from two sources -
>         return walk_component(nd, WALK_TRAILING);
> in lookup_last() (and walk_component() won't go anywhere near the
> call chain leading to may_follow_link() without LAST_NORM in nd->last_type)
> and
>         res = step_into(nd, WALK_TRAILING, dentry, inode, seq);
> in open_last_lookups(), which also won't go anywhere near that line without
> LAST_NORM in the nd->last_type.
> 
> IOW, unless we manage to call that without having called link_path_walk()
> at all or after link_path_walk() returning an error, we shouldn't hit
> that.  And if we *do* go there without link_path_walk() or with an error
> from link_path_walk(), we have a much worse problem.
> 
> I want to see the details of reproducer.  If it's for real, we have a much
> more serious problem; if it's a false positive, the right place to deal
> with it would be elsewhere (perhaps on return from link_path_walk() with
> a slashes-only pathname), but in any case it should only be done after we
> manage to understand what's going on.

Reproducer is pretty simple:
	https://syzkaller.appspot.com/text?tag=ReproC&x=13974b2f100000

Now if that is actually valid or not, I don't know...

thanks,

greg k-h

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Greg KH]

On Thu, Sep 17, 2020 at 01:22:38AM +0100, Al Viro wrote:
> On Tue, Sep 15, 2020 at 10:41:57PM -0700, Eric Biggers wrote:
> 
> > Looking at the actual KMSAN report, it looks like it's nameidata::dir_mode or
> > nameidata::dir_uid that is uninitialized.  You need to figure out the correct
> > solution, not just blindly initialize with zeroes -- that could hide a bug.
> > Is there a bug that is preventing these fields from being initialized to the
> > correct values, are these fields being used when they shouldn't be, etc...
> 
> False positive, and this is the wrong place to shut it up.
> 
> ->dir_uid and ->dir_mode are set when link_path_walk() resolves the pathname
> to directory + final component.  They are used when deciding whether to reject
> a trailing symlink (on fs.protected_symlinks setups) and whether to allow
> creation in sticky directories (on fs.protected_regular and fs.protected_fifos
> setups).  Both operations really need the results of successful link_path_walk().
> 
> I don't see how that could be not a false positive.  If we hit the use in
> may_create_in_sticky(), we'd need the combination of
> 	* pathname that consists only of slashes (or it will be initialized)
> 	* LAST_NORM in nd->last_type, which is flat-out impossible, since
> we are left with LAST_ROOT for such pathnames.  The same goes for
> may_follow_link() use - we need WALK_TRAILING in flags to hit it in the
> first place, which can come from two sources -
>         return walk_component(nd, WALK_TRAILING);
> in lookup_last() (and walk_component() won't go anywhere near the
> call chain leading to may_follow_link() without LAST_NORM in nd->last_type)
> and
>         res = step_into(nd, WALK_TRAILING, dentry, inode, seq);
> in open_last_lookups(), which also won't go anywhere near that line without
> LAST_NORM in the nd->last_type.
> 
> IOW, unless we manage to call that without having called link_path_walk()
> at all or after link_path_walk() returning an error, we shouldn't hit
> that.  And if we *do* go there without link_path_walk() or with an error
> from link_path_walk(), we have a much worse problem.
> 
> I want to see the details of reproducer.  If it's for real, we have a much
> more serious problem; if it's a false positive, the right place to deal
> with it would be elsewhere (perhaps on return from link_path_walk() with
> a slashes-only pathname), but in any case it should only be done after we
> manage to understand what's going on.

Reproducer is pretty simple:
	https://syzkaller.appspot.com/text?tag=ReproC&x=13974b2f100000

Now if that is actually valid or not, I don't know...

thanks,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Al Viro]

On Sat, Sep 19, 2020 at 04:44:51PM +0200, Greg KH wrote:

> > ->dir_uid and ->dir_mode are set when link_path_walk() resolves the pathname
> > to directory + final component.  They are used when deciding whether to reject
> > a trailing symlink (on fs.protected_symlinks setups) and whether to allow
> > creation in sticky directories (on fs.protected_regular and fs.protected_fifos
> > setups).  Both operations really need the results of successful link_path_walk().
> > 
> > I don't see how that could be not a false positive.  If we hit the use in
> > may_create_in_sticky(), we'd need the combination of
> > 	* pathname that consists only of slashes (or it will be initialized)
> > 	* LAST_NORM in nd->last_type, which is flat-out impossible, since
> > we are left with LAST_ROOT for such pathnames.  The same goes for
> > may_follow_link() use - we need WALK_TRAILING in flags to hit it in the
> > first place, which can come from two sources -
> >         return walk_component(nd, WALK_TRAILING);
> > in lookup_last() (and walk_component() won't go anywhere near the
> > call chain leading to may_follow_link() without LAST_NORM in nd->last_type)
> > and
> >         res = step_into(nd, WALK_TRAILING, dentry, inode, seq);
> > in open_last_lookups(), which also won't go anywhere near that line without
> > LAST_NORM in the nd->last_type.
> > 
> > IOW, unless we manage to call that without having called link_path_walk()
> > at all or after link_path_walk() returning an error, we shouldn't hit
> > that.  And if we *do* go there without link_path_walk() or with an error
> > from link_path_walk(), we have a much worse problem.
> > 
> > I want to see the details of reproducer.  If it's for real, we have a much
> > more serious problem; if it's a false positive, the right place to deal
> > with it would be elsewhere (perhaps on return from link_path_walk() with
> > a slashes-only pathname), but in any case it should only be done after we
> > manage to understand what's going on.
> 
> Reproducer is pretty simple:
> 	https://syzkaller.appspot.com/text?tag=ReproC&x=13974b2f100000
> 
> Now if that is actually valid or not, I don't know...

Lovely...  That would get an empty path and non-directory for a starting
point, but it should end up with LAST_ROOT in nd->last_type.  Which should
not be able to reach the readers of those fields...  Which kernel had
that been on?

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Al Viro]

On Sat, Sep 19, 2020 at 04:44:51PM +0200, Greg KH wrote:

> > ->dir_uid and ->dir_mode are set when link_path_walk() resolves the pathname
> > to directory + final component.  They are used when deciding whether to reject
> > a trailing symlink (on fs.protected_symlinks setups) and whether to allow
> > creation in sticky directories (on fs.protected_regular and fs.protected_fifos
> > setups).  Both operations really need the results of successful link_path_walk().
> > 
> > I don't see how that could be not a false positive.  If we hit the use in
> > may_create_in_sticky(), we'd need the combination of
> > 	* pathname that consists only of slashes (or it will be initialized)
> > 	* LAST_NORM in nd->last_type, which is flat-out impossible, since
> > we are left with LAST_ROOT for such pathnames.  The same goes for
> > may_follow_link() use - we need WALK_TRAILING in flags to hit it in the
> > first place, which can come from two sources -
> >         return walk_component(nd, WALK_TRAILING);
> > in lookup_last() (and walk_component() won't go anywhere near the
> > call chain leading to may_follow_link() without LAST_NORM in nd->last_type)
> > and
> >         res = step_into(nd, WALK_TRAILING, dentry, inode, seq);
> > in open_last_lookups(), which also won't go anywhere near that line without
> > LAST_NORM in the nd->last_type.
> > 
> > IOW, unless we manage to call that without having called link_path_walk()
> > at all or after link_path_walk() returning an error, we shouldn't hit
> > that.  And if we *do* go there without link_path_walk() or with an error
> > from link_path_walk(), we have a much worse problem.
> > 
> > I want to see the details of reproducer.  If it's for real, we have a much
> > more serious problem; if it's a false positive, the right place to deal
> > with it would be elsewhere (perhaps on return from link_path_walk() with
> > a slashes-only pathname), but in any case it should only be done after we
> > manage to understand what's going on.
> 
> Reproducer is pretty simple:
> 	https://syzkaller.appspot.com/text?tag=ReproC&x=13974b2f100000
> 
> Now if that is actually valid or not, I don't know...

Lovely...  That would get an empty path and non-directory for a starting
point, but it should end up with LAST_ROOT in nd->last_type.  Which should
not be able to reach the readers of those fields...  Which kernel had
that been on?
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Al Viro]

On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:

> Lovely...  That would get an empty path and non-directory for a starting
> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
> not be able to reach the readers of those fields...  Which kernel had
> that been on?

Yecchhh...  I see what's going on; I suspect that this ought to be enough.
Folks, could somebody test it on the original reproducer setup?

diff --git a/fs/namei.c b/fs/namei.c
index e99e2a9da0f7..3f02cae7e73f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2113,8 +2113,10 @@ static int link_path_walk(const char *name, struct nameidata *nd)
 		return PTR_ERR(name);
 	while (*name=='/')
 		name++;
-	if (!*name)
+	if (!*name) {
+		nd->dir_mode = 0; // short-circuit the 'hardening' idiocy
 		return 0;
+	}
 
 	/* At this point we know we have a real path component. */
 	for(;;) {

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Al Viro]

On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:

> Lovely...  That would get an empty path and non-directory for a starting
> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
> not be able to reach the readers of those fields...  Which kernel had
> that been on?

Yecchhh...  I see what's going on; I suspect that this ought to be enough.
Folks, could somebody test it on the original reproducer setup?

diff --git a/fs/namei.c b/fs/namei.c
index e99e2a9da0f7..3f02cae7e73f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2113,8 +2113,10 @@ static int link_path_walk(const char *name, struct nameidata *nd)
 		return PTR_ERR(name);
 	while (*name=='/')
 		name++;
-	if (!*name)
+	if (!*name) {
+		nd->dir_mode = 0; // short-circuit the 'hardening' idiocy
 		return 0;
+	}
 
 	/* At this point we know we have a real path component. */
 	for(;;) {
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

On 19-09-2020 17:03, Anant Thazhemadam wrote:
> On 19-09-2020 22:25, Al Viro wrote:
>> On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
>>
>>> Lovely...  That would get an empty path and non-directory for a starting
>>> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
>>> not be able to reach the readers of those fields...  Which kernel had
>>> that been on?
>> Yecchhh...  I see what's going on; I suspect that this ought to be enough.
>> Folks, could somebody test it on the original reproducer setup?
> Sure. I can do that.

Looks like this patch actually fixes this bug.
I made syzbot test the patch, and no issues were triggered!

Note:    syzbot tested the patch with the KMSAN kernel, which
was recently rebased on v5.9-rc4.

Thanks,
Anant

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

On 19-09-2020 17:03, Anant Thazhemadam wrote:
> On 19-09-2020 22:25, Al Viro wrote:
>> On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
>>
>>> Lovely...  That would get an empty path and non-directory for a starting
>>> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
>>> not be able to reach the readers of those fields...  Which kernel had
>>> that been on?
>> Yecchhh...  I see what's going on; I suspect that this ought to be enough.
>> Folks, could somebody test it on the original reproducer setup?
> Sure. I can do that.

Looks like this patch actually fixes this bug.
I made syzbot test the patch, and no issues were triggered!

Note:    syzbot tested the patch with the KMSAN kernel, which
was recently rebased on v5.9-rc4.

Thanks,
Anant

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

On 20-09-2020 01:47, Anant Thazhemadam wrote:
> On 19-09-2020 17:03, Anant Thazhemadam wrote:
>> On 19-09-2020 22:25, Al Viro wrote:
>>> On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
>>>
>>>> Lovely...  That would get an empty path and non-directory for a starting
>>>> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
>>>> not be able to reach the readers of those fields...  Which kernel had
>>>> that been on?
>>> Yecchhh...  I see what's going on; I suspect that this ought to be enough.
>>> Folks, could somebody test it on the original reproducer setup?
>> Sure. I can do that.
> Looks like this patch actually fixes this bug.
> I made syzbot test the patch, and no issues were triggered!
>
> Note:    syzbot tested the patch with the KMSAN kernel, which
> was recently rebased on v5.9-rc4.
>
> Thanks,
> Anant

Ping.
Has the patch that was tested been applied to any tree yet?
If yes, could someone please let me know the commit details, so we can close
the issue? (Unfortunately, I was unable to find it. :( )

Thanks,
Anant

Re: [Linux-kernel-mentees] [PATCH] fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root

[Anant Thazhemadam]

On 20-09-2020 01:47, Anant Thazhemadam wrote:
> On 19-09-2020 17:03, Anant Thazhemadam wrote:
>> On 19-09-2020 22:25, Al Viro wrote:
>>> On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
>>>
>>>> Lovely...  That would get an empty path and non-directory for a starting
>>>> point, but it should end up with LAST_ROOT in nd->last_type.  Which should
>>>> not be able to reach the readers of those fields...  Which kernel had
>>>> that been on?
>>> Yecchhh...  I see what's going on; I suspect that this ought to be enough.
>>> Folks, could somebody test it on the original reproducer setup?
>> Sure. I can do that.
> Looks like this patch actually fixes this bug.
> I made syzbot test the patch, and no issues were triggered!
>
> Note:    syzbot tested the patch with the KMSAN kernel, which
> was recently rebased on v5.9-rc4.
>
> Thanks,
> Anant

Ping.
Has the patch that was tested been applied to any tree yet?
If yes, could someone please let me know the commit details, so we can close
the issue? (Unfortunately, I was unable to find it. :( )

Thanks,
Anant

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees