Skip to contentSkip to conversation

SUNRPC: Replace strlcpy() with strscpy()

PATCH Applied Kees Cook 2 participants 3 messages Nov 14, 2023 – Nov 30, 2023

Subsystems (4)
Networking Network drivers NFS client NFS server
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 SUNRPC: Replace strlcpy() with strscpy()

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(-)
net/sunrpc/clnt.c +8 -2
index daa9582ec861..7afe02bdea4a 100644
@@ -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;
 }
 
 static int rpc_client_register(struct rpc_clnt *clnt,
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 47 lines · Kees Cook

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(-)
net/sunrpc/clnt.c +0 -1
index daa9582ec861..7afe02bdea4a 100644
@@ -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,

net/sunrpc/clnt.c static struct rpc_xprt *rpc_clnt_set_transport(struct rpc_clnt *clnt, · line 298

Quoted 10 lines
net/sunrpc/clnt.c +8 -1
-			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

Quoted 6 lines
net/sunrpc/clnt.c +0 -0
 static int rpc_client_register(struct rpc_clnt *clnt,
Signature · 1 line
2.34.1
author Applied
#2 lore raw
In reply to Kees Cook
Quoted 8 lines · Kees Cook

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().

[...]

Applied to for-next/hardening, thanks!

[1/1] SUNRPC: Replace strlcpy() with strscpy()

      https://git.kernel.org/kees/c/cb6d2fd30ddd

Take care,

Signature · 1 line
Kees Cook