[PATCH] rds: avoid potential stack overflow

[PATCH] rds: avoid potential stack overflow

[Arnd Bergmann]

The rds_iw_add_conn function stores a large 'struct rds_sock'
object on the stack in order to pass a pair of addresses. This
happens to just fit withint the 1024 byte stack size warning
limit on x86, but just exceed that limit on ARM, which gives
us this warning:

net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]

The warning is correct in principle, though unlikely to be
related to a serious problem.

As the use of this large variable is basically bogus, we can
rearrange the code to not do that. Instead of passing an
rds socket into rds_iw_get_device, we now just pass the two
addresses that we have available in rds_iw_update_cm_id, and
we change rds_iw_get_mr accordingly, to create two address
structures on the stack there.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/net/rds/iw_rdma.c b/net/rds/iw_rdma.c
index a817705ce2d0..dba8d0864f18 100644
--- a/net/rds/iw_rdma.c
+++ b/net/rds/iw_rdma.c
@@ -88,7 +88,9 @@ static unsigned int rds_iw_unmap_fastreg_list(struct rds_iw_mr_pool *pool,
 			int *unpinned);
 static void rds_iw_destroy_fastreg(struct rds_iw_mr_pool *pool, struct rds_iw_mr *ibmr);
 
-static int rds_iw_get_device(struct rds_sock *rs, struct rds_iw_device **rds_iwdev, struct rdma_cm_id **cm_id)
+static int rds_iw_get_device(struct sockaddr_in *src, struct sockaddr_in *dst,
+			     struct rds_iw_device **rds_iwdev,
+			     struct rdma_cm_id **cm_id)
 {
 	struct rds_iw_device *iwdev;
 	struct rds_iw_cm_id *i_cm_id;
@@ -112,15 +114,15 @@ static int rds_iw_get_device(struct rds_sock *rs, struct rds_iw_device **rds_iwd
 				src_addr->sin_port,
 				dst_addr->sin_addr.s_addr,
 				dst_addr->sin_port,
-				rs->rs_bound_addr,
-				rs->rs_bound_port,
-				rs->rs_conn_addr,
-				rs->rs_conn_port);
+				src->sin_addr.s_addr,
+				src->sin_port,
+				dst->sin_addr.s_addr,
+				dst->sin_port);
 #ifdef WORKING_TUPLE_DETECTION
