Re: ksmbd: fix lookup on idmapped mounts

From: Christian Brauner <christian.brauner@ubuntu.com>
To: Colin Ian King <colin.king@canonical.com>
Cc: Steve French <stfrench@microsoft.com>,
	Christoph Hellwig <hch@infradead.org>,
	Namjae Jeon <namjae.jeon@samsung.com>,
	Hyunchul Lee <hyc.lee@gmail.com>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	David Sterba <dsterba@suse.com>,
	linux-cifs@vger.kernel.org
Subject: Re: ksmbd: fix lookup on idmapped mounts
Date: Thu, 19 Aug 2021 16:15:42 +0200	[thread overview]
Message-ID: <20210819141542.7temhlbcbz2wjq3t@wittgenstein> (raw)
In-Reply-To: <abe632a9-cde9-25b4-f336-4ff128bf7fd9@canonical.com>

On Thu, Aug 19, 2021 at 02:11:43PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis on linux-next with Coverity has detected a potential
> issue in fs/ksmbd/smb2pdu.c with the following commit:
> 
> commit 4b499755e1024f97e75411920a404b357af6e153
> Author: Christian Brauner <christian.brauner@ubuntu.com>
> Date:   Mon Aug 16 13:56:05 2021 +0200
> 
>     ksmbd: fix lookup on idmapped mounts
> 
> The analysis is as follows:
> 
> 5626 static int set_rename_info(struct ksmbd_work *work, struct
> ksmbd_file *fp,
> 5627                           char *buf)
> 5628 {
> 
>     1. var_decl: Declaring variable user_ns without initializer.
> 
> 5629        struct user_namespace *user_ns;
> 5630        struct ksmbd_file *parent_fp;
> 5631        struct dentry *parent;
> 5632        struct dentry *dentry = fp->filp->f_path.dentry;
> 5633        int ret;
> 5634
> 
>     2. Condition !(fp->daccess & 65536U /* (__le32)(__u32)65536 */),
> taking false branch.
> 
> 5635        if (!(fp->daccess & FILE_DELETE_LE)) {
> 5636                pr_err("no right to delete : 0x%x\n", fp->daccess);
> 5637                return -EACCES;
> 5638        }
> 5639
> 
>     3. Condition ksmbd_stream_fd(fp), taking true branch.
> 
> 5640        if (ksmbd_stream_fd(fp))
> 
>     4. Jumping to label next.
> 
> 5641                goto next;
> 5642
> 5643        user_ns = file_mnt_user_ns(fp->filp);
> 5644        parent = dget_parent(dentry);
> 5645        ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);
> 5646        if (ret) {
> 5647                dput(parent);
> 5648                return ret;
> 5649        }
> 5650
> 5651        parent_fp = ksmbd_lookup_fd_inode(d_inode(parent));
> 5652        inode_unlock(d_inode(parent));
> 5653        dput(parent);
> 5654
> 5655        if (parent_fp) {
> 5656                if (parent_fp->daccess & FILE_DELETE_LE) {
> 5657                        pr_err("parent dir is opened with delete
> access\n");
> 5658                        return -ESHARE;
> 5659                }
> 5660        }
> 5661 next:
> 
>     Uninitialized pointer read (UNINIT)
>     5. uninit_use_in_call: Using uninitialized value user_ns when
> calling smb2_rename.
> 
> 5662        return smb2_rename(work, fp, user_ns,
> 5663                           (struct smb2_file_rename_info *)buf,
> 5664                           work->sess->conn->local_nls);
> 5665 }
> 
> The pointer user_ns is not initialized and the goto on line 5641 will
> cause this uninitialized pointer value to be passed to smb2_rename.
> 
> I suspect user_ns = file_mnt_user_ns(fp->filp) should be moved to line
> 5639 as a fix, but I'm concerned that this is not a suitable fix.

Thanks for the report, Colin.
No, the fix you propose is correct. Ugh, the goto in this function is
rather messy:

	if (ksmbd_stream_fd(fp))
		goto next;

	user_ns = file_mnt_user_ns(fp->filp);
	parent = dget_parent(dentry);
	ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);

I'll send a fixed version of this patch later today.

Thanks!
Christian