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().
>
> Explicitly handle the truncation case by returning the size of the
> resulting string.
>
> 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.
>
> Cc: Trond Myklebust <[email protected]>
> Cc: Anna Schumaker <[email protected]>
> Cc: Chuck Lever <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Cc: Neil Brown <[email protected]>
> Cc: Olga Kornievskaia <[email protected]>
> Cc: Dai Ngo <[email protected]>
> Cc: Tom Talpey <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1]
> Link: https://github.com/KSPP/linux/issues/89 [2]
> Co-developed-by: Azeem Shaikh <[email protected]>
> Signed-off-by: Azeem Shaikh <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> net/sunrpc/clnt.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> 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,
>
> static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *nodename)
> {
> - clnt->cl_nodelen = strlcpy(clnt->cl_nodename,
> - nodename, sizeof(clnt->cl_nodename));
> + ssize_t copied;
> +
> + copied = strscpy(clnt->cl_nodename,
> + nodename, sizeof(clnt->cl_nodename));
> +
> + clnt->cl_nodelen = copied < 0
> + ? sizeof(clnt->cl_nodename) - 1
> + : copied;
> }
Reviewed-by: NeilBrown <[email protected]>
If it were "copied == -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
>
> static int rpc_client_register(struct rpc_clnt *clnt,
> --
> 2.34.1
>
>