From: Eduard Zingerman <eddyz87@gmail.com>
To: Paul Chaignon <paul.chaignon@gmail.com>, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next 1/2] bpf: Reject narrower access to pointer ctx fields
Date: Mon, 21 Jul 2025 17:08:05 -0700 [thread overview]
Message-ID: <ee25ac4771732bb09513e48fb2bc86614d3fd045.camel@gmail.com> (raw)
In-Reply-To: <e900f2e8c188460284127fe1403728c10c1eb8f4.1753099618.git.paul.chaignon@gmail.com>
On Mon, 2025-07-21 at 14:57 +0200, Paul Chaignon wrote:
[...]
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 72c8b50dca0a..3a4ad9f124e1 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -2577,17 +2577,17 @@ static bool cg_sockopt_is_valid_access(int off, int size,
> }
>
> switch (off) {
> - case offsetof(struct bpf_sockopt, sk):
> + case bpf_ctx_range_ptr(struct bpf_sockopt, sk):
> if (size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_SOCKET;
> break;
> - case offsetof(struct bpf_sockopt, optval):
> + case bpf_ctx_range_ptr(struct bpf_sockopt, optval):
> if (size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_PACKET;
> break;
> - case offsetof(struct bpf_sockopt, optval_end):
> + case bpf_ctx_range_ptr(struct bpf_sockopt, optval_end):
> if (size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_PACKET_END;
Nit: I'd also convert `case offsetof(struct bpf_sockopt, retval):`
just below. Otherwise reader would spend some time figuring out
why `retval` is special (it's not).
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 7a72f766aacf..458908c5f1f4 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8690,7 +8690,7 @@ static bool bpf_skb_is_valid_access(int off, int size, enum bpf_access_type type
> if (size != sizeof(__u64))
> return false;
> break;
> - case offsetof(struct __sk_buff, sk):
> + case bpf_ctx_range_ptr(struct __sk_buff, sk):
> if (type == BPF_WRITE || size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_SOCK_COMMON_OR_NULL;
> @@ -9268,7 +9268,7 @@ static bool sock_addr_is_valid_access(int off, int size,
> return false;
> }
> break;
> - case offsetof(struct bpf_sock_addr, sk):
> + case bpf_ctx_range_ptr(struct bpf_sock_addr, sk):
> if (type != BPF_READ)
> return false;
> if (size != sizeof(__u64))
> @@ -9318,17 +9318,17 @@ static bool sock_ops_is_valid_access(int off, int size,
> if (size != sizeof(__u64))
> return false;
> break;
> - case offsetof(struct bpf_sock_ops, sk):
> + case bpf_ctx_range_ptr(struct bpf_sock_ops, sk):
> if (size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_SOCKET_OR_NULL;
> break;
> - case offsetof(struct bpf_sock_ops, skb_data):
> + case bpf_ctx_range_ptr(struct bpf_sock_ops, skb_data):
> if (size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_PACKET;
> break;
> - case offsetof(struct bpf_sock_ops, skb_data_end):
> + case bpf_ctx_range_ptr(struct bpf_sock_ops, skb_data_end):
> if (size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_PACKET_END;
I think this function is buggy for `skb_hwtstamp` as well.
The skb_hwtstamp field is u64, side_default is sizeof(u32).
So access at `offsetof(struct bpf_sock_ops, skb_hwtstamp) + 4` would
be permitted by the default branch. But this range is not handled by
accompanying sock_ops_convert_ctx_access().
> @@ -9417,7 +9417,7 @@ static bool sk_msg_is_valid_access(int off, int size,
> if (size != sizeof(__u64))
> return false;
> break;
> - case offsetof(struct sk_msg_md, sk):
> + case bpf_ctx_range_ptr(struct sk_msg_md, sk):
> if (size != sizeof(__u64))
> return false;
> info->reg_type = PTR_TO_SOCKET;
I don't think this change is necessary, the default branch rejects
access at any not matched offset. Otherwise `data` and `data_end`
should be converted for uniformity.
> @@ -11623,7 +11623,7 @@ static bool sk_lookup_is_valid_access(int off, int size,
> return false;
>
> switch (off) {
> - case offsetof(struct bpf_sk_lookup, sk):
> + case bpf_ctx_range_ptr(struct bpf_sk_lookup, sk):
> info->reg_type = PTR_TO_SOCKET_OR_NULL;
> return size == sizeof(__u64);
>
Same here, the default branch would reject access at the wrong offset already.
next prev parent reply other threads:[~2025-07-22 0:08 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-21 12:57 [PATCH bpf-next 1/2] bpf: Reject narrower access to pointer ctx fields Paul Chaignon
2025-07-21 13:02 ` [PATCH bpf-next 2/2] selftests/bpf: Test invalid narrower ctx load Paul Chaignon
2025-07-22 0:11 ` Eduard Zingerman
2025-07-22 0:08 ` Eduard Zingerman [this message]
2025-07-22 5:30 ` [PATCH bpf-next 1/2] bpf: Reject narrower access to pointer ctx fields John Fastabend
2025-07-22 14:44 ` Paul Chaignon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ee25ac4771732bb09513e48fb2bc86614d3fd045.camel@gmail.com \
--to=eddyz87@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=paul.chaignon@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).