Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Kees Cook" <keescook@chromium.org>
Cc:     "Trond Myklebust" <trond.myklebust@hammerspace.com>,
        "Kees Cook" <keescook@chromium.org>,
        "Anna Schumaker" <anna@kernel.org>,
        "Chuck Lever" <chuck.lever@oracle.com>,
        "Jeff Layton" <jlayton@kernel.org>,
        "Olga Kornievskaia" <kolga@netapp.com>,
        "Dai Ngo" <Dai.Ngo@oracle.com>, "Tom Talpey" <tom@talpey.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Eric Dumazet" <edumazet@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>,
        "Paolo Abeni" <pabeni@redhat.com>, linux-nfs@vger.kernel.org,
        netdev@vger.kernel.org, "Azeem Shaikh" <azeemshaikh38@gmail.com>,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] SUNRPC: Replace strlcpy() with strscpy()
In-reply-to: <20231114175407.work.410-kees@kernel.org>
References: <20231114175407.work.410-kees@kernel.org>
Date:   Fri, 17 Nov 2023 13:53:15 +1100
Message-id: <170018959595.19300.7615621918111285329@noble.neil.brown.name>
Precedence: bulk

On Wed, 15 Nov 2023, Kees Cook wrote:
> strlcpy() reads the entire source buffer first. This read may exceed
> the destination size limit. This is both inefficient and can lead
> to linear read overflows if a source string is not NUL-terminated[1].
> Additionally, it returns the size of the source string, not the
> resulting size of the destination string. In an effort to remove strlcpy()
> completely[2], replace strlcpy() here with strscpy().
>=20
> Explicitly handle the truncation case by returning the size of the
> resulting string.
>=20
> If "nodename" was ever longer than sizeof(clnt->cl_nodename) - 1, this
> change will fix a bug where clnt->cl_nodelen would end up thinking there
> were more characters in clnt->cl_nodename than there actually were,
> which might have lead to kernel memory content exposures.
>=20
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Neil Brown <neilb@suse.de>
> Cc: Olga Kornievskaia <kolga@netapp.com>
> Cc: Dai Ngo <Dai.Ngo@oracle.com>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: linux-nfs@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcp=
y [1]
> Link: https://github.com/KSPP/linux/issues/89 [2]
> Co-developed-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  net/sunrpc/clnt.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>=20
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index daa9582ec861..7afe02bdea4a 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -287,8 +287,14 @@ static struct rpc_xprt *rpc_clnt_set_transport(struct =
rpc_clnt *clnt,
> =20
>  static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *noden=
ame)
>  {
> -	clnt->cl_nodelen =3D strlcpy(clnt->cl_nodename,
> -			nodename, sizeof(clnt->cl_nodename));
> +	ssize_t copied;
> +
> +	copied =3D strscpy(clnt->cl_nodename,
> +			 nodename, sizeof(clnt->cl_nodename));
> +
> +	clnt->cl_nodelen =3D copied < 0
> +				? sizeof(clnt->cl_nodename) - 1
> +				: copied;
>  }

Reviewed-by: NeilBrown <neilb@suse.de>

If it were "copied =3D=3D -E2BIG" instead of "copied < 0" it would be more
obvious why sizeof(...) is used in that case.
But we really want to do something sensible for *any* error message that
might be added in the future.. I guess.

Thanks,
NeilBrown


> =20
>  static int rpc_client_register(struct rpc_clnt *clnt,
> --=20
> 2.34.1
>=20
>=20