netdev - Re: [PATCH][next] uapi: net/ipv4: Use __DECLARE_FLEX

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: <202208311116.62D0CD477@keescook>
Date:   Wed, 31 Aug 2022 11:17:31 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] uapi: net/ipv4: Use __DECLARE_FLEX_ARRAY() helper

On Tue, Aug 30, 2022 at 12:24:44PM -0500, Gustavo A. R. Silva wrote:
> We now have a cleaner way to keep compatibility with user-space
> (a.k.a. not breaking it) when we need to keep in place a one-element
> array (for its use in user-space) together with a flexible-array
> member (for its use in kernel-space) without making it hard to read
> at the source level. This is through the use of the new
> __DECLARE_FLEX_ARRAY() helper macro.
> 
> The size and memory layout of the structure is preserved after the
> changes. See below.
> 
> Before changes:
> 
> $ pahole -C ip_msfilter net/ipv4/igmp.o
> struct ip_msfilter {
> 	union {
> 		struct {
> 			__be32     imsf_multiaddr_aux;   /*     0     4 */
> 			__be32     imsf_interface_aux;   /*     4     4 */
> 			__u32      imsf_fmode_aux;       /*     8     4 */
> 			__u32      imsf_numsrc_aux;      /*    12     4 */
> 			__be32     imsf_slist[1];        /*    16     4 */
> 		};                                       /*     0    20 */
> 		struct {
> 			__be32     imsf_multiaddr;       /*     0     4 */
> 			__be32     imsf_interface;       /*     4     4 */
> 			__u32      imsf_fmode;           /*     8     4 */
> 			__u32      imsf_numsrc;          /*    12     4 */
> 			__be32     imsf_slist_flex[0];   /*    16     0 */
> 		};                                       /*     0    16 */
> 	};                                               /*     0    20 */
> 
> 	/* size: 20, cachelines: 1, members: 1 */
> 	/* last cacheline: 20 bytes */
> };
> 
> After changes:
> 
> $ pahole -C ip_msfilter net/ipv4/igmp.o
> struct ip_msfilter {
> 	struct {
> 		__be32             imsf_multiaddr;       /*     0     4 */
> 		__be32             imsf_interface;       /*     4     4 */
> 		__u32              imsf_fmode;           /*     8     4 */
> 		__u32              imsf_numsrc;          /*    12     4 */
> 		union {
> 			__be32     imsf_slist[1];        /*    16     4 */
> 			struct {
> 				struct {
> 				} __empty_imsf_slist_flex; /*    16     0 */
> 				__be32 imsf_slist_flex[0]; /*    16     0 */
> 			};                               /*    16     0 */
> 		};                                       /*    16     4 */
> 	};                                               /*     0    20 */
> 
> 	/* size: 20, cachelines: 1, members: 1 */
> 	/* last cacheline: 20 bytes */
> };
> 
> In the past, we had to duplicate the whole original structure within
> a union, and update the names of all the members. Now, we just need to
> declare the flexible-array member to be used in kernel-space through
> the __DECLARE_FLEX_ARRAY() helper together with the one-element array,
> within a union. This makes the source code more clean and easier to read.
> 
> Link: https://github.com/KSPP/linux/issues/193
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>  include/uapi/linux/in.h | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/in.h b/include/uapi/linux/in.h
> index 14168225cecd..fa4dc8f8f081 100644
> --- a/include/uapi/linux/in.h
> +++ b/include/uapi/linux/in.h
> @@ -188,20 +188,14 @@ struct ip_mreq_source {
>  };
>  
>  struct ip_msfilter {
> -	union {
> -		struct {
> -			__be32		imsf_multiaddr_aux;
> -			__be32		imsf_interface_aux;
> -			__u32		imsf_fmode_aux;
> -			__u32		imsf_numsrc_aux;
> +	struct {

I don't think this internal anonymous struct is needed any more?

> +		__be32		imsf_multiaddr;
> +		__be32		imsf_interface;
> +		__u32		imsf_fmode;
> +		__u32		imsf_numsrc;
> +		union {
>  			__be32		imsf_slist[1];
> -		};
> -		struct {
> -			__be32		imsf_multiaddr;
> -			__be32		imsf_interface;
> -			__u32		imsf_fmode;
> -			__u32		imsf_numsrc;
> -			__be32		imsf_slist_flex[];
> +			__DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex);
>  		};
>  	};
>  };

I.e. can't this now just be:

struct ip_msfilter {
	__be32          imsf_multiaddr;
	__be32          imsf_interface;
	__u32           imsf_fmode;
	__u32           imsf_numsrc;
	union {
		__be32          imsf_slist[1];
		__DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex);
	};
};


> -- 
> 2.34.1
> 

-- 
Kees Cook