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 :)
thanks,
greg k-h
