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@xxxxxxxxxx>
> ---
>
> 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;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
