linux-kernel - Re: [PATCH] nfsd4: ensure cm_xid does not change across userspace fetches

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170926215216.GA30989@fieldses.org>
Date:   Tue, 26 Sep 2017 17:52:16 -0400
From:   "J. Bruce Fields" <bfields@...ldses.org>
To:     Meng Xu <mengxu.gatech@...il.com>
Cc:     jlayton@...chiereds.net, linux-nfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, meng.xu@...ech.edu,
        sanidhya@...ech.edu, taesoo@...ech.edu
Subject: Re: [PATCH] nfsd4: ensure cm_xid does not change across userspace
 fetches

On Sun, Sep 24, 2017 at 12:59:40PM -0400, Meng Xu wrote:
> cld_pipe_downcall() has two fetches from an overapped userspace memory.
> The first fetch copy_from_user(&xid, &cmsg->cm_xid, sizeof(xid)) get
> the xid and use xid to lookup the parent struct cld_upcall *cup.
> The second fetch copy_from_user(&cup->cu_msg, src, mlen) place the whole
> message into &cup->cu_msg.
> 
> Since the userspace thread has full control over this &cmsg->cm_xid,
> it can race condition to change the cm_xid value across the two fetches,
> (say, change from 1 to 2), therefore, de-listing the cup with
> cu_msg.cm_xid == 1 but later put cu_msg.cm_xid = 2.
> 
> Whether this double-fetch situation is a security critical bug depends
> on how cup->cu_msg is used later. However, given that it is hard to
> enumerate all the possible use cases, a safer way might be to ensure
> that the xid does not change across the fetches, which is what this
> patch is for.

Alternatively, couldn't we copy the whole message just once at the
start?  It doesn't look like there very large, so there's room for it on
the stack, if that's the problem.

--b.

> 
> Signed-off-by: Meng Xu <mengxu.gatech@...il.com>
> ---
>  fs/nfsd/nfs4recover.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 66eaeb1..1abe9ed 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -753,6 +753,11 @@ cld_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
>  	if (copy_from_user(&cup->cu_msg, src, mlen) != 0)
>  		return -EFAULT;
>  
> +	/* ensure that the xid has not been changed */
> +	if (cup->cu_msg.cm_xid != xid) {
> +		return -EFAULT;
> +	}
> +
>  	wake_up_process(cup->cu_task);
>  	return mlen;
>  }
> -- 
> 2.7.4