Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 19/64] ip: Use struct_group() for memcpy() regions
From:   "Gustavo A. R. Silva" <gustavo@embeddedor.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Kees Cook <keescook@chromium.org>, linux-hardening@vger.kernel.org,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Keith Packard <keithpac@amazon.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-staging@lists.linux.dev, linux-block@vger.kernel.org,
        linux-kbuild@vger.kernel.org, clang-built-linux@googlegroups.com
References: <20210727205855.411487-1-keescook@chromium.org>
 <20210727205855.411487-20-keescook@chromium.org> <YQDxaYrHu0PeBIuX@kroah.com>
 <baead202-569f-775f-348c-aa64e69f03ed@embeddedor.com>
 <YQD2/CA7zJU7MW6M@kroah.com>
 <e3193698-86d5-d529-e095-e09b4d52927b@embeddedor.com>
Message-ID: <b46e1ac6-ee40-1d23-dbba-b985d9764971@embeddedor.com>
Date:   Wed, 28 Jul 2021 01:37:33 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <e3193698-86d5-d529-e095-e09b4d52927b@embeddedor.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/28/21 01:31, Gustavo A. R. Silva wrote:
> 
> 
> On 7/28/21 01:19, Greg Kroah-Hartman wrote:
>> On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote:
>>>
>>>
>>> On 7/28/21 00:55, Greg Kroah-Hartman wrote:
>>>> On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote:
>>>>> In preparation for FORTIFY_SOURCE performing compile-time and run-time
>>>>> field bounds checking for memcpy(), memmove(), and memset(), avoid
>>>>> intentionally writing across neighboring fields.
>>>>>
>>>>> Use struct_group() in struct flowi4, struct ipv4hdr, and struct ipv6hdr
>>>>> around members saddr and daddr, so they can be referenced together. This
>>>>> will allow memcpy() and sizeof() to more easily reason about sizes,
>>>>> improve readability, and avoid future warnings about writing beyond the
>>>>> end of saddr.
>>>>>
>>>>> "pahole" shows no size nor member offset changes to struct flowi4.
>>>>> "objdump -d" shows no meaningful object code changes (i.e. only source
>>>>> line number induced differences.)
>>>>>
>>>>> Note that since this is a UAPI header, struct_group() has been open
>>>>> coded.
>>>>>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>>  include/net/flow.h            |  6 ++++--
>>>>>  include/uapi/linux/if_ether.h | 12 ++++++++++--
>>>>>  include/uapi/linux/ip.h       | 12 ++++++++++--
>>>>>  include/uapi/linux/ipv6.h     | 12 ++++++++++--
>>>>>  net/core/flow_dissector.c     | 10 ++++++----
>>>>>  net/ipv4/ip_output.c          |  6 ++----
>>>>>  6 files changed, 42 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/include/net/flow.h b/include/net/flow.h
>>>>> index 6f5e70240071..f1a3b6c8eae2 100644
>>>>> --- a/include/net/flow.h
>>>>> +++ b/include/net/flow.h
>>>>> @@ -81,8 +81,10 @@ struct flowi4 {
>>>>>  #define flowi4_multipath_hash	__fl_common.flowic_multipath_hash
>>>>>  
>>>>>  	/* (saddr,daddr) must be grouped, same order as in IP header */
>>>>> -	__be32			saddr;
>>>>> -	__be32			daddr;
>>>>> +	struct_group(addrs,
>>>>> +		__be32			saddr;
>>>>> +		__be32			daddr;
>>>>> +	);
>>>>>  
>>>>>  	union flowi_uli		uli;
>>>>>  #define fl4_sport		uli.ports.sport
>>>>> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
>>>>> index a0b637911d3c..8f5667b2ea92 100644
>>>>> --- a/include/uapi/linux/if_ether.h
>>>>> +++ b/include/uapi/linux/if_ether.h
>>>>> @@ -163,8 +163,16 @@
>>>>>  
>>>>>  #if __UAPI_DEF_ETHHDR
>>>>>  struct ethhdr {
>>>>> -	unsigned char	h_dest[ETH_ALEN];	/* destination eth addr	*/
>>>>> -	unsigned char	h_source[ETH_ALEN];	/* source ether addr	*/
>>>>> +	union {
>>>>> +		struct {
>>>>> +			unsigned char h_dest[ETH_ALEN];	  /* destination eth addr */
>>>>> +			unsigned char h_source[ETH_ALEN]; /* source ether addr	  */
>>>>> +		};
>>>>> +		struct {
>>>>> +			unsigned char h_dest[ETH_ALEN];	  /* destination eth addr */
>>>>> +			unsigned char h_source[ETH_ALEN]; /* source ether addr	  */
>>>>> +		} addrs;
>>>>
>>>> A union of the same fields in the same structure in the same way?
>>>>
>>>> Ah, because struct_group() can not be used here?  Still feels odd to see
>>>> in a userspace-visible header.
>>>>
>>>>> +	};
>>>>>  	__be16		h_proto;		/* packet type ID field	*/
>>>>>  } __attribute__((packed));
>>>>>  #endif
>>>>> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
>>>>> index e42d13b55cf3..33647a37e56b 100644
>>>>> --- a/include/uapi/linux/ip.h
>>>>> +++ b/include/uapi/linux/ip.h
>>>>> @@ -100,8 +100,16 @@ struct iphdr {
>>>>>  	__u8	ttl;
>>>>>  	__u8	protocol;
>>>>>  	__sum16	check;
>>>>> -	__be32	saddr;
>>>>> -	__be32	daddr;
>>>>> +	union {
>>>>> +		struct {
>>>>> +			__be32	saddr;
>>>>> +			__be32	daddr;
>>>>> +		} addrs;
>>>>> +		struct {
>>>>> +			__be32	saddr;
>>>>> +			__be32	daddr;
>>>>> +		};
>>>>
>>>> Same here (except you named the first struct addrs, not the second,
>>>> unlike above).
>>>>
>>>>
>>>>> +	};
>>>>>  	/*The options start here. */
>>>>>  };
>>>>>  
>>>>> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
>>>>> index b243a53fa985..1c26d32e733b 100644
>>>>> --- a/include/uapi/linux/ipv6.h
>>>>> +++ b/include/uapi/linux/ipv6.h
>>>>> @@ -130,8 +130,16 @@ struct ipv6hdr {
>>>>>  	__u8			nexthdr;
>>>>>  	__u8			hop_limit;
>>>>>  
>>>>> -	struct	in6_addr	saddr;
>>>>> -	struct	in6_addr	daddr;
>>>>> +	union {
>>>>> +		struct {
>>>>> +			struct	in6_addr	saddr;
>>>>> +			struct	in6_addr	daddr;
>>>>> +		} addrs;
>>>>> +		struct {
>>>>> +			struct	in6_addr	saddr;
>>>>> +			struct	in6_addr	daddr;
>>>>> +		};
>>>>
>>>> addrs first?  Consistancy is key :)
>>>
>>> I think addrs should be second. In general, I think all newly added
>>> non-anonymous structures should be second.
>>
>> Why not use a local version of the macro like was done in the DRM header
>> file, to make it always work the same and more obvious what is

Yep; I agree. That one looks just fine. :)

--
Gustavo