| Message ID | 20210901001341.79887-1-stephen.s.brennan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | namei: Fix use after free in kern_path_locked | expand |
On Wed, Sep 1, 2021 at 7:13 AM Stephen Brennan <stephen.s.brennan@oracle.com> wrote: > > In 0ee50b47532a ("namei: change filename_parentat() calling > conventions"), filename_parentat() was made to always put the struct > filename before returning, and kern_path_locked() was migrated to this > calling convention. However, kern_path_locked() uses the "last" > parameter to lookup and potentially create a new dentry. The last > parameter contains the last component of the path and points within the > filename, which was recently freed at the end of filename_parentat(). > Thus, when kern_path_locked() calls __lookup_hash(), it is using the > filename after it has already been freed. > > To avoid this, switch back to __filename_parentat() and place a putname > at the end of the function, once all uses are completed. Ouch. Thanks for taking care of this, Stephen. I guess filename_parentat() should be killed, since kern_path_locked() was the only place it's used in and it always results in danging "last", provoking bugs just like this one. I can send a patch on top of this if you prefer.
On Wed, Sep 01, 2021 at 02:35:08PM +0700, Dmitry Kadashev wrote: > On Wed, Sep 1, 2021 at 7:13 AM Stephen Brennan > <stephen.s.brennan@oracle.com> wrote: > > > > In 0ee50b47532a ("namei: change filename_parentat() calling > > conventions"), filename_parentat() was made to always put the struct > > filename before returning, and kern_path_locked() was migrated to this > > calling convention. However, kern_path_locked() uses the "last" > > parameter to lookup and potentially create a new dentry. The last > > parameter contains the last component of the path and points within the > > filename, which was recently freed at the end of filename_parentat(). > > Thus, when kern_path_locked() calls __lookup_hash(), it is using the > > filename after it has already been freed. > > > > To avoid this, switch back to __filename_parentat() and place a putname > > at the end of the function, once all uses are completed. > > Ouch. Thanks for taking care of this, Stephen. I guess > filename_parentat() should be killed, since kern_path_locked() was the > only place it's used in and it always results in danging "last", > provoking bugs just like this one. I can send a patch on top of this if > you prefer. Yes. And then rename __filename_parentat to filename_parentat, please.
On Wed, Sep 1, 2021 at 4:13 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Sep 01, 2021 at 02:35:08PM +0700, Dmitry Kadashev wrote: > > Ouch. Thanks for taking care of this, Stephen. I guess > > filename_parentat() should be killed, since kern_path_locked() was the > > only place it's used in and it always results in danging "last", > > provoking bugs just like this one. I can send a patch on top of this if > > you prefer. > > Yes. And then rename __filename_parentat to filename_parentat, please. I see why you want it to be renamed - and I'll send the patch. The only problem I have with the rename is with __filename_parentat() there is a nice uniformity: filename_* functions consume the passed name, and __filename_* do not. So maybe it's something nice to have. Maybe not. Anyway, as I've mentioned, I'll send the patch and it can be either picked up or ignored.
diff --git a/fs/namei.c b/fs/namei.c index d049d3972695..a0122f0016a3 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2551,17 +2551,22 @@ static int filename_parentat(int dfd, struct filename *name, /* does lookup, returns the object with parent locked */ struct dentry *kern_path_locked(const char *name, struct path *path) { + struct filename *filename; struct dentry *d; struct qstr last; int type, error; - error = filename_parentat(AT_FDCWD, getname_kernel(name), 0, path, + filename = getname_kernel(name); + error = __filename_parentat(AT_FDCWD, filename, 0, path, &last, &type); - if (error) - return ERR_PTR(error); + if (error) { + d = ERR_PTR(error); + goto out; + } if (unlikely(type != LAST_NORM)) { path_put(path); - return ERR_PTR(-EINVAL); + d = ERR_PTR(-EINVAL); + goto out; } inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); d = __lookup_hash(&last, path->dentry, 0); @@ -2569,6 +2574,8 @@ struct dentry *kern_path_locked(const char *name, struct path *path) inode_unlock(path->dentry->d_inode); path_put(path); } +out: + putname(filename); return d; }
In 0ee50b47532a ("namei: change filename_parentat() calling conventions"), filename_parentat() was made to always put the struct filename before returning, and kern_path_locked() was migrated to this calling convention. However, kern_path_locked() uses the "last" parameter to lookup and potentially create a new dentry. The last parameter contains the last component of the path and points within the filename, which was recently freed at the end of filename_parentat(). Thus, when kern_path_locked() calls __lookup_hash(), it is using the filename after it has already been freed. To avoid this, switch back to __filename_parentat() and place a putname at the end of the function, once all uses are completed. Fixes: 0ee50b47532a ("namei: change filename_parentat() calling conventions") Reported-by: syzbot+fb0d60a179096e8c2731@syzkaller.appspotmail.com Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com> --- fs/namei.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)