[PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in ntfs_lookup()

[PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in ntfs_lookup()

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

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 <error27@gmail.com>
Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
---
 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);
 	}
-- 
2.25.1

Re: [PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in ntfs_lookup()

[Al Viro]

On Thu, Jan 12, 2023 at 01:32:48AM +0000, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> 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.

That's a bad approach - you are papering over bad calling conventions instead of
fixing them.

IS_ERR_OR_NULL is almost never the right tool.  Occasionally there are valid
cases for function possibly returning pointer/NULL/ERR_PTR(...); this is
almost certainly not one of those.

Incidentally, inodes with NULL ->i_op should never exist.  _Any_ place that
sets ->i_op to NULL is broken, plain and simple.  A new instance of struct
inode has ->i_op pointing to empty method table; it *is* initialized.

Re: [PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in ntfs_lookup()

[Al Viro]

On Thu, Jan 12, 2023 at 03:43:31AM +0000, Al Viro wrote:
> On Thu, Jan 12, 2023 at 01:32:48AM +0000, Peng Zhang wrote:
> > From: ZhangPeng <zhangpeng362@huawei.com>
> > 
> > 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.
> 
> That's a bad approach - you are papering over bad calling conventions instead of
> fixing them.
> 
> IS_ERR_OR_NULL is almost never the right tool.  Occasionally there are valid
> cases for function possibly returning pointer/NULL/ERR_PTR(...); this is
> almost certainly not one of those.
> 
> Incidentally, inodes with NULL ->i_op should never exist.  _Any_ place that
> sets ->i_op to NULL is broken, plain and simple.  A new instance of struct
> inode has ->i_op pointing to empty method table; it *is* initialized.

IOW, the real bug is in ntfs_read_mft() -
	inode->i_op = NULL;
in there is garbage.  Unless I'm misreading the history, it used to be possible
for the damn thing to get all the way to ntfs_lookup() - up until the
commit 0e8235d28f3a "fs/ntfs3: Check fields while reading" had taken that
path out:
-       if (!is_rec_base(rec))
-               goto Ok;
+       if (!is_rec_base(rec)) {
+               err = -EINVAL;
+               goto out;
+       }
is the relevant part.  Situation after that commit:

	* useless check in ntfs_lookup() is a dead code; it should be
taken out, especially since it's broken.
	* NULL assignment in ntfs_read_mft() is still garbage; thankfully,
with the current tree the inode will either have it overwritten by later
assignment or it won't make it out of ntfs_read_mft().  Still, that
assignment should be taken out and shot to get rid of bad example.

While we are at it, the calling conventions of ntfs_read_mft() could've
been better.  Look:

ntfs_read_mft(inode, ...) either returns its first argument (on success) or
it disposes of the inode the argument points to and returns ERR_PTR(-E...)
(on failure).  There is only one caller, and it would be easier to follow if
it had been
        /* If this is a freshly allocated inode, need to read it now. */
        if (inode->i_state & I_NEW) {
		int err = ntfs_read_mft(inode, name, ref);

		if (unlikely(err)) {
			if (name)
				ntfs_set_state(sb->s_fs_info, NTFS_DIRTY_ERROR);
			iget_failed(inode);
			return ERR_PTR(err);
		}
	} else if (ref->seq != ntfs_i(inode)->mi.mrec->seq) {
                /* Inode overlaps? */
                _ntfs_bad_inode(inode);
        }

        return inode;

with ntfs_read_mft() always acting the same way wrt inode refcount...

Re: [PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in ntfs_lookup()

[Konstantin Komarov]

On 12.01.2023 05:32, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
>
> 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 <error27@gmail.com>
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> ---
>   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/ee705b24-865b-26ff-157d-4cb2a303a962@paragon-software.com/)

Thank you for your attention!

Re: [PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in ntfs_lookup()

[Al Viro]

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,

Re: [PATCH -next] fs/ntfs3: Fix potential NULL/IS_ERR bug in ntfs_lookup()

[Al Viro]

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/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.

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...