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