On 12.01.2023 05:32, Peng Zhang wrote:
> From: ZhangPeng <[email protected]>
>
> Dan Carpenter reported a Smatch static checker warning:
>
> fs/ntfs3/namei.c:96 ntfs_lookup()
> error: potential NULL/IS_ERR bug 'inode'
> It will cause null-ptr-deref when dir_search_u() returns NULL if the
> file is not found.
> Fix this by replacing IS_ERR() with IS_ERR_OR_NULL() to add a check for
> NULL.
>
> Fixes: fb6b59b5a2d6 ("fs/ntfs3: Fix null-ptr-deref on inode->i_op in ntfs_lookup()")
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: ZhangPeng <[email protected]>
> ---
> fs/ntfs3/namei.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c
> index 3db34d5c03dc..f23c2c26dd08 100644
> --- a/fs/ntfs3/namei.c
> +++ b/fs/ntfs3/namei.c
> @@ -93,7 +93,7 @@ static struct dentry *ntfs_lookup(struct inode *dir, struct dentry *dentry,
> * If the MFT record of ntfs inode is not a base record, inode->i_op can be NULL.
> * This causes null pointer dereference in d_splice_alias().
> */
> - if (!IS_ERR(inode) && inode->i_op == NULL) {
> + if (!IS_ERR_OR_NULL(inode) && inode->i_op == NULL) {
> iput(inode);
> inode = ERR_PTR(-EINVAL);
> }
Hello.
We have added a patch with this check just before the New Year. (here
https://lore.kernel.org/lkml/[email protected]/)
Thank you for your attention!
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/[email protected]/)
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,
On Mon, Jan 16, 2023 at 08:18:07PM +0000, Al Viro wrote:
> 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/[email protected]/)
>
> 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.
While we are at it - what's the point passing the symlink body length
to ntfs_create_inode()? We could calculate it there just as well -
it's used only for symlinks (unsurprisingly) and you've got uncomfortably
many arguments as it is...