linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Colin Ian King <colin.king@canonical.com>
To: Christian Brauner <christian.brauner@ubuntu.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 14:11:43 +0100	[thread overview]
Message-ID: <abe632a9-cde9-25b4-f336-4ff128bf7fd9@canonical.com> (raw)

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.

Colin

             reply	other threads:[~2021-08-19 13:11 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-19 13:11 Colin Ian King [this message]
2021-08-19 14:15 ` Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=abe632a9-cde9-25b4-f336-4ff128bf7fd9@canonical.com \
    --to=colin.king@canonical.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=dsterba@suse.com \
    --cc=hch@infradead.org \
    --cc=hyc.lee@gmail.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=namjae.jeon@samsung.com \
    --cc=senozhatsky@chromium.org \
    --cc=stfrench@microsoft.com \
    --subject='Re: ksmbd: fix lookup on idmapped mounts' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).