* [PATCH] netfs: Use container_of() for offset casting
@ 2022-05-17 21:02 Kees Cook
2022-05-17 22:32 ` Jeff Layton
2022-05-18 8:05 ` David Howells
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2022-05-17 21:02 UTC (permalink / raw)
To: Jeff Layton; +Cc: Kees Cook, David Howells, linux-kernel, linux-hardening
While randstruct was satisfied with using an open-coded "void *" offset
cast for the netfs_i_context <-> inode casting, __builtin_object_size()
as used by FORTIFY_SOURCE was not as easily fooled. Switch to using
an internally defined netfs_i_context/inode struct for doing a full
container_of() casting. This keeps both randstruct and __bos() happy
under GCC 12. Silences:
In file included from ./include/linux/string.h:253,
from ./include/linux/ceph/ceph_debug.h:7,
from fs/ceph/inode.c:2:
In function ‘fortify_memset_chk’,
inlined from ‘netfs_i_context_init’ at ./include/linux/netfs.h:326:2,
inlined from ‘ceph_alloc_inode’ at fs/ceph/inode.c:463:2:
./include/linux/fortify-string.h:242:25: warning: call to ‘__write_overflow_field’ declared with attribute warning:
detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
242 | __write_overflow_field(p_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Reported-by: Jeff Layton <jlayton@kernel.org>
Link: https://lore.kernel.org/lkml/d2ad3a3d7bdd794c6efb562d2f2b655fb67756b9.camel@kernel.org
Cc: Jeff Layton <jlayton@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
If this looks good I can add it to my hardening tree, or if you want to
carry it, I can respin this without the earlier randstruct changes and
drop that patch from my tree?
---
include/linux/netfs.h | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 0c33b715cbfd..cce5a9b53a8a 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -286,6 +286,17 @@ extern void netfs_put_subrequest(struct netfs_io_subrequest *subreq,
bool was_async, enum netfs_sreq_ref_trace what);
extern void netfs_stats_show(struct seq_file *);
+/*
+ * The struct netfs_i_context instance must always follow the VFS inode,
+ * but existing users want to avoid a substructure name space, so just
+ * use this internally to perform the needed container_of() offset
+ * casting, which will keep both FORTIFY_SOURCE and randstruct happy.
+ */
+struct netfs_i_c_pair {
+ struct inode inode;
+ struct netfs_i_context ctx;
+};
+
/**
* netfs_i_context - Get the netfs inode context from the inode
* @inode: The inode to query
@@ -295,7 +306,7 @@ extern void netfs_stats_show(struct seq_file *);
*/
static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
{
- return (void *)inode + sizeof(*inode);
+ return &container_of(inode, struct netfs_i_c_pair, inode)->ctx;
}
/**
@@ -307,7 +318,7 @@ static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
*/
static inline struct inode *netfs_inode(struct netfs_i_context *ctx)
{
- return (void *)ctx - sizeof(struct inode);
+ return &container_of(ctx, struct netfs_i_c_pair, ctx)->inode;
}
/**
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] netfs: Use container_of() for offset casting
2022-05-17 21:02 [PATCH] netfs: Use container_of() for offset casting Kees Cook
@ 2022-05-17 22:32 ` Jeff Layton
2022-05-18 8:05 ` David Howells
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2022-05-17 22:32 UTC (permalink / raw)
To: Kees Cook; +Cc: David Howells, linux-kernel, linux-hardening
On Tue, 2022-05-17 at 14:02 -0700, Kees Cook wrote:
> While randstruct was satisfied with using an open-coded "void *" offset
> cast for the netfs_i_context <-> inode casting, __builtin_object_size()
> as used by FORTIFY_SOURCE was not as easily fooled. Switch to using
> an internally defined netfs_i_context/inode struct for doing a full
> container_of() casting. This keeps both randstruct and __bos() happy
> under GCC 12. Silences:
>
> In file included from ./include/linux/string.h:253,
> from ./include/linux/ceph/ceph_debug.h:7,
> from fs/ceph/inode.c:2:
> In function ‘fortify_memset_chk’,
> inlined from ‘netfs_i_context_init’ at ./include/linux/netfs.h:326:2,
> inlined from ‘ceph_alloc_inode’ at fs/ceph/inode.c:463:2:
> ./include/linux/fortify-string.h:242:25: warning: call to ‘__write_overflow_field’ declared with attribute warning:
> detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
> 242 | __write_overflow_field(p_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Jeff Layton <jlayton@kernel.org>
> Link: https://lore.kernel.org/lkml/d2ad3a3d7bdd794c6efb562d2f2b655fb67756b9.camel@kernel.org
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> If this looks good I can add it to my hardening tree, or if you want to
> carry it, I can respin this without the earlier randstruct changes and
> drop that patch from my tree?
> ---
> include/linux/netfs.h | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index 0c33b715cbfd..cce5a9b53a8a 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -286,6 +286,17 @@ extern void netfs_put_subrequest(struct netfs_io_subrequest *subreq,
> bool was_async, enum netfs_sreq_ref_trace what);
> extern void netfs_stats_show(struct seq_file *);
>
> +/*
> + * The struct netfs_i_context instance must always follow the VFS inode,
> + * but existing users want to avoid a substructure name space, so just
> + * use this internally to perform the needed container_of() offset
> + * casting, which will keep both FORTIFY_SOURCE and randstruct happy.
> + */
> +struct netfs_i_c_pair {
> + struct inode inode;
> + struct netfs_i_context ctx;
> +};
> +
> /**
> * netfs_i_context - Get the netfs inode context from the inode
> * @inode: The inode to query
> @@ -295,7 +306,7 @@ extern void netfs_stats_show(struct seq_file *);
> */
> static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
> {
> - return (void *)inode + sizeof(*inode);
> + return &container_of(inode, struct netfs_i_c_pair, inode)->ctx;
> }
>
> /**
> @@ -307,7 +318,7 @@ static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
> */
> static inline struct inode *netfs_inode(struct netfs_i_context *ctx)
> {
> - return (void *)ctx - sizeof(struct inode);
> + return &container_of(ctx, struct netfs_i_c_pair, ctx)->inode;
> }
>
> /**
This patch didn't apply cleanly for me to a recent tree, but I was able
to wiggle it into place and it seemed to work.
Tested-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfs: Use container_of() for offset casting
2022-05-17 21:02 [PATCH] netfs: Use container_of() for offset casting Kees Cook
2022-05-17 22:32 ` Jeff Layton
@ 2022-05-18 8:05 ` David Howells
2022-05-18 10:01 ` David Laight
` (2 more replies)
1 sibling, 3 replies; 6+ messages in thread
From: David Howells @ 2022-05-18 8:05 UTC (permalink / raw)
To: Kees Cook; +Cc: dhowells, Jeff Layton, linux-kernel, linux-hardening
I wonder if it would be worth making this explicit in the inode wrappers of
the users of netfslib. In afs, for instance, there is:
struct afs_vnode {
struct {
/* These must be contiguous */
struct inode vfs_inode;
struct netfs_i_context netfs_ctx;
};
...
};
would it be worth making that:
struct afs_vnode {
union {
struct netfs_i_c_pair netfs_inode;
struct {
/* These must be contiguous */
struct inode vfs_inode;
struct netfs_i_context netfs_ctx;
};
};
...
};
I don't want to do the following, say:
struct afs_vnode {
struct netfs_i_c_pair ni;
...
};
as that would then require a lot of s/->vfs_inode/->ni.vfs_inode/, but maybe
it would be better to include a struct inode in struct netfs_i_context, and
then do:
struct afs_vnode {
union {
struct netfs_i_context netfs_ctx;
struct inode vfs_inode;
};
...
};
and perhaps rename netfs_i_context to netfs_inode (though that looks a bit
close to nfs_inode). It's just a shame I can't do:
struct netfs_inode : inode {
...
};
struct afs_vnode : netfs_inode {
...
};
right? ;-)
On the other hand:
warthog>git grep '[>.]vfs_inode' -- fs/{9p,afs,ceph,cifs,nfs} | wc -l
181
so maybe a mass change to, say:
struct netfs_inode {
struct inode vfs_inode;
...
};
struct afs_vnode {
struct netfs_inode ni;
...
};
wouldn't be so bad.
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] netfs: Use container_of() for offset casting
2022-05-18 8:05 ` David Howells
@ 2022-05-18 10:01 ` David Laight
2022-05-18 15:21 ` David Howells
2022-05-18 20:21 ` Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2022-05-18 10:01 UTC (permalink / raw)
To: 'David Howells', Kees Cook
Cc: Jeff Layton, linux-kernel, linux-hardening
From: David Howells
> Sent: 18 May 2022 09:05
>
> I wonder if it would be worth making this explicit in the inode wrappers of
> the users of netfslib. In afs, for instance, there is:
>
> struct afs_vnode {
> struct {
> /* These must be contiguous */
> struct inode vfs_inode;
> struct netfs_i_context netfs_ctx;
> };
> ...
> };
>
> would it be worth making that:
>
> struct afs_vnode {
> union {
> struct netfs_i_c_pair netfs_inode;
> struct {
> /* These must be contiguous */
> struct inode vfs_inode;
> struct netfs_i_context netfs_ctx;
> };
> };
> ...
> };
>
Can't you just name the structure so it is:
struct afs_vnode {
struct netfs_i_c_pair {
/* These must be contiguous */
struct inode vfs_inode;
struct netfs_i_context netfs_ctx;
};
...
};
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfs: Use container_of() for offset casting
2022-05-18 8:05 ` David Howells
2022-05-18 10:01 ` David Laight
@ 2022-05-18 15:21 ` David Howells
2022-05-18 20:21 ` Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: David Howells @ 2022-05-18 15:21 UTC (permalink / raw)
To: David Laight
Cc: dhowells, Kees Cook, Jeff Layton, linux-kernel, linux-hardening
David Laight <David.Laight@ACULAB.COM> wrote:
> Can't you just name the structure so it is:
>
> struct afs_vnode {
> struct netfs_i_c_pair {
> /* These must be contiguous */
> struct inode vfs_inode;
> struct netfs_i_context netfs_ctx;
> };
> ...
> };
No. C won't let you do that (the same thing has to be done in 9p, ceph as
well, and at some point hopefully cifs and nfs too).
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfs: Use container_of() for offset casting
2022-05-18 8:05 ` David Howells
2022-05-18 10:01 ` David Laight
2022-05-18 15:21 ` David Howells
@ 2022-05-18 20:21 ` Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2022-05-18 20:21 UTC (permalink / raw)
To: David Howells; +Cc: Jeff Layton, linux-kernel, linux-hardening
On Wed, May 18, 2022 at 09:05:14AM +0100, David Howells wrote:
> [...]
> I don't want to do the following, say:
>
>
> struct afs_vnode {
> struct netfs_i_c_pair ni;
> ...
> };
>
> as that would then require a lot of s/->vfs_inode/->ni.vfs_inode/, but maybe
> it would be better to include a struct inode in struct netfs_i_context, and
Right; that's why I kept the struct internal -- the implicit ordering of
inode and netfs_i_context is already present in all the users.
> On the other hand:
>
> warthog>git grep '[>.]vfs_inode' -- fs/{9p,afs,ceph,cifs,nfs} | wc -l
> 181
That seems painful. Maybe _new_ users of netfs could be written to use
the proposed netfs_inode:
> so maybe a mass change to, say:
>
> struct netfs_inode {
> struct inode vfs_inode;
> ...
> };
Better yet, netfs can define a macro helper. I'll send a v2...
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-18 20:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 21:02 [PATCH] netfs: Use container_of() for offset casting Kees Cook
2022-05-17 22:32 ` Jeff Layton
2022-05-18 8:05 ` David Howells
2022-05-18 10:01 ` David Laight
2022-05-18 15:21 ` David Howells
2022-05-18 20:21 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.