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

Message ID 20210727205855.411487-20-keescook@chromium.org
State New
Headers show
Series
  • Introduce strict memcpy() bounds checking
Related show

Commit Message

Kees Cook July 27, 2021, 8:58 p.m.
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(-)

Comments

Greg Kroah-Hartman July 28, 2021, 5:55 a.m. | #1
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

Patch

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, &eth->h_dest, sizeof(*key_eth_addrs));
+		BUILD_BUG_ON(sizeof(*key_eth_addrs) != sizeof(eth->addrs));
+		memcpy(key_eth_addrs, &eth->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 */