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
