| Message ID | 20210727205855.411487-20-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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 :) thanks, greg k-h
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. Thanks -- Gustavo
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 happening? If I were a userspace developer and saw the above, I would think that the kernel developers have lost it :) thanks, greg k-h
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 > 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
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
On Wed, Jul 28, 2021 at 01:31:16AM -0500, Gustavo A. R. Silva wrote: > > 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 That one at least looks a "little" different so maybe it could be seen as semi-reasonable :)
On Wed, Jul 28, 2021 at 07:55:53AM +0200, Greg Kroah-Hartman wrote: > > 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. Yeah, there is some inconsistency here. I will clean this up for v2. Is there a place we can put kernel-specific macros for use in UAPI headers? (I need to figure out where things like __kernel_size_t get defined...)
On 7/28/21 2:01 PM, Kees Cook wrote: > On Wed, Jul 28, 2021 at 07:55:53AM +0200, Greg Kroah-Hartman wrote: >>> 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. > > Yeah, there is some inconsistency here. I will clean this up for v2. > > Is there a place we can put kernel-specific macros for use in UAPI > headers? (I need to figure out where things like __kernel_size_t get > defined...) How about using two memset() calls to clear h_dest[] and h_source[] instead of modifying the uapi header? Thanks, Bart.
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; + }; __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; + }; + }; /*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; + }; + }; }; diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 2aadbfc5193b..87655a2ac200 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -1029,7 +1029,8 @@ bool __skb_flow_dissect(const struct net *net, key_eth_addrs = skb_flow_dissector_target(flow_dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS, target_container); - memcpy(key_eth_addrs, ð->h_dest, sizeof(*key_eth_addrs)); + BUILD_BUG_ON(sizeof(*key_eth_addrs) != sizeof(eth->addrs)); + memcpy(key_eth_addrs, ð->addrs, sizeof(*key_eth_addrs)); } proto_again: @@ -1056,8 +1057,8 @@ bool __skb_flow_dissect(const struct net *net, FLOW_DISSECTOR_KEY_IPV4_ADDRS, target_container); - memcpy(&key_addrs->v4addrs, &iph->saddr, - sizeof(key_addrs->v4addrs)); + BUILD_BUG_ON(sizeof(key_addrs->v4addrs) != sizeof(iph->addrs)); + memcpy(&key_addrs->v4addrs, &iph->addrs, sizeof(iph->addrs)); key_control->addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS; } @@ -1101,7 +1102,8 @@ bool __skb_flow_dissect(const struct net *net, FLOW_DISSECTOR_KEY_IPV6_ADDRS, target_container); - memcpy(&key_addrs->v6addrs, &iph->saddr, + BUILD_BUG_ON(sizeof(iph->addrs) != sizeof(key_addrs->v6addrs)); + memcpy(&key_addrs->v6addrs, &iph->addrs, sizeof(key_addrs->v6addrs)); key_control->addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS; } diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 8d8a8da3ae7e..58603995d889 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -444,10 +444,8 @@ EXPORT_SYMBOL(ip_output); */ static void ip_copy_addrs(struct iphdr *iph, const struct flowi4 *fl4) { - BUILD_BUG_ON(offsetof(typeof(*fl4), daddr) != - offsetof(typeof(*fl4), saddr) + sizeof(fl4->saddr)); - memcpy(&iph->saddr, &fl4->saddr, - sizeof(fl4->saddr) + sizeof(fl4->daddr)); + BUILD_BUG_ON(sizeof(iph->addrs) != sizeof(fl4->addrs)); + memcpy(&iph->addrs, &fl4->addrs, sizeof(fl4->addrs)); } /* Note: skb->sk can be different from sk, in case of tunnels */
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(-)