linux-hardening - Re: [PATCH] netfs: Use container

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: <4bbdac4563817326cc5777aa6ec5fcb366235492.camel@kernel.org>
Date:   Tue, 17 May 2022 18:32:34 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     David Howells <dhowells@...hat.com>, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] netfs: Use container_of() for offset casting

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@...nel.org>
> Link: https://lore.kernel.org/lkml/d2ad3a3d7bdd794c6efb562d2f2b655fb67756b9.camel@kernel.org
> Cc: Jeff Layton <jlayton@...nel.org>
> Cc: David Howells <dhowells@...hat.com>
> Signed-off-by: Kees Cook <keescook@...omium.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@...nel.org>