-			if (src_addr->sin_addr.s_addr == rs->rs_bound_addr &&
-			    src_addr->sin_port == rs->rs_bound_port &&
-			    dst_addr->sin_addr.s_addr == rs->rs_conn_addr &&
-			    dst_addr->sin_port == rs->rs_conn_port) {
+			if (src_addr->sin_addr.s_addr == src->sin_addr.s_addr &&
+			    src_addr->sin_port == src->sin_port &&
+			    dst_addr->sin_addr.s_addr == dst->sin_addr.s_addr &&
+			    dst_addr->sin_port == dst->sin_port) {
 #else
 			/* FIXME - needs to compare the local and remote
 			 * ipaddr/port tuple, but the ipaddr is the only
@@ -128,7 +130,7 @@ static int rds_iw_get_device(struct rds_sock *rs, struct rds_iw_device **rds_iwd
 			 * zero'ed.  It doesn't appear to be properly populated
 			 * during connection setup...
 			 */
-			if (src_addr->sin_addr.s_addr == rs->rs_bound_addr) {
+			if (src_addr->sin_addr.s_addr == src->sin_addr.s_addr) {
 #endif
 				spin_unlock_irq(&iwdev->spinlock);
 				*rds_iwdev = iwdev;
@@ -180,19 +182,13 @@ int rds_iw_update_cm_id(struct rds_iw_device *rds_iwdev, struct rdma_cm_id *cm_i
 {
 	struct sockaddr_in *src_addr, *dst_addr;
 	struct rds_iw_device *rds_iwdev_old;
-	struct rds_sock rs;
 	struct rdma_cm_id *pcm_id;
 	int rc;
 
 	src_addr = (struct sockaddr_in *)&cm_id->route.addr.src_addr;
 	dst_addr = (struct sockaddr_in *)&cm_id->route.addr.dst_addr;
 
-	rs.rs_bound_addr = src_addr->sin_addr.s_addr;
-	rs.rs_bound_port = src_addr->sin_port;
-	rs.rs_conn_addr = dst_addr->sin_addr.s_addr;
-	rs.rs_conn_port = dst_addr->sin_port;
-
-	rc = rds_iw_get_device(&rs, &rds_iwdev_old, &pcm_id);
+	rc = rds_iw_get_device(src_addr, dst_addr, &rds_iwdev_old, &pcm_id);
 	if (rc)
 		rds_iw_remove_cm_id(rds_iwdev, cm_id);
 
@@ -598,9 +594,17 @@ void *rds_iw_get_mr(struct scatterlist *sg, unsigned long nents,
 	struct rds_iw_device *rds_iwdev;
 	struct rds_iw_mr *ibmr = NULL;
 	struct rdma_cm_id *cm_id;
+	struct sockaddr_in src = {
+		.sin_addr.s_addr = rs->rs_bound_addr,
+		.sin_port = rs->rs_bound_port,
+	};
+	struct sockaddr_in dst = {
+		.sin_addr.s_addr = rs->rs_conn_addr,
+		.sin_port = rs->rs_conn_port,
+	};
 	int ret;
 
-	ret = rds_iw_get_device(rs, &rds_iwdev, &cm_id);
+	ret = rds_iw_get_device(&src, &dst, &rds_iwdev, &cm_id);
 	if (ret || !cm_id) {
 		ret = -ENODEV;
 		goto out;

Re: [PATCH] rds: avoid potential stack overflow

[Sowmini Varadhan]

On 03/09/2015 08:06 AM, Arnd Bergmann wrote:
> The rds_iw_add_conn function stores a large 'struct rds_sock'

I think you might have a typo here- did you mean
rds_iw_update_cm_id above (which is the function that has
a 'struct rds_sock rs' on the stack)?

The rest of the change looks fine to me.

--Sowmini

> object on the stack in order to pass a pair of addresses. This
> happens to just fit withint the 1024 byte stack size warning
> limit on x86, but just exceed that limit on ARM, which gives
> us this warning:
>
> net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> The warning is correct in principle, though unlikely to be
> related to a serious problem.
>
> As the use of this large variable is basically bogus, we can
> rearrange the code to not do that. Instead of passing an
> rds socket into rds_iw_get_device, we now just pass the two
> addresses that we have available in rds_iw_update_cm_id, and
> we change rds_iw_get_mr accordingly, to create two address
> structures on the stack there.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/net/rds/iw_rdma.c b/net/rds/iw_rdma.c
> index a817705ce2d0..dba8d0864f18 100644
> --- a/net/rds/iw_rdma.c
> +++ b/net/rds/iw_rdma.c
> @@ -88,7 +88,9 @@ static unsigned int rds_iw_unmap_fastreg_list(struct rds_iw_mr_pool *pool,
>   			int *unpinned);
>   static void rds_iw_destroy_fastreg(struct rds_iw_mr_pool *pool, struct rds_iw_mr *ibmr);
>
> -static int rds_iw_get_device(struct rds_sock *rs, struct rds_iw_device **rds_iwdev, struct rdma_cm_id **cm_id)
> +static int rds_iw_get_device(struct sockaddr_in *src, struct sockaddr_in *dst,
> +			     struct rds_iw_device **rds_iwdev,
> +			     struct rdma_cm_id **cm_id)
>   {
>   	struct rds_iw_device *iwdev;
>   	struct rds_iw_cm_id *i_cm_id;
> @@ -112,15 +114,15 @@ static int rds_iw_get_device(struct rds_sock *rs, struct rds_iw_device **rds_iwd
>   				src_addr->sin_port,
>   				dst_addr->sin_addr.s_addr,
>   				dst_addr->sin_port,
> -				rs->rs_bound_addr,
> -				rs->rs_bound_port,
> -				rs->rs_conn_addr,
> -				rs->rs_conn_port);
> +				src->sin_addr.s_addr,
> +				src->sin_port,
> +				dst->sin_addr.s_addr,
> +				dst->sin_port);
>   #ifdef WORKING_TUPLE_DETECTION
> -			if (src_addr->sin_addr.s_addr == rs->rs_bound_addr &&
> -			    src_addr->sin_port == rs->rs_bound_port &&
> -			    dst_addr->sin_addr.s_addr == rs->rs_conn_addr &&
> -			    dst_addr->sin_port == rs->rs_conn_port) {
> +			if (src_addr->sin_addr.s_addr == src->sin_addr.s_addr &&
> +			    src_addr->sin_port == src->sin_port &&
> +			    dst_addr->sin_addr.s_addr == dst->sin_addr.s_addr &&
> +			    dst_addr->sin_port == dst->sin_port) {
>   #else
>   			/* FIXME - needs to compare the local and remote
>   			 * ipaddr/port tuple, but the ipaddr is the only
> @@ -128,7 +130,7 @@ static int rds_iw_get_device(struct rds_sock *rs, struct rds_iw_device **rds_iwd
>   			 * zero'ed.  It doesn't appear to be properly populated
>   			 * during connection setup...
>   			 */
> -			if (src_addr->sin_addr.s_addr == rs->rs_bound_addr) {
> +			if (src_addr->sin_addr.s_addr == src->sin_addr.s_addr) {
>   #endif
>   				spin_unlock_irq(&iwdev->spinlock);
>   				*rds_iwdev = iwdev;
> @@ -180,19 +182,13 @@ int rds_iw_update_cm_id(struct rds_iw_device *rds_iwdev, struct rdma_cm_id *cm_i
>   {
>   	struct sockaddr_in *src_addr, *dst_addr;
>   	struct rds_iw_device *rds_iwdev_old;
> -	struct rds_sock rs;
>   	struct rdma_cm_id *pcm_id;
>   	int rc;
>
>   	src_addr = (struct sockaddr_in *)&cm_id->route.addr.src_addr;
>   	dst_addr = (struct sockaddr_in *)&cm_id->route.addr.dst_addr;
>
> -	rs.rs_bound_addr = src_addr->sin_addr.s_addr;
> -	rs.rs_bound_port = src_addr->sin_port;
> -	rs.rs_conn_addr = dst_addr->sin_addr.s_addr;
> -	rs.rs_conn_port = dst_addr->sin_port;
> -
> -	rc = rds_iw_get_device(&rs, &rds_iwdev_old, &pcm_id);
> +	rc = rds_iw_get_device(src_addr, dst_addr, &rds_iwdev_old, &pcm_id);
>   	if (rc)
>   		rds_iw_remove_cm_id(rds_iwdev, cm_id);
>
> @@ -598,9 +594,17 @@ void *rds_iw_get_mr(struct scatterlist *sg, unsigned long nents,
>   	struct rds_iw_device *rds_iwdev;
>   	struct rds_iw_mr *ibmr = NULL;
>   	struct rdma_cm_id *cm_id;
> +	struct sockaddr_in src = {
> +		.sin_addr.s_addr = rs->rs_bound_addr,
> +		.sin_port = rs->rs_bound_port,
> +	};
> +	struct sockaddr_in dst = {
> +		.sin_addr.s_addr = rs->rs_conn_addr,
> +		.sin_port = rs->rs_conn_port,
> +	};
>   	int ret;
>
> -	ret = rds_iw_get_device(rs, &rds_iwdev, &cm_id);
> +	ret = rds_iw_get_device(&src, &dst, &rds_iwdev, &cm_id);
>   	if (ret || !cm_id) {
>   		ret = -ENODEV;
>   		goto out;
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] rds: avoid potential stack overflow

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 09 Mar 2015 13:06:52 +0100

> The rds_iw_add_conn function stores a large 'struct rds_sock' object
> on the stack in order to pass a pair of addresses.

As Sowmini pointed out, this function is not the top-level
guilty one, it's rds_iw_update_cm_id.

Please respin this with a corrected commit message, thanks.