[patch -stable] svcrdma: underflow issue in decode_write_list()

[patch -stable] svcrdma: underflow issue in decode_write_list()

[Dan Carpenter]

My static checker marks everything from ntohl() as untrusted and it
complains we could have an underflow problem doing:

	return (u32 *)&ary->wc_array[nchunks];

Also on 32 bit systems the upper bound check could overflow.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/net/sunrpc/xprtrdma/svc_rdma_marshal.c b/net/sunrpc/xprtrdma/svc_rdma_marshal.c
index 8d2eddd..65b1462 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_marshal.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_marshal.c
@@ -98,6 +98,7 @@ void svc_rdma_rcl_chunk_counts(struct rpcrdma_read_chunk *ch,
  */
 static u32 *decode_write_list(u32 *va, u32 *vaend)
 {
+	unsigned long start, end;
 	int nchunks;
 
 	struct rpcrdma_write_array *ary =
@@ -113,9 +114,12 @@ static u32 *decode_write_list(u32 *va, u32 *vaend)
 		return NULL;
 	}
 	nchunks = ntohl(ary->wc_nchunks);
-	if (((unsigned long)&ary->wc_array[0] +
-	     (sizeof(struct rpcrdma_write_chunk) * nchunks)) >
-	    (unsigned long)vaend) {
+
+	start = (unsigned long)&ary->wc_array[0];
+	end = (unsigned long)vaend;
+	if (nchunks < 0 ||
+	    nchunks > (SIZE_MAX - start) / sizeof(struct rpcrdma_write_chunk) ||
+	    (start + (sizeof(struct rpcrdma_write_chunk) * nchunks)) > end) {
 		dprintk("svcrdma: ary=%p, wc_nchunks=%d, vaend=%p\n",
 			ary, nchunks, vaend);
 		return NULL;
@@ -129,6 +133,7 @@ static u32 *decode_write_list(u32 *va, u32 *vaend)
 
 static u32 *decode_reply_array(u32 *va, u32 *vaend)
 {
+	unsigned long start, end;
 	int nchunks;
 	struct rpcrdma_write_array *ary =
 		(struct rpcrdma_write_array *)va;
@@ -143,9 +148,12 @@ static u32 *decode_reply_array(u32 *va, u32 *vaend)
 		return NULL;
 	}
 	nchunks = ntohl(ary->wc_nchunks);
-	if (((unsigned long)&ary->wc_array[0] +
-	     (sizeof(struct rpcrdma_write_chunk) * nchunks)) >
-	    (unsigned long)vaend) {
+
+	start = (unsigned long)&ary->wc_array[0];
+	end = (unsigned long)vaend;
+	if (nchunks < 0 ||
+	    nchunks > (SIZE_MAX - start) / sizeof(struct rpcrdma_write_chunk) ||
+	    (start + (sizeof(struct rpcrdma_write_chunk) * nchunks)) > end) {
 		dprintk("svcrdma: ary=%p, wc_nchunks=%d, vaend=%p\n",
 			ary, nchunks, vaend);
 		return NULL;

Re: [patch -stable] svcrdma: underflow issue in decode_write_list()

[walter harms]

Am 12.07.2013 08:39, schrieb Dan Carpenter:
> My static checker marks everything from ntohl() as untrusted and it
> complains we could have an underflow problem doing:
> 
> 	return (u32 *)&ary->wc_array[nchunks];
> 
> Also on 32 bit systems the upper bound check could overflow.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_marshal.c b/net/sunrpc/xprtrdma/svc_rdma_marshal.c
> index 8d2eddd..65b1462 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_marshal.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_marshal.c
> @@ -98,6 +98,7 @@ void svc_rdma_rcl_chunk_counts(struct rpcrdma_read_chunk *ch,
>   */
>  static u32 *decode_write_list(u32 *va, u32 *vaend)
>  {
> +	unsigned long start, end;
>  	int nchunks;
>  
>  	struct rpcrdma_write_array *ary =
> @@ -113,9 +114,12 @@ static u32 *decode_write_list(u32 *va, u32 *vaend)
>  		return NULL;
>  	}
>  	nchunks = ntohl(ary->wc_nchunks);
> -	if (((unsigned long)&ary->wc_array[0] +
> -	     (sizeof(struct rpcrdma_write_chunk) * nchunks)) >
> -	    (unsigned long)vaend) {
> +
> +	start = (unsigned long)&ary->wc_array[0];
> +	end = (unsigned long)vaend;
> +	if (nchunks < 0 ||
> +	    nchunks > (SIZE_MAX - start) / sizeof(struct rpcrdma_write_chunk) ||
> +	    (start + (sizeof(struct rpcrdma_write_chunk) * nchunks)) > end) {
>  		dprintk("svcrdma: ary=%p, wc_nchunks=%d, vaend=%p\n",
>  			ary, nchunks, vaend);


i am struggling to understand what is actually checked here.
Perhaps this improves the readability a bit
 if ( nchunks < 0 ||
       sizeof(struct rpcrdma_write_chunk) * nchunks > (SIZE_MAX - start) ||
       sizeof(struct rpcrdma_write_chunk) * nchunks > (end - start) )

 with that rewrite i would say that (SIZE_MAX - start) is strange.

just my 2 cents,
 wh

>  		return NULL;
> @@ -129,6 +133,7 @@ static u32 *decode_write_list(u32 *va, u32 *vaend)
>  
>  static u32 *decode_reply_array(u32 *va, u32 *vaend)
>  {
> +	unsigned long start, end;
>  	int nchunks;
>  	struct rpcrdma_write_array *ary =
>  		(struct rpcrdma_write_array *)va;
> @@ -143,9 +148,12 @@ static u32 *decode_reply_array(u32 *va, u32 *vaend)
>  		return NULL;
>  	}
>  	nchunks = ntohl(ary->wc_nchunks);
> -	if (((unsigned long)&ary->wc_array[0] +
> -	     (sizeof(struct rpcrdma_write_chunk) * nchunks)) >
> -	    (unsigned long)vaend) {
> +
> +	start = (unsigned long)&ary->wc_array[0];
> +	end = (unsigned long)vaend;
> +	if (nchunks < 0 ||
> +	    nchunks > (SIZE_MAX - start) / sizeof(struct rpcrdma_write_chunk) ||
> +	    (start + (sizeof(struct rpcrdma_write_chunk) * nchunks)) > end) {
>  		dprintk("svcrdma: ary=%p, wc_nchunks=%d, vaend=%p\n",
>  			ary, nchunks, vaend);
>  		return NULL;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch -stable] svcrdma: underflow issue in decode_write_list()

[J. Bruce Fields]

On Fri, Jul 12, 2013 at 10:24:41AM +0200, walter harms wrote:
> 
> 
> Am 12.07.2013 08:39, schrieb Dan Carpenter:
> > My static checker marks everything from ntohl() as untrusted and it
> > complains we could have an underflow problem doing:
> > 
> > 	return (u32 *)&ary->wc_array[nchunks];
> > 
> > Also on 32 bit systems the upper bound check could overflow.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/net/sunrpc/xprtrdma/svc_rdma_marshal.c b/net/sunrpc/xprtrdma/svc_rdma_marshal.c
> > index 8d2eddd..65b1462 100644
> > --- a/net/sunrpc/xprtrdma/svc_rdma_marshal.c
> > +++ b/net/sunrpc/xprtrdma/svc_rdma_marshal.c
> > @@ -98,6 +98,7 @@ void svc_rdma_rcl_chunk_counts(struct rpcrdma_read_chunk *ch,
> >   */
> >  static u32 *decode_write_list(u32 *va, u32 *vaend)
> >  {
> > +	unsigned long start, end;
> >  	int nchunks;
> >  
> >  	struct rpcrdma_write_array *ary =
> > @@ -113,9 +114,12 @@ static u32 *decode_write_list(u32 *va, u32 *vaend)
> >  		return NULL;
> >  	}
> >  	nchunks = ntohl(ary->wc_nchunks);
> > -	if (((unsigned long)&ary->wc_array[0] +
> > -	     (sizeof(struct rpcrdma_write_chunk) * nchunks)) >
> > -	    (unsigned long)vaend) {
> > +
> > +	start = (unsigned long)&ary->wc_array[0];
> > +	end = (unsigned long)vaend;
> > +	if (nchunks < 0 ||
> > +	    nchunks > (SIZE_MAX - start) / sizeof(struct rpcrdma_write_chunk) ||
> > +	    (start + (sizeof(struct rpcrdma_write_chunk) * nchunks)) > end) {
> >  		dprintk("svcrdma: ary=%p, wc_nchunks=%d, vaend=%p\n",
> >  			ary, nchunks, vaend);
> 
> 
> i am struggling to understand what is actually checked here.
> Perhaps this improves the readability a bit
>  if ( nchunks < 0 ||
>        sizeof(struct rpcrdma_write_chunk) * nchunks > (SIZE_MAX - start) ||
>        sizeof(struct rpcrdma_write_chunk) * nchunks > (end - start) )

If the product on the left-hand size overflows, the product could pass
all these tests while nchunks is still too large.  That's the same
problem the original code had.

Committing Dan's version unless someone has something better.

--b.

> 
>  with that rewrite i would say that (SIZE_MAX - start) is strange.
> 
> just my 2 cents,
>  wh
> 
> >  		return NULL;
> > @@ -129,6 +133,7 @@ static u32 *decode_write_list(u32 *va, u32 *vaend)
> >  
> >  static u32 *decode_reply_array(u32 *va, u32 *vaend)
> >  {
> > +	unsigned long start, end;
> >  	int nchunks;
> >  	struct rpcrdma_write_array *ary =
> >  		(struct rpcrdma_write_array *)va;
> > @@ -143,9 +148,12 @@ static u32 *decode_reply_array(u32 *va, u32 *vaend)
> >  		return NULL;
> >  	}
> >  	nchunks = ntohl(ary->wc_nchunks);
> > -	if (((unsigned long)&ary->wc_array[0] +
> > -	     (sizeof(struct rpcrdma_write_chunk) * nchunks)) >
> > -	    (unsigned long)vaend) {
> > +
> > +	start = (unsigned long)&ary->wc_array[0];
> > +	end = (unsigned long)vaend;
> > +	if (nchunks < 0 ||
> > +	    nchunks > (SIZE_MAX - start) / sizeof(struct rpcrdma_write_chunk) ||
> > +	    (start + (sizeof(struct rpcrdma_write_chunk) * nchunks)) > end) {
> >  		dprintk("svcrdma: ary=%p, wc_nchunks=%d, vaend=%p\n",
> >  			ary, nchunks, vaend);
> >  		return NULL;
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [patch -stable] svcrdma: underflow issue in decode_write_list()

[Dan Carpenter]

On 7/12/13, walter harms <wharms-fPG8STNUNVg@public.gmane.org> wrote:

> i am struggling to understand what is actually checked here.
> Perhaps this improves the readability a bit
>  if ( nchunks < 0 ||
>        sizeof(struct rpcrdma_write_chunk) * nchunks > (SIZE_MAX - start) ||

That doesn't work.

This is one of the few times I get to use algebra in real life.  Yay!
The problem is you have to
have the nchunks by itself and the trusted bits on the other side.  So
the multiply becomes a
divide.

There are lots of these checks in the kernel.  It's idiomatic.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html