| Message ID | 20221011065243.583650-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFS: Avoid memcpy() run-time warning for struct sockaddr overflows | expand |
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 >
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.
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; }
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(-)