| Message ID | 20200916052657.18683-1-anant.thazhemadam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: fix KMSAN uninit-value bug by initializing nd in do_file_open_root | expand |
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
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
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.
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
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
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?
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(;;) {
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
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
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;