Development discussion of WireGuard
 help / color / mirror / Atom feed
* [PATCH] skbuff: Switch structure bounds to struct_group()
@ 2021-11-18 18:36 Kees Cook
  2021-11-19  7:13 ` Jakub Kicinski
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-11-18 18:36 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Kees Cook, Gustavo A . R . Silva, David S. Miller,
	Jakub Kicinski, Jonathan Lemon, Alexander Lobakin,
	Jakub Sitnicki, Marco Elver, Willem de Bruijn, Eric Dumazet,
	Cong Wang, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas,
	Kumar Kartikeya Dwivedi, Vasily Averin, linux-kernel, wireguard,
	netdev, linux-hardening

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: Kees Cook <keescook@chromium.org>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com> # drivers/net/wireguard/*
---
 drivers/net/wireguard/queueing.h |  4 +---
 include/linux/skbuff.h           | 10 +++-------
 net/core/skbuff.c                | 14 +++++---------
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
index 4ef2944a68bc..52da5e963003 100644
--- a/drivers/net/wireguard/queueing.h
+++ b/drivers/net/wireguard/queueing.h
@@ -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;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 686a666d073d..875adfd056b6 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -808,12 +808,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,
 
 /* if you move pkt_type around you also must adapt those constants */
 #ifdef __BIG_ENDIAN_BITFIELD
@@ -932,9 +930,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;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ba2f38246f07..3a42b2a3a571 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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);
-- 
2.30.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] skbuff: Switch structure bounds to struct_group()
  2021-11-18 18:36 [PATCH] skbuff: Switch structure bounds to struct_group() Kees Cook
@ 2021-11-19  7:13 ` Jakub Kicinski
  2021-11-19 16:24   ` Kees Cook
  2021-11-19 18:26   ` Kees Cook
  0 siblings, 2 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-11-19  7:13 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jason A. Donenfeld, Gustavo A . R . Silva, David S. Miller,
	Jonathan Lemon, Alexander Lobakin, Jakub Sitnicki, Marco Elver,
	Willem de Bruijn, Eric Dumazet, Cong Wang, Paolo Abeni,
	Talal Ahmad, Kevin Hao, Ilias Apalodimas,
	Kumar Kartikeya Dwivedi, Vasily Averin, linux-kernel, wireguard,
	netdev, linux-hardening

On Thu, 18 Nov 2021 10:36:15 -0800 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.
> 
> 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).

This adds ~27k of these warnings to W=1 gcc builds:

include/linux/skbuff.h:851:1: warning: directive in macro's argument list

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] skbuff: Switch structure bounds to struct_group()
  2021-11-19  7:13 ` Jakub Kicinski
@ 2021-11-19 16:24   ` Kees Cook
  2021-11-19 18:26   ` Kees Cook
  1 sibling, 0 replies; 7+ messages in thread
From: Kees Cook @ 2021-11-19 16:24 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jason A. Donenfeld, Gustavo A . R . Silva, David S. Miller,
	Jonathan Lemon, Alexander Lobakin, Jakub Sitnicki, Marco Elver,
	Willem de Bruijn, Eric Dumazet, Cong Wang, Paolo Abeni,
	Talal Ahmad, Kevin Hao, Ilias Apalodimas,
	Kumar Kartikeya Dwivedi, Vasily Averin, linux-kernel, wireguard,
	netdev, linux-hardening

On Thu, Nov 18, 2021 at 11:13:55PM -0800, Jakub Kicinski wrote:
> On Thu, 18 Nov 2021 10:36:15 -0800 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.
> > 
> > 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).
> 
> This adds ~27k of these warnings to W=1 gcc builds:
> 
> include/linux/skbuff.h:851:1: warning: directive in macro's argument list

Oh my, I see it[1]. I will get that fixed. This smells like a missing
header or something weird. I have a dim memory of fixing this warning
long ago when evolving this series.

Thanks!

-Kees

[1] https://patchwork.kernel.org/project/netdevbpf/patch/20211118183615.1281978-1-keescook@chromium.org/

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] skbuff: Switch structure bounds to struct_group()
  2021-11-19  7:13 ` Jakub Kicinski
  2021-11-19 16:24   ` Kees Cook
@ 2021-11-19 18:26   ` Kees Cook
  2021-11-19 18:41     ` Jakub Kicinski
  1 sibling, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-11-19 18:26 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jason A. Donenfeld, Gustavo A . R . Silva, David S. Miller,
	Jonathan Lemon, Alexander Lobakin, Jakub Sitnicki, Marco Elver,
	Willem de Bruijn, Eric Dumazet, Cong Wang, Paolo Abeni,
	Talal Ahmad, Kevin Hao, Ilias Apalodimas,
	Kumar Kartikeya Dwivedi, Vasily Averin, linux-kernel, wireguard,
	netdev, linux-hardening

On Thu, Nov 18, 2021 at 11:13:55PM -0800, Jakub Kicinski wrote:
> On Thu, 18 Nov 2021 10:36:15 -0800 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.
> > 
> > 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).
> 
> This adds ~27k of these warnings to W=1 gcc builds:
> 
> include/linux/skbuff.h:851:1: warning: directive in macro's argument list

Hrm, I can't reproduce this, using several GCC versions and net-next. What
compiler version[1] and base commit[2] were used here?

-Kees

[1] https://github.com/kuba-moo/nipa/pull/10
[2] https://github.com/kuba-moo/nipa/pull/11

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] skbuff: Switch structure bounds to struct_group()
  2021-11-19 18:26   ` Kees Cook
