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
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 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
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-09-17 00:28:13
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
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:
quoted
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.
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-09-19 16:17:40
On Sat, Sep 19, 2020 at 04:44:51PM +0200, Greg KH wrote:
quoted
->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.
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
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-09-19 16:56:07
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?
@@ -2113,8 +2113,10 @@ static int link_path_walk(const char *name, struct nameidata *nd)returnPTR_ERR(name);while(*name=='/')name++;-if(!*name)+if(!*name){+nd->dir_mode=0;// short-circuit the 'hardening' idiocyreturn0;+}/* At this point we know we have a real path component. */for(;;){
On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
quoted
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?
On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
quoted
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
On Sat, Sep 19, 2020 at 05:17:27PM +0100, Al Viro wrote:
quoted
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