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@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.
>
> 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
> happening? If I were a userspace developer and saw the above, I would
> think that the kernel developers have lost it :)
Then don't take a look at this[1]. :p
--
Gustavo
[1] https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
