* [PATCH] NFS: Avoid memcpy() run-time warning for struct sockaddr overflows
@ 2022-10-11 6:52 Kees Cook
2022-10-11 15:25 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2022-10-11 6:52 UTC (permalink / raw)
To: Trond Myklebust
Cc: Kees Cook, kernel test robot, Anna Schumaker, linux-nfs,
linux-kernel, linux-hardening
The 'nfs_server' and 'mount_server' structures include a union of
'struct sockaddr' (with the older 16 bytes max address size) and
'struct sockaddr_storage' which is large enough to hold all the supported
sa_family types (128 bytes max size). The runtime memcpy() buffer overflow
checker is seeing attempts to write beyond the 16 bytes as an overflow,
but the actual expected size is that of 'struct sockaddr_storage'. Adjust
the pointers to the correct union member. Avoids this false positive
run-time warning under CONFIG_FORTIFY_SOURCE:
memcpy: detected field-spanning write (size 28) of single field "&ctx->nfs_server.address" at fs/nfs/namespace.c:178 (size 16)
Reported-by: kernel test robot <yujie.liu@intel.com>
Link: https://lore.kernel.org/all/202210110948.26b43120-yujie.liu@intel.com
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna@kernel.org>
Cc: linux-nfs@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/nfs/fs_context.c | 2 +-
fs/nfs/namespace.c | 2 +-
fs/nfs/super.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 4da701fd1424..bffa31bb35b9 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -1540,7 +1540,7 @@ static int nfs_init_fs_context(struct fs_context *fc)
ctx->version = nfss->nfs_client->rpc_ops->version;
ctx->minorversion = nfss->nfs_client->cl_minorversion;
- memcpy(&ctx->nfs_server.address, &nfss->nfs_client->cl_addr,
+ memcpy(&ctx->nfs_server._address, &nfss->nfs_client->cl_addr,
ctx->nfs_server.addrlen);
if (fc->net_ns != net) {
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 3295af4110f1..2f336ace7555 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -175,7 +175,7 @@ struct vfsmount *nfs_d_automount(struct path *path)
}
/* for submounts we want the same server; referrals will reassign */
- memcpy(&ctx->nfs_server.address, &client->cl_addr, client->cl_addrlen);
+ memcpy(&ctx->nfs_server._address, &client->cl_addr, client->cl_addrlen);
ctx->nfs_server.addrlen = client->cl_addrlen;
ctx->nfs_server.port = server->port;
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 82944e14fcea..8ea7dfdea427 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -823,7 +823,7 @@ static int nfs_request_mount(struct fs_context *fc,
struct nfs_fs_context *ctx = nfs_fc2context(fc);
struct nfs_mount_request request = {
.sap = (struct sockaddr *)
- &ctx->mount_server.address,
+ &ctx->mount_server._address,
.dirpath = ctx->nfs_server.export_path,
.protocol = ctx->mount_server.protocol,
.fh = root_fh,
@@ -854,7 +854,7 @@ static int nfs_request_mount(struct fs_context *fc,
* Construct the mount server's address.
*/
if (ctx->mount_server.address.sa_family == AF_UNSPEC) {
- memcpy(request.sap, &ctx->nfs_server.address,
+ memcpy(request.sap, &ctx->nfs_server._address,
ctx->nfs_server.addrlen);
ctx->mount_server.addrlen = ctx->nfs_server.addrlen;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] NFS: Avoid memcpy() run-time warning for struct sockaddr overflows
2022-10-11 6:52 [PATCH] NFS: Avoid memcpy() run-time warning for struct sockaddr overflows Kees Cook
@ 2022-10-11 15:25 ` Gustavo A. R. Silva
2022-10-11 16:38 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2022-10-11 15:25 UTC (permalink / raw)
To: Kees Cook
Cc: Trond Myklebust, kernel test robot, Anna Schumaker, linux-nfs,
linux-kernel, linux-hardening
On Mon, Oct 10, 2022 at 11:52:43PM -0700, Kees Cook wrote:
> The 'nfs_server' and 'mount_server' structures include a union of
> 'struct sockaddr' (with the older 16 bytes max address size) and
> 'struct sockaddr_storage' which is large enough to hold all the supported
> sa_family types (128 bytes max size). The runtime memcpy() buffer overflow
> checker is seeing attempts to write beyond the 16 bytes as an overflow,
> but the actual expected size is that of 'struct sockaddr_storage'. Adjust
> the pointers to the correct union member. Avoids this false positive
> run-time warning under CONFIG_FORTIFY_SOURCE:
>
> memcpy: detected field-spanning write (size 28) of single field "&ctx->nfs_server.address" at fs/nfs/namespace.c:178 (size 16)
>
> Reported-by: kernel test robot <yujie.liu@intel.com>
> Link: https://lore.kernel.org/all/202210110948.26b43120-yujie.liu@intel.com
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: linux-nfs@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
--
Gustavo
> ---
> fs/nfs/fs_context.c | 2 +-
> fs/nfs/namespace.c | 2 +-
> fs/nfs/super.c | 4 ++--
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 4da701fd1424..bffa31bb35b9 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -1540,7 +1540,7 @@ static int nfs_init_fs_context(struct fs_context *fc)
> ctx->version = nfss->nfs_client->rpc_ops->version;
> ctx->minorversion = nfss->nfs_client->cl_minorversion;
>
> - memcpy(&ctx->nfs_server.address, &nfss->nfs_client->cl_addr,
> + memcpy(&ctx->nfs_server._address, &nfss->nfs_client->cl_addr,
> ctx->nfs_server.addrlen);
>
> if (fc->net_ns != net) {
> diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> index 3295af4110f1..2f336ace7555 100644
> --- a/fs/nfs/namespace.c
> +++ b/fs/nfs/namespace.c
> @@ -175,7 +175,7 @@ struct vfsmount *nfs_d_automount(struct path *path)
> }
>
> /* for submounts we want the same server; referrals will reassign */
> - memcpy(&ctx->nfs_server.address, &client->cl_addr, client->cl_addrlen);
> + memcpy(&ctx->nfs_server._address, &client->cl_addr, client->cl_addrlen);
> ctx->nfs_server.addrlen = client->cl_addrlen;
> ctx->nfs_server.port = server->port;
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 82944e14fcea..8ea7dfdea427 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -823,7 +823,7 @@ static int nfs_request_mount(struct fs_context *fc,
> struct nfs_fs_context *ctx = nfs_fc2context(fc);
> struct nfs_mount_request request = {
> .sap = (struct sockaddr *)
> - &ctx->mount_server.address,
> + &ctx->mount_server._address,
> .dirpath = ctx->nfs_server.export_path,
> .protocol = ctx->mount_server.protocol,
> .fh = root_fh,
> @@ -854,7 +854,7 @@ static int nfs_request_mount(struct fs_context *fc,
> * Construct the mount server's address.
> */
> if (ctx->mount_server.address.sa_family == AF_UNSPEC) {
> - memcpy(request.sap, &ctx->nfs_server.address,
> + memcpy(request.sap, &ctx->nfs_server._address,
> ctx->nfs_server.addrlen);
> ctx->mount_server.addrlen = ctx->nfs_server.addrlen;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] NFS: Avoid memcpy() run-time warning for struct sockaddr overflows
2022-10-11 15:25 ` Gustavo A. R. Silva
@ 2022-10-11 16:38 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2022-10-11 16:38 UTC (permalink / raw)
To: Gustavo A. R. Silva, Trond Myklebust
Cc: kernel test robot, Anna Schumaker, linux-nfs, linux-kernel,
linux-hardening
On Tue, Oct 11, 2022 at 10:25:16AM -0500, Gustavo A. R. Silva wrote:
> On Mon, Oct 10, 2022 at 11:52:43PM -0700, Kees Cook wrote:
> > The 'nfs_server' and 'mount_server' structures include a union of
> > 'struct sockaddr' (with the older 16 bytes max address size) and
> > 'struct sockaddr_storage' which is large enough to hold all the supported
> > sa_family types (128 bytes max size). The runtime memcpy() buffer overflow
> > checker is seeing attempts to write beyond the 16 bytes as an overflow,
> > but the actual expected size is that of 'struct sockaddr_storage'. Adjust
> > the pointers to the correct union member. Avoids this false positive
> > run-time warning under CONFIG_FORTIFY_SOURCE:
> >
> > memcpy: detected field-spanning write (size 28) of single field "&ctx->nfs_server.address" at fs/nfs/namespace.c:178 (size 16)
> >
> > Reported-by: kernel test robot <yujie.liu@intel.com>
> > Link: https://lore.kernel.org/all/202210110948.26b43120-yujie.liu@intel.com
> > Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> > Cc: Anna Schumaker <anna@kernel.org>
> > Cc: linux-nfs@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
> > ---
> > fs/nfs/fs_context.c | 2 +-
> > fs/nfs/namespace.c | 2 +-
> > fs/nfs/super.c | 4 ++--
> > 3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> > index 4da701fd1424..bffa31bb35b9 100644
> > --- a/fs/nfs/fs_context.c
> > +++ b/fs/nfs/fs_context.c
> > @@ -1540,7 +1540,7 @@ static int nfs_init_fs_context(struct fs_context *fc)
> > ctx->version = nfss->nfs_client->rpc_ops->version;
> > ctx->minorversion = nfss->nfs_client->cl_minorversion;
> >
> > - memcpy(&ctx->nfs_server.address, &nfss->nfs_client->cl_addr,
> > + memcpy(&ctx->nfs_server._address, &nfss->nfs_client->cl_addr,
> > ctx->nfs_server.addrlen);
So, I spent a bunch more time looking at the plumbing of struct sockaddr
vs struct sockaddr_storage. In NFS, everything I could find is actually
already backed by a full sockaddr_storage, so I think a more complete
fix here would actually be to update all the internals to pass
sockaddr_storage instead of sockaddr. The interfaces to other things
(e.g. rpc) can cast back to sockaddr for now. It is a pretty reasonable
cleanup, IMO.
I'll send a follow-up that makes this change on top of this patch,
though they could be squashed if that was desired.
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-11 16:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-11 6:52 [PATCH] NFS: Avoid memcpy() run-time warning for struct sockaddr overflows Kees Cook
2022-10-11 15:25 ` Gustavo A. R. Silva
2022-10-11 16:38 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).