Date:   Wed, 18 May 2022 08:57:02 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Kees Cook' <keescook@...omium.org>,
        Jeff Layton <jlayton@...nel.org>
CC:     David Howells <dhowells@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: RE: new __write_overflow_field compiler warning

From: Kees Cook
> Sent: 17 May 2022 21:54
> 
> On Tue, May 17, 2022 at 02:03:52PM -0400, Jeff Layton wrote:
> > Hi Kees,
> 
> Hi!
> 
> > I'm hoping you can help with this. I recently updated to Fedora 36,
> > which has gcc v12, and I've started seeing this warning pop up when
> > compiling the ceph.ko:
> >
> > 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);
> >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > This doesn't seem to happen with gcc v11. It looks like the code is
> > doing the right thing. Is there something we need to fix how the netfs
> > context gets initialized or is this a compiler problem?
> >
> > FWIW: I'm using:
> >
> >     gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1)
> 
> Yeah, GCC 12 got "smarter" about how deeply it can analyze object sizes.
> Usually, this has been helpful. Other times, it's a bit weirder, like
> here.
> 
> So this is resolving to:
> 
> static inline void netfs_i_context_init(struct inode *inode,
>                                         const struct netfs_request_ops *ops)
> {
>         struct netfs_i_context *ctx = netfs_i_context(inode);
> 
>         memset(ctx, 0, sizeof(*ctx));
> ...
> 
> In the sense that the compiler is having trouble understanding this
> object, it's due to the same "unexpected" manipulations that manifest in
> other areas (randstruct) which got fixed recently:
> https://lore.kernel.org/lkml/20220503205503.3054173-2-keescook@chromium.org/
> 
> But it seems randstruct is happy to look the other way here after the
> (void *) cast, where as __builtin_object_size() (the work-horse of the
> memcpy checking) is not. Hmpf.
> 
> Ignoring the linked change above (which doesn't change the warning
> here), GCC is effectively seeing:
> 
> static inline void netfs_i_context_init(struct inode *inode,
>                                         const struct netfs_request_ops *ops)
> {
> 	struct netfs_i_context *ctx = (struct netfs_i_context *)(inode + 1);
> 
> 	if (__builtin_object_size(ctx, 1) < sizeof(*ctx))
> 		__write_overflow_field(...)
> 
> And __builtin_object_size() see "ctx" as pointing past the end of a single
> "struct inode" (i.e. there are zero bytes left in the original
> structure).
> 
> However, I think we can solve both the FORTIFY and the randstruct
> concerns by wrapping the conversions in container_of(). This passes for
> me with -next (i.e. on top of the above linked change):
> 
> 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;
>  }
> 
>  /**

That is unreadable crap.
Can't the compiler be fixed so that it doesn't object to
a very common construct.

Are you sure the compiler isn't returning a size of 0
when it doesn't know the size - as well as when it knows the
size is 0.
Which would mean that all the checks in the kernel headers
are just wrong.

is it enough to replace:
> 	struct netfs_i_context *ctx = (struct netfs_i_context *)(inode + 1);
with
	ctx = (void *)(long)(inode + 1);
or:
	ctx = (void *)((long)inode + sizeof *inode);

Failing that add struct_after() and struct_before()
definitions somewhere.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists