[PATCH] nfsd4: ensure cm_xid does not change across userspace fetches

[PATCH] nfsd4: ensure cm_xid does not change across userspace fetches

[Meng Xu]

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.

Signed-off-by: Meng Xu <mengxu.gatech@gmail.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

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

[J. Bruce Fields]

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

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

[J. Bruce Fields]

On Tue, Sep 26, 2017 at 05:52:16PM -0400, J. Bruce Fields wrote:
> 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.

Well, I'm bikeshedding, your version's OK.

I'm not convinced there's really a bug, though: as far as I can tell we
*only* use the xid for matching call and response, so if it's changed it
doesn't affect what any later code does.

--b.

> 
> --b.
> 
> > 
> > Signed-off-by: Meng Xu <mengxu.gatech@gmail.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