fs/ntfs3/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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.1On 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!
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,
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...
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.
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...
© 2016 - 2025 Red Hat, Inc.