| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y8Ww/48pwi8RbTIv@ZenIV>
Date: Mon, 16 Jan 2023 20:18:07 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Konstantin Komarov <almaz.alexandrovich@...agon-software.com>
Cc: Peng Zhang <zhangpeng362@...wei.com>, kari.argillander@...il.com,
akpm@...ux-foundation.org, ntfs3@...ts.linux.dev,
linux-kernel@...r.kernel.org, sunnanyong@...wei.com,
wangkefeng.wang@...wei.com, Dan Carpenter <error27@...il.com>
Subject: Re: [PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in
ntfs_lookup()
On Fri, Jan 13, 2023 at 02:05:56PM +0400, Konstantin Komarov wrote:
> Hello.
>
> We have added a patch with this check just before the New Year. (here https://lore.kernel.org/lkml/ee705b24-865b-26ff-157d-4cb2a303a962@paragon-software.com/)
See upthread for the reasons why that's wrong. Incidentally,
mixing logical change with a pile of whitespace changes is
bad idea - it's very easy for reviewers to miss...
Other observation from the cursory look through your namei.c:
ntfs_create_inode() has no reason to return inode; the reference
it creates goes into dentry. Make it return int, the callers will
be happier. While we are at it, use d_instantiate_new() instead
of d_instantiate() + unlock_new_inode() there.
Incidentally, control flow in there is harder to follow that it
needs to be:
* everything that reaches out{3,4,5,6,7} is guaranteed
to have err != 0;
* fallthrough into out2 is guaranteed to have err != 0;
direct branch to it - err == 0.
* direct branch to out1 is guaranteed to have err != 0.
I would suggest something along the lines of the following (completely
untested) delta; the callers are clearly better off that way and
failure paths are separated from the success one - they didn't share
anywhere near enough to have it worth bothering.
diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index 51f9542de7b0..3ae4ad329b51 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -1197,11 +1197,11 @@ ntfs_create_reparse_buffer(struct ntfs_sb_info *sbi, const char *symname,
*
* NOTE: if fnd != NULL (ntfs_atomic_open) then @dir is locked
*/
-struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
- struct inode *dir, struct dentry *dentry,
- const struct cpu_str *uni, umode_t mode,
- dev_t dev, const char *symname, u32 size,
- struct ntfs_fnd *fnd)
+int ntfs_create_inode(struct user_namespace *mnt_userns,
+ struct inode *dir, struct dentry *dentry,
+ const struct cpu_str *uni, umode_t mode,
+ dev_t dev, const char *symname, u32 size,
+ struct ntfs_fnd *fnd)
{
int err;
struct super_block *sb = dir->i_sb;
@@ -1642,18 +1642,19 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
goto out7;
}
- /*
- * Call 'd_instantiate' after inode->i_op is set
- * but before finish_open.
- */
- d_instantiate(dentry, inode);
-
ntfs_save_wsl_perm(inode);
mark_inode_dirty(dir);
mark_inode_dirty(inode);
/* Normal exit. */
- goto out2;
+ __putname(new_de);
+ kfree(rp);
+ /*
+ * Call 'd_instantiate_new' after inode->i_op is set
+ * but before finish_open.
+ */
+ d_instantiate_new(dentry, inode);
+ return 0;
out7:
@@ -1678,21 +1679,13 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
discard_new_inode(inode);
out3:
ntfs_mark_rec_free(sbi, ino, false);
-
out2:
__putname(new_de);
kfree(rp);
-
out1:
- if (err) {
- if (!fnd)
- ni_unlock(dir_ni);
- return ERR_PTR(err);
- }
-
- unlock_new_inode(inode);
-
- return inode;
+ if (!fnd)
+ ni_unlock(dir_ni);
+ return err;
}
int ntfs_link_inode(struct inode *inode, struct dentry *dentry)
diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c
index 53ddea219e37..f98d0252a785 100644
--- a/fs/ntfs3/namei.c
+++ b/fs/ntfs3/namei.c
@@ -107,12 +107,8 @@ static struct dentry *ntfs_lookup(struct inode *dir, struct dentry *dentry,
static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, bool excl)
{
- struct inode *inode;
-
- inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFREG | mode,
+ return ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFREG | mode,
0, NULL, 0, NULL);
-
- return IS_ERR(inode) ? PTR_ERR(inode) : 0;
}
/*
@@ -123,12 +119,8 @@ static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir,
static int ntfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, dev_t rdev)
{
- struct inode *inode;
-
- inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, mode, rdev,
+ return ntfs_create_inode(mnt_userns, dir, dentry, NULL, mode, rdev,
NULL, 0, NULL);
-
- return IS_ERR(inode) ? PTR_ERR(inode) : 0;
}
/*
@@ -196,13 +188,8 @@ static int ntfs_unlink(struct inode *dir, struct dentry *dentry)
static int ntfs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, const char *symname)
{
- u32 size = strlen(symname);
- struct inode *inode;
-
- inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFLNK | 0777,
- 0, symname, size, NULL);
-
- return IS_ERR(inode) ? PTR_ERR(inode) : 0;
+ return ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFLNK | 0777,
+ 0, symname, strlen(symname), NULL);
}
/*
@@ -211,12 +198,8 @@ static int ntfs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
static int ntfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode)
{
- struct inode *inode;
-
- inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFDIR | mode,
+ return ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFDIR | mode,
0, NULL, 0, NULL);
-
- return IS_ERR(inode) ? PTR_ERR(inode) : 0;
}
/*
@@ -358,7 +341,6 @@ static int ntfs_atomic_open(struct inode *dir, struct dentry *dentry,
struct file *file, u32 flags, umode_t mode)
{
int err;
- struct inode *inode;
struct ntfs_fnd *fnd = NULL;
struct ntfs_inode *ni = ntfs_i(dir);
struct dentry *d = NULL;
@@ -431,10 +413,10 @@ static int ntfs_atomic_open(struct inode *dir, struct dentry *dentry,
* Looks like ntfs_atomic_open must accept 'struct user_namespace *mnt_userns' as argument.
*/
- inode = ntfs_create_inode(&init_user_ns, dir, dentry, uni, mode, 0,
+ err = ntfs_create_inode(&init_user_ns, dir, dentry, uni, mode, 0,
NULL, 0, fnd);
- err = IS_ERR(inode) ? PTR_ERR(inode)
- : finish_open(file, dentry, ntfs_file_open);
+ if (!err)
+ err = finish_open(file, dentry, ntfs_file_open);
dput(d);
out2:
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index 2050eb3f6a5a..cea0b8a3a38a 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -707,7 +707,7 @@ int ntfs_sync_inode(struct inode *inode);
int ntfs_flush_inodes(struct super_block *sb, struct inode *i1,
struct inode *i2);
int inode_write_data(struct inode *inode, const void *data, size_t bytes);
-struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
+int ntfs_create_inode(struct user_namespace *mnt_userns,
struct inode *dir, struct dentry *dentry,
const struct cpu_str *uni, umode_t mode,
dev_t dev, const char *symname, u32 size,
Powered by blists - more mailing lists