@ 2021-11-19 18:41     ` Jakub Kicinski
  2021-11-19 18:53       ` Jakub Kicinski
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2021-11-19 18:41 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jason A. Donenfeld, Gustavo A . R . Silva, David S. Miller,
	Jonathan Lemon, Alexander Lobakin, Jakub Sitnicki, Marco Elver,
	Willem de Bruijn, Eric Dumazet, Cong Wang, Paolo Abeni,
	Talal Ahmad, Kevin Hao, Ilias Apalodimas,
	Kumar Kartikeya Dwivedi, Vasily Averin, linux-kernel, wireguard,
	netdev, linux-hardening

On Fri, 19 Nov 2021 10:26:19 -0800 Kees Cook wrote:
> On Thu, Nov 18, 2021 at 11:13:55PM -0800, Jakub Kicinski wrote:
> > On Thu, 18 Nov 2021 10:36:15 -0800 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.
> > > 
> > > 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).  
> > 
> > This adds ~27k of these warnings to W=1 gcc builds:
> > 
> > include/linux/skbuff.h:851:1: warning: directive in macro's argument list  
> 
> Hrm, I can't reproduce this, using several GCC versions and net-next. What
> compiler version[1] and base commit[2] were used here?

gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC) 

HEAD was at: 3b1abcf12894 Merge tag 'regmap-no-bus-update-bits' of git://...

> [1] https://github.com/kuba-moo/nipa/pull/10
> [2] https://github.com/kuba-moo/nipa/pull/11

Thanks for these! Will pull in as soon as the bot finishes with what
it's chewing on right now.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] skbuff: Switch structure bounds to struct_group()
  2021-11-19 18:41     ` Jakub Kicinski
@ 2021-11-19 18:53       ` Jakub Kicinski
  2021-11-19 19:04         ` Kees Cook
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2021-11-19 18:53 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jason A. Donenfeld, Gustavo A . R . Silva, David S. Miller,
	Jonathan Lemon, Alexander Lobakin, Jakub Sitnicki, Marco Elver,
	Willem de Bruijn, Eric Dumazet, Cong Wang, Paolo Abeni,
	Talal Ahmad, Kevin Hao, Ilias Apalodimas,
	Kumar Kartikeya Dwivedi, Vasily Averin, linux-kernel, wireguard,
	netdev, linux-hardening

On Fri, 19 Nov 2021 10:41:44 -0800 Jakub Kicinski wrote:
> On Fri, 19 Nov 2021 10:26:19 -0800 Kees Cook wrote:
> > On Thu, Nov 18, 2021 at 11:13:55PM -0800, Jakub Kicinski wrote:  
> > > This adds ~27k of these warnings to W=1 gcc builds:
> > > 
> > > include/linux/skbuff.h:851:1: warning: directive in macro's argument list    
> > 
> > Hrm, I can't reproduce this, using several GCC versions and net-next. What
> > compiler version[1] and base commit[2] were used here?  
> 
> gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC) 
> 
> HEAD was at: 3b1abcf12894 Merge tag 'regmap-no-bus-update-bits' of git://...

Ah, damn, I just realized, it's probably sparse! The build sets C=1.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] skbuff: Switch structure bounds to struct_group()
  2021-11-19 18:53       ` Jakub Kicinski
@ 2021-11-19 19:04         ` Kees Cook
  0 siblings, 0 replies; 7+ messages in thread
From: Kees Cook @ 2021-11-19 19:04 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jason A. Donenfeld, Gustavo A . R . Silva, David S. Miller,
	Jonathan Lemon, Alexander Lobakin, Jakub Sitnicki, Marco Elver,
	Willem de Bruijn, Eric Dumazet, Cong Wang, Paolo Abeni,
	Talal Ahmad, Kevin Hao, Ilias Apalodimas,
	Kumar Kartikeya Dwivedi, Vasily Averin, linux-kernel, wireguard,
	netdev, linux-hardening

On Fri, Nov 19, 2021 at 10:53:05AM -0800, Jakub Kicinski wrote:
> On Fri, 19 Nov 2021 10:41:44 -0800 Jakub Kicinski wrote:
> > On Fri, 19 Nov 2021 10:26:19 -0800 Kees Cook wrote:
> > > On Thu, Nov 18, 2021 at 11:13:55PM -0800, Jakub Kicinski wrote:  
> > > > This adds ~27k of these warnings to W=1 gcc builds:
> > > > 
> > > > include/linux/skbuff.h:851:1: warning: directive in macro's argument list    
> > > 
> > > Hrm, I can't reproduce this, using several GCC versions and net-next. What
> > > compiler version[1] and base commit[2] were used here?  
> > 
> > gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC) 
> > 
> > HEAD was at: 3b1abcf12894 Merge tag 'regmap-no-bus-update-bits' of git://...
> 
> Ah, damn, I just realized, it's probably sparse! The build sets C=1.

*head desk* I looked right at the "C=1" (noting it was missing for the
clang build) and didn't put it together. Let me see what I need to do to
make sparse happy...

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-11-19 19:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 18:36 [PATCH] skbuff: Switch structure bounds to struct_group() Kees Cook
2021-11-19  7:13 ` Jakub Kicinski
2021-11-19 16:24   ` Kees Cook
2021-11-19 18:26   ` Kees Cook
2021-11-19 18:41     ` Jakub Kicinski
2021-11-19 18:53       ` Jakub Kicinski
2021-11-19 19:04         ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).