| Message ID | 20231114175407.work.410-kees@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | cb6d2fd30dddd00499333e9475f8b11bbd84f37c |
| Headers | show |
| Series | SUNRPC: Replace strlcpy() with strscpy() | expand |
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 <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#strlcpy [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(-) > > 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 <neilb@suse.de> 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 > >
On Tue, 14 Nov 2023 09:54:18 -0800, 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(). > > [...] Applied to for-next/hardening, thanks! [1/1] SUNRPC: Replace strlcpy() with strscpy() https://git.kernel.org/kees/c/cb6d2fd30ddd Take care,
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; } static int rpc_client_register(struct rpc_clnt *clnt,