linux-kernel - Re: [PATCH 2/2] SUNRPC: Optimize 'svc_print

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Mar 2020 10:52:44 -0400
From:   Chuck Lever <chuck.lever@...cle.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     trond.myklebust@...merspace.com,
        Anna Schumaker <anna.schumaker@...app.com>,
        Bruce Fields <bfields@...ldses.org>, davem@...emloft.net,
        kuba@...nel.org, gnb@....com, Neil Brown <neilb@...e.de>,
        Tom Tucker <tom@...ngridcomputing.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/2] SUNRPC: Optimize 'svc_print_xprts()'

Hi Christophe,


> On Mar 25, 2020, at 3:04 AM, Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
> 
> Using 'snprintf' is safer than 'sprintf' because it can avoid a buffer
> overflow.

That's true as a general statement, but how likely is such an
overflow to occur here?


> The return value can also be used to avoid a strlen a call.

That's also true of sprintf, isn't it?


> Finally, we know where we need to copy and the length to copy, so, we
> can save a few cycles by rearraging the code and using a memcpy instead of
> a strcat.

I would be OK with squashing these two patches together. I don't
see the need to keep the two changes separated.


> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> This patch should have no functionnal change.
> We could go further, use scnprintf and write directly in the destination
> buffer. However, this could lead to a truncated last line.

That's exactly what this function is trying to avoid. As part of any
change in this area, it would be good to replace the current block
comment before this function with a Doxygen-format comment that
documents that goal.


> ---
> net/sunrpc/svc_xprt.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index df39e7b8b06c..6df861650040 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -118,12 +118,12 @@ 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;
> +		memcpy(buf + len, tmpstr, slen + 1);
> 		len += slen;
> -		strcat(buf, tmpstr);

IMO replacing the strcat makes the code harder to read, and this
is certainly not a performance path. Can you drop that part of the
patch?


> 	}
> 	spin_unlock(&svc_xprt_class_lock);
> 
> -- 
> 2.20.1
> 

--
Chuck Lever