Re: [PATCH 01/40] SUNRPC: Fix return type of svc_addr_len()

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Mar 12, 2009 at 12:07:06PM -0400, Chuck Lever wrote:
> The svc_addr_len() helper function can return a negative errno value,
> but its return type is size_t, which is unsigned.
> 
> The RDMA transport code passes this return value directly to memset(),
> without checking first if it's negative.  This could cause memset() to
> clobber a large piece of memory if svc_addr_len() has returned an
> error.

Is this something that can in fact happen, and if so, in what
circumstances?  And what ends up happening to the -EAFNOSUPPORT error?

Tracing up through the callers, I get lost somewhere in the ib code.
Maybe Tom can rescue me....

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  include/linux/sunrpc/svc_xprt.h          |    3 ++-
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |   16 ++++++++++++++--
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index 0127dac..c2aa8cd 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -113,7 +113,7 @@ static inline unsigned short svc_addr_port(struct sockaddr *sa)
>  	return ret;
>  }
>  
> -static inline size_t svc_addr_len(struct sockaddr *sa)
> +static inline int svc_addr_len(const struct sockaddr *sa)
>  {
>  	switch (sa->sa_family) {
>  	case AF_INET:
> @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct sockaddr *sa)
>  	case AF_INET6:
>  		return sizeof(struct sockaddr_in6);
>  	}
> +
>  	return -EAFNOSUPPORT;
>  }
>  
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 3d810e7..d1ec6f9 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>  	struct svcxprt_rdma *listen_xprt = new_cma_id->context;
>  	struct svcxprt_rdma *newxprt;
>  	struct sockaddr *sa;
> +	int len;
>  
>  	/* Create a new transport */
>  	newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0);
> @@ -563,9 +564,20 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
>  
>  	/* Set the local and remote addresses in the transport */
>  	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
> -	svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa));
> +	len = svc_addr_len(sa);
> +	if (len < 0) {
> +		dprintk("svcrdma: dst_addr has a bad address family\n");
> +		return;
> +	}
> +	svc_xprt_set_remote(&newxprt->sc_xprt, sa, len);
> +
>  	sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
> -	svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa));
> +	len = svc_addr_len(sa);
> +	if (len < 0) {
> +		dprintk("svcrdma: src_addr has a bad address family\n");
> +		return;
> +	}
> +	svc_xprt_set_local(&newxprt->sc_xprt, sa, len);
>  
>  	/*
>  	 * Enqueue the new transport on the accept queue of the listening
>