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@xxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
