lkml.org 
[lkml]   [2020]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V2] SUNRPC: Fix a potential buffer overflow in 'svc_print_xprts()'
From
Date


> On Mar 27, 2020, at 12:15 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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@wanadoo.fr>
> ---
> 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



\
 
 \ /
  Last update: 2020-03-27 17:34    [W:0.054 / U:1.556 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site