| From 05267e90abfb63e68bf7cf9252e30c3aff581400 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Wed, 3 Nov 2021 13:47:35 -0700 |
| Subject: bpf: sockmap, strparser, and tls are reusing qdisc_skb_cb and |
| colliding |
| |
| From: John Fastabend <john.fastabend@gmail.com> |
| |
| [ Upstream commit e0dc3b93bd7bcff8c3813d1df43e0908499c7cf0 ] |
| |
| Strparser is reusing the qdisc_skb_cb struct to stash the skb message handling |
| progress, e.g. offset and length of the skb. First this is poorly named and |
| inherits a struct from qdisc that doesn't reflect the actual usage of cb[] at |
| this layer. |
| |
| But, more importantly strparser is using the following to access its metadata. |
| |
| (struct _strp_msg *)((void *)skb->cb + offsetof(struct qdisc_skb_cb, data)) |
| |
| Where _strp_msg is defined as: |
| |
| struct _strp_msg { |
| struct strp_msg strp; /* 0 8 */ |
| int accum_len; /* 8 4 */ |
| |
| /* size: 12, cachelines: 1, members: 2 */ |
| /* last cacheline: 12 bytes */ |
| }; |
| |
| So we use 12 bytes of ->data[] in struct. However in BPF code running parser |
| and verdict the user has read capabilities into the data[] array as well. Its |
| not too problematic, but we should not be exposing internal state to BPF |
| program. If its really needed then we can use the probe_read() APIs which allow |
| reading kernel memory. And I don't believe cb[] layer poses any API breakage by |
| moving this around because programs can't depend on cb[] across layers. |
| |
| In order to fix another issue with a ctx rewrite we need to stash a temp |
| variable somewhere. To make this work cleanly this patch builds a cb struct |
| for sk_skb types called sk_skb_cb struct. Then we can use this consistently |
| in the strparser, sockmap space. Additionally we can start allowing ->cb[] |
| write access after this. |
| |
| Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") |
| Signed-off-by: John Fastabend <john.fastabend@gmail.com> |
| Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> |
| Tested-by: Jussi Maki <joamaki@gmail.com> |
| Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> |
| Link: https://lore.kernel.org/bpf/20211103204736.248403-5-john.fastabend@gmail.com |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| include/net/strparser.h | 16 +++++++++++++++- |
| net/core/filter.c | 21 +++++++++++++++++++++ |
| net/strparser/strparser.c | 10 +--------- |
| 3 files changed, 37 insertions(+), 10 deletions(-) |
| |
| diff --git a/include/net/strparser.h b/include/net/strparser.h |
| index 1d20b98493a10..bec1439bd3be6 100644 |
| --- a/include/net/strparser.h |
| +++ b/include/net/strparser.h |
| @@ -54,10 +54,24 @@ struct strp_msg { |
| int offset; |
| }; |
| |
| +struct _strp_msg { |
| + /* Internal cb structure. struct strp_msg must be first for passing |
| + * to upper layer. |
| + */ |
| + struct strp_msg strp; |
| + int accum_len; |
| +}; |
| + |
| +struct sk_skb_cb { |
| +#define SK_SKB_CB_PRIV_LEN 20 |
| + unsigned char data[SK_SKB_CB_PRIV_LEN]; |
| + struct _strp_msg strp; |
| +}; |
| + |
| static inline struct strp_msg *strp_msg(struct sk_buff *skb) |
| { |
| return (struct strp_msg *)((void *)skb->cb + |
| - offsetof(struct qdisc_skb_cb, data)); |
| + offsetof(struct sk_skb_cb, strp)); |
| } |
| |
| /* Structure for an attached lower socket */ |
| diff --git a/net/core/filter.c b/net/core/filter.c |
| index 0e161a6dff7e5..5ebc973ed4c50 100644 |
| --- a/net/core/filter.c |
| +++ b/net/core/filter.c |
| @@ -8356,6 +8356,27 @@ static u32 sk_skb_convert_ctx_access(enum bpf_access_type type, |
| *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg, |
| si->src_reg, off); |
| break; |
| + case offsetof(struct __sk_buff, cb[0]) ... |
| + offsetofend(struct __sk_buff, cb[4]) - 1: |
| + BUILD_BUG_ON(sizeof_field(struct sk_skb_cb, data) < 20); |
| + BUILD_BUG_ON((offsetof(struct sk_buff, cb) + |
| + offsetof(struct sk_skb_cb, data)) % |
| + sizeof(__u64)); |
| + |
| + prog->cb_access = 1; |
| + off = si->off; |
| + off -= offsetof(struct __sk_buff, cb[0]); |
| + off += offsetof(struct sk_buff, cb); |
| + off += offsetof(struct sk_skb_cb, data); |
| + if (type == BPF_WRITE) |
| + *insn++ = BPF_STX_MEM(BPF_SIZE(si->code), si->dst_reg, |
| + si->src_reg, off); |
| + else |
| + *insn++ = BPF_LDX_MEM(BPF_SIZE(si->code), si->dst_reg, |
| + si->src_reg, off); |
| + break; |
| + |
| + |
| default: |
| return bpf_convert_ctx_access(type, si, insn_buf, prog, |
| target_size); |
| diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c |
| index b3815c1e8f2ea..cd9954c4ad808 100644 |
| --- a/net/strparser/strparser.c |
| +++ b/net/strparser/strparser.c |
| @@ -27,18 +27,10 @@ |
| |
| static struct workqueue_struct *strp_wq; |
| |
| -struct _strp_msg { |
| - /* Internal cb structure. struct strp_msg must be first for passing |
| - * to upper layer. |
| - */ |
| - struct strp_msg strp; |
| - int accum_len; |
| -}; |
| - |
| static inline struct _strp_msg *_strp_msg(struct sk_buff *skb) |
| { |
| return (struct _strp_msg *)((void *)skb->cb + |
| - offsetof(struct qdisc_skb_cb, data)); |
| + offsetof(struct sk_skb_cb, strp)); |
| } |
| |
| /* Lower lock held */ |
| -- |
| 2.33.0 |
| |