| Message ID | 1506272380-17769-1-git-send-email-mengxu.gatech@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(+)