Re: [PATCH 09/19] NFSD: Prevent a buffer overflow in svc_xprt_names()

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

On Thu, Apr 23, 2009 at 07:32:25PM -0400, Chuck Lever wrote:
> The svc_xprt_names() function can overflow its buffer if it's so near
> the end of the passed in buffer that the "name too long" string still
> doesn't fit.  Of course, it could never tell if it was near the end
> of the passed in buffer, since its only caller passes in zero as the
> buffer length.
> 
> Let's make this API a little safer.
> 
> Change svc_xprt_names() so it *always* checks for a buffer overflow,
> and change its only caller to pass in the correct buffer length.
> 
> If svc_xprt_names() does overflow its buffer, it now fails with an
> ENAMETOOLONG errno, instead of trying to write a message at the end
> of the buffer.  I don't like this much, but I can't figure out a clean
> way that's always safe to return some of the names, *and* an
> indication that the buffer was not long enough.
> 
> The displayed error when doing a 'cat /proc/fs/nfsd/portlist' is
> "File name too long".

Also worth noting that the buffer in question is nearly a page, which
makes this a bit academic for now.  (Not an objection to the patch, just
something to note.)

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfsd/nfsctl.c                |    2 +
>  include/linux/sunrpc/svc_xprt.h |    2 +
>  net/sunrpc/svc_xprt.c           |   56 +++++++++++++++++++++++++++------------
>  3 files changed, 41 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index e051847..6a1cd90 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -918,7 +918,7 @@ static ssize_t __write_ports_names(char *buf)
>  {
>  	if (nfsd_serv == NULL)
>  		return 0;
> -	return svc_xprt_names(nfsd_serv, buf, 0);
> +	return svc_xprt_names(nfsd_serv, buf, SIMPLE_TRANSACTION_LIMIT);
>  }
>  
>  /*
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index d790c52..2223ae0 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -83,7 +83,7 @@ int	svc_port_is_privileged(struct sockaddr *sin);
>  int	svc_print_xprts(char *buf, int maxlen);
>  struct	svc_xprt *svc_find_xprt(struct svc_serv *serv, const char *xcl_name,
>  			const sa_family_t af, const unsigned short port);
> -int	svc_xprt_names(struct svc_serv *serv, char *buf, int buflen);
> +int	svc_xprt_names(struct svc_serv *serv, char *buf, const int buflen);
>  
>  static inline void svc_xprt_get(struct svc_xprt *xprt)
>  {
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index c200d92..96eb873 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -1097,36 +1097,58 @@ struct svc_xprt *svc_find_xprt(struct svc_serv *serv, const char *xcl_name,
>  }
>  EXPORT_SYMBOL_GPL(svc_find_xprt);
>  
> -/*
> - * Format a buffer with a list of the active transports. A zero for
> - * the buflen parameter disables target buffer overflow checking.
> +static int svc_one_xprt_name(const struct svc_xprt *xprt,
> +			     char *pos, int remaining)
> +{
> +	int len;
> +
> +	len = snprintf(pos, remaining, "%s %u\n",
> +			xprt->xpt_class->xcl_name,
> +			svc_xprt_local_port(xprt));
> +	if (len >= remaining)
> +		return -ENAMETOOLONG;
> +	return len;
> +}
> +
> +/**
> + * svc_xprt_names - format a buffer with a list of transport names
> + * @serv: pointer to an RPC service
> + * @buf: pointer to a buffer to be filled in
> + * @buflen: length of buffer to be filled in
> + *
> + * Fills in @buf with a string containing a list of transport names,
> + * each name terminated with '\n'.
> + *
> + * Returns positive length of the filled-in string on success; otherwise
> + * a negative errno value is returned if an error occurs.
>   */
> -int svc_xprt_names(struct svc_serv *serv, char *buf, int buflen)
> +int svc_xprt_names(struct svc_serv *serv, char *buf, const int buflen)
>  {
>  	struct svc_xprt *xprt;
> -	char xprt_str[64];
> -	int totlen = 0;
> -	int len;
> +	int len, totlen;
> +	char *pos;
>  
>  	/* Sanity check args */
>  	if (!serv)
>  		return 0;
>  
>  	spin_lock_bh(&serv->sv_lock);
> +
> +	pos = buf;
> +	totlen = 0;
>  	list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
> -		len = snprintf(xprt_str, sizeof(xprt_str),
> -			       "%s %d\n", xprt->xpt_class->xcl_name,
> -			       svc_xprt_local_port(xprt));
> -		/* If the string was truncated, replace with error string */
> -		if (len >= sizeof(xprt_str))
> -			strcpy(xprt_str, "name-too-long\n");
> -		/* Don't overflow buffer */
> -		len = strlen(xprt_str);
> -		if (buflen && (len + totlen >= buflen))
> +		len = svc_one_xprt_name(xprt, pos, buflen - totlen);
> +		if (len < 0) {
> +			*buf = '\0';
> +			totlen = len;
> +		}
> +		if (len <= 0)
>  			break;
> -		strcpy(buf+totlen, xprt_str);
> +
> +		pos += len;
>  		totlen += len;
>  	}
> +
>  	spin_unlock_bh(&serv->sv_lock);
>  	return totlen;
>  }
>