| Message ID | 20150212151451.18290.89745.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-02-12 at 10:14 -0500, Chuck Lever wrote: > Dan Carpenter's static checker pointed out: > > net/sunrpc/xprtrdma/rpc_rdma.c:879 rpcrdma_reply_handler() > warn: can 'credits' be negative? > > "credits" is defined as an int. The credits value comes from the > server as a 32-bit unsigned integer. > > A malicious or broken server can plant a large unsigned integer in > that field which would result in an underflow in the following > logic, potentially triggering a deadlock of the mount point by > blocking the client from issuing more RPC requests. > > net/sunrpc/xprtrdma/rpc_rdma.c: > > 876 credits = be32_to_cpu(headerp->rm_credit); > 877 if (credits == 0) > 878 credits = 1; /* don't deadlock */ > 879 else if (credits > r_xprt->rx_buf.rb_max_requests) > 880 credits = r_xprt->rx_buf.rb_max_requests; > 881 > 882 cwnd = xprt->cwnd; > 883 xprt->cwnd = credits << RPC_CWNDSHIFT; > 884 if (xprt->cwnd > cwnd) > 885 xprt_release_rqst_cong(rqst->rq_task); > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Fixes: eba8ff660b2d ("xprtrdma: Move credit update to RPC . . .") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/xprtrdma/rpc_rdma.c | 3 ++- > net/sunrpc/xprtrdma/xprt_rdma.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c > index 7e9acd9..91ffde8 100644 > --- a/net/sunrpc/xprtrdma/rpc_rdma.c > +++ b/net/sunrpc/xprtrdma/rpc_rdma.c > @@ -738,8 +738,9 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep) > struct rpc_xprt *xprt = rep->rr_xprt; > struct rpcrdma_xprt *r_xprt = rpcx_to_rdmax(xprt); > __be32 *iptr; > - int credits, rdmalen, status; > + int rdmalen, status; > unsigned long cwnd; > + u32 credits; > > /* Check status. If bad, signal disconnect and return rep to pool */ > if (rep->rr_len == ~0U) { > diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h > index d1b7039..0a16fb6 100644 > --- a/net/sunrpc/xprtrdma/xprt_rdma.h > +++ b/net/sunrpc/xprtrdma/xprt_rdma.h > @@ -285,7 +285,7 @@ rpcr_to_rdmar(struct rpc_rqst *rqst) > */ > struct rpcrdma_buffer { > spinlock_t rb_lock; /* protects indexes */ > - int rb_max_requests;/* client max requests */ > + u32 rb_max_requests;/* client max requests */ > struct list_head rb_mws; /* optional memory windows/fmrs/frmrs */ > struct list_head rb_all; > int rb_send_index; > Anna, do you want me to take this one directly, or are you preparing a pull request? Cheers Trond -- 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 02/13/2015 05:43 PM, Trond Myklebust wrote: > On Thu, 2015-02-12 at 10:14 -0500, Chuck Lever wrote: >> Dan Carpenter's static checker pointed out: >> >> net/sunrpc/xprtrdma/rpc_rdma.c:879 rpcrdma_reply_handler() >> warn: can 'credits' be negative? >> >> "credits" is defined as an int. The credits value comes from the >> server as a 32-bit unsigned integer. >> >> A malicious or broken server can plant a large unsigned integer in >> that field which would result in an underflow in the following >> logic, potentially triggering a deadlock of the mount point by >> blocking the client from issuing more RPC requests. >> >> net/sunrpc/xprtrdma/rpc_rdma.c: >> >> 876 credits = be32_to_cpu(headerp->rm_credit); >> 877 if (credits == 0) >> 878 credits = 1; /* don't deadlock */ >> 879 else if (credits > r_xprt->rx_buf.rb_max_requests) >> 880 credits = r_xprt->rx_buf.rb_max_requests; >> 881 >> 882 cwnd = xprt->cwnd; >> 883 xprt->cwnd = credits << RPC_CWNDSHIFT; >> 884 if (xprt->cwnd > cwnd) >> 885 xprt_release_rqst_cong(rqst->rq_task); >> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Fixes: eba8ff660b2d ("xprtrdma: Move credit update to RPC . . .") >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> net/sunrpc/xprtrdma/rpc_rdma.c | 3 ++- >> net/sunrpc/xprtrdma/xprt_rdma.h | 2 +- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c >> index 7e9acd9..91ffde8 100644 >> --- a/net/sunrpc/xprtrdma/rpc_rdma.c >> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c >> @@ -738,8 +738,9 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep) >> struct rpc_xprt *xprt = rep->rr_xprt; >> struct rpcrdma_xprt *r_xprt = rpcx_to_rdmax(xprt); >> __be32 *iptr; >> - int credits, rdmalen, status; >> + int rdmalen, status; >> unsigned long cwnd; >> + u32 credits; >> >> /* Check status. If bad, signal disconnect and return rep to pool */ >> if (rep->rr_len == ~0U) { >> diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h >> index d1b7039..0a16fb6 100644 >> --- a/net/sunrpc/xprtrdma/xprt_rdma.h >> +++ b/net/sunrpc/xprtrdma/xprt_rdma.h >> @@ -285,7 +285,7 @@ rpcr_to_rdmar(struct rpc_rqst *rqst) >> */ >> struct rpcrdma_buffer { >> spinlock_t rb_lock; /* protects indexes */ >> - int rb_max_requests;/* client max requests */ >> + u32 rb_max_requests;/* client max requests */ >> struct list_head rb_mws; /* optional memory windows/fmrs/frmrs */ >> struct list_head rb_all; >> int rb_send_index; >> > > Anna, do you want me to take this one directly, or are you preparing a > pull request? I'll make a pull request. Thanks, Anna > > Cheers > Trond > -- 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/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c index 7e9acd9..91ffde8 100644 --- a/net/sunrpc/xprtrdma/rpc_rdma.c +++ b/net/sunrpc/xprtrdma/rpc_rdma.c @@ -738,8 +738,9 @@ rpcrdma_reply_handler(struct rpcrdma_rep *rep) struct rpc_xprt *xprt = rep->rr_xprt; struct rpcrdma_xprt *r_xprt = rpcx_to_rdmax(xprt); __be32 *iptr; - int credits, rdmalen, status; + int rdmalen, status; unsigned long cwnd; + u32 credits; /* Check status. If bad, signal disconnect and return rep to pool */ if (rep->rr_len == ~0U) { diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h index d1b7039..0a16fb6 100644 --- a/net/sunrpc/xprtrdma/xprt_rdma.h +++ b/net/sunrpc/xprtrdma/xprt_rdma.h @@ -285,7 +285,7 @@ rpcr_to_rdmar(struct rpc_rqst *rqst) */ struct rpcrdma_buffer { spinlock_t rb_lock; /* protects indexes */ - int rb_max_requests;/* client max requests */ + u32 rb_max_requests;/* client max requests */ struct list_head rb_mws; /* optional memory windows/fmrs/frmrs */ struct list_head rb_all; int rb_send_index;
Dan Carpenter's static checker pointed out: net/sunrpc/xprtrdma/rpc_rdma.c:879 rpcrdma_reply_handler() warn: can 'credits' be negative? "credits" is defined as an int. The credits value comes from the server as a 32-bit unsigned integer. A malicious or broken server can plant a large unsigned integer in that field which would result in an underflow in the following logic, potentially triggering a deadlock of the mount point by blocking the client from issuing more RPC requests. net/sunrpc/xprtrdma/rpc_rdma.c: 876 credits = be32_to_cpu(headerp->rm_credit); 877 if (credits == 0) 878 credits = 1; /* don't deadlock */ 879 else if (credits > r_xprt->rx_buf.rb_max_requests) 880 credits = r_xprt->rx_buf.rb_max_requests; 881 882 cwnd = xprt->cwnd; 883 xprt->cwnd = credits << RPC_CWNDSHIFT; 884 if (xprt->cwnd > cwnd) 885 xprt_release_rqst_cong(rqst->rq_task); Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Fixes: eba8ff660b2d ("xprtrdma: Move credit update to RPC . . .") Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- net/sunrpc/xprtrdma/rpc_rdma.c | 3 ++- net/sunrpc/xprtrdma/xprt_rdma.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) -- 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