linux-kernel - Re: new __write_overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202205171327.78B12807@keescook>
Date:   Tue, 17 May 2022 13:53:45 -0700
From:   Kees Cook <keescook@...omium.org>
To:     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

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;
 }
 
 /**






-- 
Kees Cook