Commit 03f61041 authored by Kees Cook's avatar Kees Cook Committed by David S. Miller
Browse files

skbuff: Switch structure bounds to struct_group() 



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.

Replace the existing empty member position markers "headers_start" and
"headers_end" with a struct_group(). This will allow memcpy() and sizeof()
to more easily reason about sizes, and improve readability.

"pahole" shows no size nor member offset changes to struct sk_buff.
"objdump -d" shows no object code changes (outside of WARNs affected by
source line number changes).

Signed-off-by: default avatarKees Cook <keescook@chromium.org>
Reviewed-by: default avatarGustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com> # drivers/net/wireguard/*
Link: https://lore.kernel.org/lkml/20210728035006.GD35706@embeddedor


Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent fba84957

drivers/net/wireguard/queueing.h

+1 −3
Original line number Diff line number Diff line
@@ -79,9 +79,7 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating) 
	u8 sw_hash = skb->sw_hash;

	u32 hash = skb->hash;

	skb_scrub_packet(skb, true);

	memset(&skb->headers_start, 0,

	       offsetof(struct sk_buff, headers_end) -

		       offsetof(struct sk_buff, headers_start));

	memset(&skb->headers, 0, sizeof(skb->headers));

	if (encapsulating) {

		skb->l4_hash = l4_hash;

		skb->sw_hash = sw_hash;

include/linux/skbuff.h

+3 −7
Original line number Diff line number Diff line
@@ -811,12 +811,10 @@ struct sk_buff { 
	__u8			active_extensions;

#endif



	/* fields enclosed in headers_start/headers_end are copied

	/* Fields enclosed in headers group are copied

	 * using a single memcpy() in __copy_skb_header()

	 */

	/* private: */

	__u32			headers_start[0];

	/* public: */

	struct_group(headers,



	/* private: */

	__u8			__pkt_type_offset[0];
@@ -921,9 +919,7 @@ struct sk_buff { 
	u64			kcov_handle;

#endif



	/* private: */

	__u32			headers_end[0];

	/* public: */

	); /* end headers group */



	/* These elements must be at the end, see alloc_skb() for details.  */

	sk_buff_data_t		tail;

net/core/skbuff.c

+5 −9
Original line number Diff line number Diff line
@@ -992,12 +992,10 @@ void napi_consume_skb(struct sk_buff *skb, int budget) 
}

EXPORT_SYMBOL(napi_consume_skb);



/* Make sure a field is enclosed inside headers_start/headers_end section */

/* Make sure a field is contained by headers group */

#define CHECK_SKB_FIELD(field) \

	BUILD_BUG_ON(offsetof(struct sk_buff, field) <		\

		     offsetof(struct sk_buff, headers_start));	\

	BUILD_BUG_ON(offsetof(struct sk_buff, field) >		\

		     offsetof(struct sk_buff, headers_end));	\

	BUILD_BUG_ON(offsetof(struct sk_buff, field) !=		\

		     offsetof(struct sk_buff, headers.field));	\



static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)

{
@@ -1009,14 +1007,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) 
	__skb_ext_copy(new, old);

	__nf_copy(new, old, false);



	/* Note : this field could be in headers_start/headers_end section

	/* Note : this field could be in the headers group.

	 * It is not yet because we do not want to have a 16 bit hole

	 */

	new->queue_mapping = old->queue_mapping;



	memcpy(&new->headers_start, &old->headers_start,

	       offsetof(struct sk_buff, headers_end) -

	       offsetof(struct sk_buff, headers_start));

	memcpy(&new->headers, &old->headers, sizeof(new->headers));

	CHECK_SKB_FIELD(protocol);

	CHECK_SKB_FIELD(csum);

	CHECK_SKB_FIELD(hash);