Re: [PATCH 19/64] ip: Use struct_group() for memcpy() regions

[Gustavo A. R. Silva]

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@xxxxxxxxxxxx>
>> ---
>>  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.

Thanks
--
Gustavo