> On Mar 27, 2020, at 12:15 PM, Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
>
> 'maxlen' is the total size of the destination buffer. There is only one
> caller and this value is 256.
>
> When we compute the size already used and what we would like to add in
> the buffer, the trailling NULL character is not taken into account.
> However, this trailling character will be added by the 'strcat' once we
> have checked that we have enough place.
>
> So, there is a off-by-one issue and 1 byte of the stack could be
> erroneously overwridden.
>
> Take into account the trailling NULL, when checking if there is enough
> place in the destination buffer.
>
>
> While at it, also replace a 'sprintf' by a safer 'snprintf', check for
> output truncation and avoid a superfluous 'strlen'.
>
> Fixes: dc9a16e49dbba ("svc: Add /proc/sys/sunrpc/transport files")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> ---
> V2: add a doxygen comment to clarify the goal of the function
> merge previous 2 patches into a single one
> keep strcat for clarity, this function being just a slow path anyway
>
> Doc being most of the time a matter of taste, please adjust the description
> as needed.
I've applied this to my local nfsd-5.7 with a very small adjustment
to the doc-comment. Testing it now. Thanks, all.
> ---
> net/sunrpc/svc_xprt.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index de3c077733a7..e0f61a8c1965 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -104,8 +104,17 @@ void svc_unreg_xprt_class(struct svc_xprt_class *xcl)
> }
> EXPORT_SYMBOL_GPL(svc_unreg_xprt_class);
>
> -/*
> - * Format the transport list for printing
> +/**
> + * svc_print_xprts - Format the transport list for printing
> + * @buf: target buffer for formatted address
> + * @maxlen: length of target buffer
> + *
> + * Fills in @buf with a string containing a list of transport names, each name
> + * terminated with '\n'. If the buffer is too small, some entries may be
> + * missing, but it is guaranteed that the line in the output buffer are
> + * complete.
> + *
> + * Returns positive length of the filled-in string.
> */
> int svc_print_xprts(char *buf, int maxlen)
> {
> @@ -118,9 +127,9 @@ int svc_print_xprts(char *buf, int maxlen)
> list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
> int slen;
>
> - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload);
> - slen = strlen(tmpstr);
> - if (len + slen > maxlen)
> + slen = snprintf(tmpstr, sizeof(tmpstr), "%s %d\n",
> + xcl->xcl_name, xcl->xcl_max_payload);
> + if (slen >= sizeof(tmpstr) || len + slen >= maxlen)
> break;
> len += slen;
> strcat(buf, tmpstr);
> --
> 2.20.1
>
--
Chuck Lever
