* [PATCH v2 net-next 0/2] skbuff: Switch structure bounds to struct_group()
@ 2021-11-21 0:31 Kees Cook
2021-11-21 0:31 ` [PATCH v2 net-next 1/2] skbuff: Move conditional preprocessor directives out of struct sk_buff Kees Cook
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Kees Cook @ 2021-11-21 0:31 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Kees Cook, David S. Miller, Jonathan Lemon, Alexander Lobakin,
Jakub Sitnicki, Marco Elver, Willem de Bruijn,
Gustavo A. R. Silva, Jason A. Donenfeld, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Nathan Chancellor,
Nick Desaulniers, Eric Dumazet, Cong Wang, Paolo Abeni,
Talal Ahmad, Kevin Hao, Ilias Apalodimas, Vasily Averin,
linux-kernel, wireguard, netdev, bpf, llvm, linux-hardening
Hi,
This is a pair of patches to add struct_group() to struct sk_buff. The
first is needed to work around sparse-specific complaints, and is new
for v2. The second patch is the same as originally sent as v1.
-Kees
Kees Cook (2):
skbuff: Move conditional preprocessor directives out of struct sk_buff
skbuff: Switch structure bounds to struct_group()
drivers/net/wireguard/queueing.h | 4 +--
include/linux/skbuff.h | 46 +++++++++++++++-----------------
net/core/filter.c | 10 +++----
net/core/skbuff.c | 14 ++++------
4 files changed, 33 insertions(+), 41 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 net-next 1/2] skbuff: Move conditional preprocessor directives out of struct sk_buff
2021-11-21 0:31 [PATCH v2 net-next 0/2] skbuff: Switch structure bounds to struct_group() Kees Cook
@ 2021-11-21 0:31 ` Kees Cook
2021-11-21 0:31 ` [PATCH v2 net-next 2/2] skbuff: Switch structure bounds to struct_group() Kees Cook
2021-11-22 15:40 ` [PATCH v2 net-next 0/2] " patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2021-11-21 0:31 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Kees Cook, David S. Miller, Jonathan Lemon, Alexander Lobakin,
Jakub Sitnicki, Marco Elver, Willem de Bruijn,
Gustavo A. R. Silva, Jason A. Donenfeld, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Nathan Chancellor,
Nick Desaulniers, Eric Dumazet, Cong Wang, Paolo Abeni,
Talal Ahmad, Kevin Hao, Ilias Apalodimas, Vasily Averin,
linux-kernel, wireguard, netdev, bpf, llvm, linux-hardening
In preparation for using the struct_group() macro in struct sk_buff,
move the conditional preprocessor directives out of the region of struct
sk_buff that will be enclosed by struct_group(). While GCC and Clang are
happy with conditional preprocessor directives here, sparse is not, even
under -Wno-directive-within-macro[1], as would be seen under a C=1 build:
net/core/filter.c: note: in included file (through include/linux/netlink.h, include/linux/sock_diag.h):
./include/linux/skbuff.h:820:1: warning: directive in macro's argument list
./include/linux/skbuff.h:822:1: warning: directive in macro's argument list
./include/linux/skbuff.h:846:1: warning: directive in macro's argument list
./include/linux/skbuff.h:848:1: warning: directive in macro's argument list
Additionally remove empty macro argument definitions and usage.
"objdump -d" shows no object code differences.
[1] https://www.spinics.net/lists/linux-sparse/msg10857.html
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/skbuff.h | 36 +++++++++++++++++++-----------------
net/core/filter.c | 10 +++++-----
2 files changed, 24 insertions(+), 22 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 686a666d073d..0bce88ac799a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -792,7 +792,7 @@ struct sk_buff {
#else
#define CLONED_MASK 1
#endif
-#define CLONED_OFFSET() offsetof(struct sk_buff, __cloned_offset)
+#define CLONED_OFFSET offsetof(struct sk_buff, __cloned_offset)
/* private: */
__u8 __cloned_offset[0];
@@ -815,18 +815,10 @@ struct sk_buff {
__u32 headers_start[0];
/* public: */
-/* if you move pkt_type around you also must adapt those constants */
-#ifdef __BIG_ENDIAN_BITFIELD
-#define PKT_TYPE_MAX (7 << 5)
-#else
-#define PKT_TYPE_MAX 7
-#endif
-#define PKT_TYPE_OFFSET() offsetof(struct sk_buff, __pkt_type_offset)
-
/* private: */
__u8 __pkt_type_offset[0];
/* public: */
- __u8 pkt_type:3;
+ __u8 pkt_type:3; /* see PKT_TYPE_MAX */
__u8 ignore_df:1;
__u8 nf_trace:1;
__u8 ip_summed:2;
@@ -842,16 +834,10 @@ struct sk_buff {
__u8 encap_hdr_csum:1;
__u8 csum_valid:1;
-#ifdef __BIG_ENDIAN_BITFIELD
-#define PKT_VLAN_PRESENT_BIT 7
-#else
-#define PKT_VLAN_PRESENT_BIT 0
-#endif
-#define PKT_VLAN_PRESENT_OFFSET() offsetof(struct sk_buff, __pkt_vlan_present_offset)
/* private: */
__u8 __pkt_vlan_present_offset[0];
/* public: */
- __u8 vlan_present:1;
+ __u8 vlan_present:1; /* See PKT_VLAN_PRESENT_BIT */
__u8 csum_complete_sw:1;
__u8 csum_level:2;
__u8 csum_not_inet:1;
@@ -950,6 +936,22 @@ struct sk_buff {
#endif
};
+/* if you move pkt_type around you also must adapt those constants */
+#ifdef __BIG_ENDIAN_BITFIELD
+#define PKT_TYPE_MAX (7 << 5)
+#else
+#define PKT_TYPE_MAX 7
+#endif
+#define PKT_TYPE_OFFSET offsetof(struct sk_buff, __pkt_type_offset)
+
+/* if you move pkt_vlan_present around you also must adapt these constants */
+#ifdef __BIG_ENDIAN_BITFIELD
+#define PKT_VLAN_PRESENT_BIT 7
+#else
+#define PKT_VLAN_PRESENT_BIT 0
+#endif
+#define PKT_VLAN_PRESENT_OFFSET offsetof(struct sk_buff, __pkt_vlan_present_offset)
+
#ifdef __KERNEL__
/*
* Handling routines are only of interest to the kernel
diff --git a/net/core/filter.c b/net/core/filter.c
index e471c9b09670..0bf912a44099 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -301,7 +301,7 @@ static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg,
break;
case SKF_AD_PKTTYPE:
- *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_TYPE_OFFSET());
+ *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_TYPE_OFFSET);
*insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, PKT_TYPE_MAX);
#ifdef __BIG_ENDIAN_BITFIELD
*insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 5);
@@ -323,7 +323,7 @@ static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg,
offsetof(struct sk_buff, vlan_tci));
break;
case SKF_AD_VLAN_TAG_PRESENT:
- *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_VLAN_PRESENT_OFFSET());
+ *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_VLAN_PRESENT_OFFSET);
if (PKT_VLAN_PRESENT_BIT)
*insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, PKT_VLAN_PRESENT_BIT);
if (PKT_VLAN_PRESENT_BIT < 7)
@@ -8027,7 +8027,7 @@ static int bpf_unclone_prologue(struct bpf_insn *insn_buf, bool direct_write,
* (Fast-path, otherwise approximation that we might be
* a clone, do the rest in helper.)
*/
- *insn++ = BPF_LDX_MEM(BPF_B, BPF_REG_6, BPF_REG_1, CLONED_OFFSET());
+ *insn++ = BPF_LDX_MEM(BPF_B, BPF_REG_6, BPF_REG_1, CLONED_OFFSET);
*insn++ = BPF_ALU32_IMM(BPF_AND, BPF_REG_6, CLONED_MASK);
*insn++ = BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 7);
@@ -8615,7 +8615,7 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct __sk_buff, pkt_type):
*target_size = 1;
*insn++ = BPF_LDX_MEM(BPF_B, si->dst_reg, si->src_reg,
- PKT_TYPE_OFFSET());
+ PKT_TYPE_OFFSET);
*insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, PKT_TYPE_MAX);
#ifdef __BIG_ENDIAN_BITFIELD
*insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, 5);
@@ -8640,7 +8640,7 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct __sk_buff, vlan_present):
*target_size = 1;
*insn++ = BPF_LDX_MEM(BPF_B, si->dst_reg, si->src_reg,
- PKT_VLAN_PRESENT_OFFSET());
+ PKT_VLAN_PRESENT_OFFSET);
if (PKT_VLAN_PRESENT_BIT)
*insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, PKT_VLAN_PRESENT_BIT);
if (PKT_VLAN_PRESENT_BIT < 7)
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 net-next 2/2] skbuff: Switch structure bounds to struct_group()
2021-11-21 0:31 [PATCH v2 net-next 0/2] skbuff: Switch structure bounds to struct_group() Kees Cook
2021-11-21 0:31 ` [PATCH v2 net-next 1/2] skbuff: Move conditional preprocessor directives out of struct sk_buff Kees Cook
@ 2021-11-21 0:31 ` Kees Cook
2021-11-22 15:40 ` [PATCH v2 net-next 0/2] " patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2021-11-21 0:31 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Kees Cook, Gustavo A . R . Silva, Jason A . Donenfeld,
David S. Miller, Jonathan Lemon, Alexander Lobakin,
Jakub Sitnicki, Marco Elver, Willem de Bruijn,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Nathan Chancellor, Nick Desaulniers, Eric Dumazet,
Cong Wang, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas,
Vasily Averin, linux-kernel, wireguard, netdev, bpf, llvm,
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/*
Link: https://lore.kernel.org/lkml/20210728035006.GD35706@embeddedor
---
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 0bce88ac799a..b474e5bd71cf 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,
/* private: */
__u8 __pkt_type_offset[0];
@@ -918,9 +916,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 related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 net-next 0/2] skbuff: Switch structure bounds to struct_group()
2021-11-21 0:31 [PATCH v2 net-next 0/2] skbuff: Switch structure bounds to struct_group() Kees Cook
2021-11-21 0:31 ` [PATCH v2 net-next 1/2] skbuff: Move conditional preprocessor directives out of struct sk_buff Kees Cook
2021-11-21 0:31 ` [PATCH v2 net-next 2/2] skbuff: Switch structure bounds to struct_group() Kees Cook
@ 2021-11-22 15:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-11-22 15:40 UTC (permalink / raw)
To: Kees Cook
Cc: kuba, davem, jonathan.lemon, alobakin, jakub, elver, willemb,
gustavoars, Jason, ast, daniel, andrii, kafai, songliubraving,
yhs, john.fastabend, kpsingh, nathan, ndesaulniers, edumazet,
cong.wang, pabeni, talalahmad, haokexin, ilias.apalodimas, vvs,
linux-kernel, wireguard, netdev, bpf, llvm, linux-hardening
Hello:
This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:
On Sat, 20 Nov 2021 16:31:47 -0800 you wrote:
> Hi,
>
> This is a pair of patches to add struct_group() to struct sk_buff. The
> first is needed to work around sparse-specific complaints, and is new
> for v2. The second patch is the same as originally sent as v1.
>
> -Kees
>
> [...]
Here is the summary with links:
- [v2,net-next,1/2] skbuff: Move conditional preprocessor directives out of struct sk_buff
https://git.kernel.org/netdev/net-next/c/fba84957e2e2
- [v2,net-next,2/2] skbuff: Switch structure bounds to struct_group()
https://git.kernel.org/netdev/net-next/c/03f61041c179
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-22 15:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-21 0:31 [PATCH v2 net-next 0/2] skbuff: Switch structure bounds to struct_group() Kees Cook
2021-11-21 0:31 ` [PATCH v2 net-next 1/2] skbuff: Move conditional preprocessor directives out of struct sk_buff Kees Cook
2021-11-21 0:31 ` [PATCH v2 net-next 2/2] skbuff: Switch structure bounds to struct_group() Kees Cook
2021-11-22 15:40 ` [PATCH v2 net-next 0/2] " patchwork-bot+netdevbpf
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).