* [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE
@ 2024-04-26 23:16 Stanislav Fomichev
2024-04-26 23:16 ` [PATCH bpf 1/3] " Stanislav Fomichev
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2024-04-26 23:16 UTC (permalink / raw)
To: bpf, netdev
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Syzkaller found a case where it's possible to attach cgroup_skb program
to the sockopt hooks. Apparently it's currently possible to do that,
but only when using BPF_LINK_CREATE API. The first patch in the series
has more info on why that happens.
Stanislav Fomichev (3):
bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in
BPF_LINK_CREATE
selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE
selftests/bpf: Add sockopt case to verify prog_type
kernel/bpf/syscall.c | 5 ++
.../selftests/bpf/prog_tests/sockopt.c | 65 ++++++++++++++++---
2 files changed, 62 insertions(+), 8 deletions(-)
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf 1/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE
2024-04-26 23:16 [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE Stanislav Fomichev
@ 2024-04-26 23:16 ` Stanislav Fomichev
2024-04-29 19:34 ` Eduard Zingerman
2024-04-26 23:16 ` [PATCH bpf 2/3] selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE Stanislav Fomichev
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Stanislav Fomichev @ 2024-04-26 23:16 UTC (permalink / raw)
To: bpf, netdev
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, syzbot+838346b979830606c854
bpf_prog_attach uses attach_type_to_prog_type to enforce proper
attach type for BPF_PROG_TYPE_CGROUP_SKB. link_create uses
bpf_prog_get and relies on bpf_prog_attach_check_attach_type
to properly verify prog_type <> attach_type association.
Add missing attach_type enforcement for the link_create case.
Otherwise, it's currently possible to attach cgroup_skb prog
types to other cgroup hooks.
Fixes: af6eea57437a ("bpf: Implement bpf_link-based cgroup BPF program attachment")
Link: https://lore.kernel.org/bpf/0000000000004792a90615a1dde0@google.com/
Reported-by: syzbot+838346b979830606c854@syzkaller.appspotmail.com
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
kernel/bpf/syscall.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c287925471f6..cb61d8880dbe 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3985,6 +3985,11 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
* check permissions at attach time.
*/
return -EPERM;
+
+ ptype = attach_type_to_prog_type(attach_type);
+ if (prog->type != ptype)
+ return -EINVAL;
+
return prog->enforce_expected_attach_type &&
prog->expected_attach_type != attach_type ?
-EINVAL : 0;
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf 2/3] selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE
2024-04-26 23:16 [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE Stanislav Fomichev
2024-04-26 23:16 ` [PATCH bpf 1/3] " Stanislav Fomichev
@ 2024-04-26 23:16 ` Stanislav Fomichev
2024-04-29 19:57 ` Eduard Zingerman
2024-04-26 23:16 ` [PATCH bpf 3/3] selftests/bpf: Add sockopt case to verify prog_type Stanislav Fomichev
2024-04-30 18:20 ` [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE patchwork-bot+netdevbpf
3 siblings, 1 reply; 9+ messages in thread
From: Stanislav Fomichev @ 2024-04-26 23:16 UTC (permalink / raw)
To: bpf, netdev
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Run all existing test cases with the attachment created via
BPF_LINK_CREATE. Next commit will add extra test cases to verify
link_create attach_type enforcement.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../selftests/bpf/prog_tests/sockopt.c | 25 ++++++++++++++-----
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
index 5a4491d4edfe..dea340996e97 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
@@ -1036,9 +1036,10 @@ static int call_getsockopt(bool use_io_uring, int fd, int level, int optname,
return getsockopt(fd, level, optname, optval, optlen);
}
-static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring)
+static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring,
+ bool use_link)
{
- int sock_fd, err, prog_fd;
+ int sock_fd, err, prog_fd, link_fd = -1;
void *optval = NULL;
int ret = 0;
@@ -1051,7 +1052,12 @@ static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring)
return -1;
}
- err = bpf_prog_attach(prog_fd, cgroup_fd, test->attach_type, 0);
+ if (use_link) {
+ err = bpf_link_create(prog_fd, cgroup_fd, test->attach_type, NULL);
+ link_fd = err;
+ } else {
+ err = bpf_prog_attach(prog_fd, cgroup_fd, test->attach_type, 0);
+ }
if (err < 0) {
if (test->error == DENY_ATTACH)
goto close_prog_fd;
@@ -1142,7 +1148,12 @@ static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring)
close_sock_fd:
close(sock_fd);
detach_prog:
- bpf_prog_detach2(prog_fd, cgroup_fd, test->attach_type);
+ if (use_link) {
+ if (link_fd >= 0)
+ close(link_fd);
+ } else {
+ bpf_prog_detach2(prog_fd, cgroup_fd, test->attach_type);
+ }
close_prog_fd:
close(prog_fd);
return ret;
@@ -1160,10 +1171,12 @@ void test_sockopt(void)
if (!test__start_subtest(tests[i].descr))
continue;
- ASSERT_OK(run_test(cgroup_fd, &tests[i], false),
+ ASSERT_OK(run_test(cgroup_fd, &tests[i], false, false),
+ tests[i].descr);
+ ASSERT_OK(run_test(cgroup_fd, &tests[i], false, true),
tests[i].descr);
if (tests[i].io_uring_support)
- ASSERT_OK(run_test(cgroup_fd, &tests[i], true),
+ ASSERT_OK(run_test(cgroup_fd, &tests[i], true, false),
tests[i].descr);
}
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf 3/3] selftests/bpf: Add sockopt case to verify prog_type
2024-04-26 23:16 [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE Stanislav Fomichev
2024-04-26 23:16 ` [PATCH bpf 1/3] " Stanislav Fomichev
2024-04-26 23:16 ` [PATCH bpf 2/3] selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE Stanislav Fomichev
@ 2024-04-26 23:16 ` Stanislav Fomichev
2024-04-29 19:58 ` Eduard Zingerman
2024-04-30 18:20 ` [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE patchwork-bot+netdevbpf
3 siblings, 1 reply; 9+ messages in thread
From: Stanislav Fomichev @ 2024-04-26 23:16 UTC (permalink / raw)
To: bpf, netdev
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Make sure only sockopt programs can be attached to the setsockopt
and getsockopt hooks.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../selftests/bpf/prog_tests/sockopt.c | 40 ++++++++++++++++++-
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
index dea340996e97..eaac83a7f388 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
@@ -24,6 +24,7 @@ enum sockopt_test_error {
static struct sockopt_test {
const char *descr;
const struct bpf_insn insns[64];
+ enum bpf_prog_type prog_type;
enum bpf_attach_type attach_type;
enum bpf_attach_type expected_attach_type;
@@ -928,9 +929,40 @@ static struct sockopt_test {
.error = EPERM_SETSOCKOPT,
},
+
+ /* ==================== prog_type ==================== */
+
+ {
+ .descr = "can attach only BPF_CGROUP_SETSOCKOP",
+ .insns = {
+ /* return 1 */
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+
+ },
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+ .attach_type = BPF_CGROUP_SETSOCKOPT,
+ .expected_attach_type = 0,
+ .error = DENY_ATTACH,
+ },
+
+ {
+ .descr = "can attach only BPF_CGROUP_GETSOCKOP",
+ .insns = {
+ /* return 1 */
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+
+ },
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+ .attach_type = BPF_CGROUP_GETSOCKOPT,
+ .expected_attach_type = 0,
+ .error = DENY_ATTACH,
+ },
};
static int load_prog(const struct bpf_insn *insns,
+ enum bpf_prog_type prog_type,
enum bpf_attach_type expected_attach_type)
{
LIBBPF_OPTS(bpf_prog_load_opts, opts,
@@ -947,7 +979,7 @@ static int load_prog(const struct bpf_insn *insns,
}
insns_cnt++;
- fd = bpf_prog_load(BPF_PROG_TYPE_CGROUP_SOCKOPT, NULL, "GPL", insns, insns_cnt, &opts);
+ fd = bpf_prog_load(prog_type, NULL, "GPL", insns, insns_cnt, &opts);
if (verbose && fd < 0)
fprintf(stderr, "%s\n", bpf_log_buf);
@@ -1039,11 +1071,15 @@ static int call_getsockopt(bool use_io_uring, int fd, int level, int optname,
static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring,
bool use_link)
{
+ int prog_type = BPF_PROG_TYPE_CGROUP_SOCKOPT;
int sock_fd, err, prog_fd, link_fd = -1;
void *optval = NULL;
int ret = 0;
- prog_fd = load_prog(test->insns, test->expected_attach_type);
+ if (test->prog_type)
+ prog_type = test->prog_type;
+
+ prog_fd = load_prog(test->insns, prog_type, test->expected_attach_type);
if (prog_fd < 0) {
if (test->error == DENY_LOAD)
return 0;
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE
2024-04-26 23:16 ` [PATCH bpf 1/3] " Stanislav Fomichev
@ 2024-04-29 19:34 ` Eduard Zingerman
2024-04-29 21:17 ` Stanislav Fomichev
0 siblings, 1 reply; 9+ messages in thread
From: Eduard Zingerman @ 2024-04-29 19:34 UTC (permalink / raw)
To: Stanislav Fomichev, bpf, netdev
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa, syzbot+838346b979830606c854
On Fri, 2024-04-26 at 16:16 -0700, Stanislav Fomichev wrote:
> bpf_prog_attach uses attach_type_to_prog_type to enforce proper
> attach type for BPF_PROG_TYPE_CGROUP_SKB. link_create uses
> bpf_prog_get and relies on bpf_prog_attach_check_attach_type
> to properly verify prog_type <> attach_type association.
>
> Add missing attach_type enforcement for the link_create case.
> Otherwise, it's currently possible to attach cgroup_skb prog
> types to other cgroup hooks.
>
> Fixes: af6eea57437a ("bpf: Implement bpf_link-based cgroup BPF program attachment")
> Link: https://lore.kernel.org/bpf/0000000000004792a90615a1dde0@google.com/
> Reported-by: syzbot+838346b979830606c854@syzkaller.appspotmail.com
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
I've spent some time comparing:
- syscall.c:bpf_prog_attach()
- syscall.c:link_create()
- syscall.c:bpf_prog_attach_check_attach_type()
- syscall.c:attach_type_to_prog_type()
- verifier.c:check_return_code()
And it looks like BPF_PROG_TYPE_CGROUP_SKB is the only thing with
missing attach type checks.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
(The interplay between the above functions seems a bit messy,
but I don't have good suggestions for refactoring at the moment.
It appears that bpf_prog_attach_check_attach_type() could be simplified
if prog->enforce_expected_attach_type would be set more aggressively and
if (prog->enforce_expected_attach_type &&
prog->expected_attach_type != attach_type)
return -EINVAL;
moved as a top-level check outside of the switch.
Also BPF_PROG_TYPE_SCHED_CLS case could be removed as it is handled
by default branch. But these are larger changes).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 2/3] selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE
2024-04-26 23:16 ` [PATCH bpf 2/3] selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE Stanislav Fomichev
@ 2024-04-29 19:57 ` Eduard Zingerman
0 siblings, 0 replies; 9+ messages in thread
From: Eduard Zingerman @ 2024-04-29 19:57 UTC (permalink / raw)
To: Stanislav Fomichev, bpf, netdev
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Fri, 2024-04-26 at 16:16 -0700, Stanislav Fomichev wrote:
> Run all existing test cases with the attachment created via
> BPF_LINK_CREATE. Next commit will add extra test cases to verify
> link_create attach_type enforcement.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 3/3] selftests/bpf: Add sockopt case to verify prog_type
2024-04-26 23:16 ` [PATCH bpf 3/3] selftests/bpf: Add sockopt case to verify prog_type Stanislav Fomichev
@ 2024-04-29 19:58 ` Eduard Zingerman
0 siblings, 0 replies; 9+ messages in thread
From: Eduard Zingerman @ 2024-04-29 19:58 UTC (permalink / raw)
To: Stanislav Fomichev, bpf, netdev
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Fri, 2024-04-26 at 16:16 -0700, Stanislav Fomichev wrote:
> Make sure only sockopt programs can be attached to the setsockopt
> and getsockopt hooks.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE
2024-04-29 19:34 ` Eduard Zingerman
@ 2024-04-29 21:17 ` Stanislav Fomichev
0 siblings, 0 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2024-04-29 21:17 UTC (permalink / raw)
To: Eduard Zingerman
Cc: bpf, netdev, ast, daniel, andrii, martin.lau, song, yhs,
john.fastabend, kpsingh, haoluo, jolsa,
syzbot+838346b979830606c854
On 04/29, Eduard Zingerman wrote:
> On Fri, 2024-04-26 at 16:16 -0700, Stanislav Fomichev wrote:
> > bpf_prog_attach uses attach_type_to_prog_type to enforce proper
> > attach type for BPF_PROG_TYPE_CGROUP_SKB. link_create uses
> > bpf_prog_get and relies on bpf_prog_attach_check_attach_type
> > to properly verify prog_type <> attach_type association.
> >
> > Add missing attach_type enforcement for the link_create case.
> > Otherwise, it's currently possible to attach cgroup_skb prog
> > types to other cgroup hooks.
> >
> > Fixes: af6eea57437a ("bpf: Implement bpf_link-based cgroup BPF program attachment")
> > Link: https://lore.kernel.org/bpf/0000000000004792a90615a1dde0@google.com/
> > Reported-by: syzbot+838346b979830606c854@syzkaller.appspotmail.com
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
>
> I've spent some time comparing:
> - syscall.c:bpf_prog_attach()
> - syscall.c:link_create()
> - syscall.c:bpf_prog_attach_check_attach_type()
> - syscall.c:attach_type_to_prog_type()
> - verifier.c:check_return_code()
>
> And it looks like BPF_PROG_TYPE_CGROUP_SKB is the only thing with
> missing attach type checks.
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
[..]
> (The interplay between the above functions seems a bit messy,
> but I don't have good suggestions for refactoring at the moment.
> It appears that bpf_prog_attach_check_attach_type() could be simplified
> if prog->enforce_expected_attach_type would be set more aggressively and
>
> if (prog->enforce_expected_attach_type &&
> prog->expected_attach_type != attach_type)
> return -EINVAL;
>
> moved as a top-level check outside of the switch.
> Also BPF_PROG_TYPE_SCHED_CLS case could be removed as it is handled
> by default branch. But these are larger changes).
Fully agree on this one. Now that we have some tests around that part,
we can probably try to simplify it a bit. Those small differences with
link_create vs prog_attach are a bit hard to follow.
Thanks for the review!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE
2024-04-26 23:16 [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE Stanislav Fomichev
` (2 preceding siblings ...)
2024-04-26 23:16 ` [PATCH bpf 3/3] selftests/bpf: Add sockopt case to verify prog_type Stanislav Fomichev
@ 2024-04-30 18:20 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-04-30 18:20 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, netdev, ast, daniel, andrii, martin.lau, song, yhs,
john.fastabend, kpsingh, haoluo, jolsa
Hello:
This series was applied to bpf/bpf.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:
On Fri, 26 Apr 2024 16:16:17 -0700 you wrote:
> Syzkaller found a case where it's possible to attach cgroup_skb program
> to the sockopt hooks. Apparently it's currently possible to do that,
> but only when using BPF_LINK_CREATE API. The first patch in the series
> has more info on why that happens.
>
> Stanislav Fomichev (3):
> bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in
> BPF_LINK_CREATE
> selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE
> selftests/bpf: Add sockopt case to verify prog_type
>
> [...]
Here is the summary with links:
- [bpf,1/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE
https://git.kernel.org/bpf/bpf/c/543576ec15b1
- [bpf,2/3] selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE
https://git.kernel.org/bpf/bpf/c/d70b2660e75b
- [bpf,3/3] selftests/bpf: Add sockopt case to verify prog_type
https://git.kernel.org/bpf/bpf/c/095ddb501b39
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] 9+ messages in thread
end of thread, other threads:[~2024-04-30 18:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-26 23:16 [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE Stanislav Fomichev
2024-04-26 23:16 ` [PATCH bpf 1/3] " Stanislav Fomichev
2024-04-29 19:34 ` Eduard Zingerman
2024-04-29 21:17 ` Stanislav Fomichev
2024-04-26 23:16 ` [PATCH bpf 2/3] selftests/bpf: Extend sockopt tests to use BPF_LINK_CREATE Stanislav Fomichev
2024-04-29 19:57 ` Eduard Zingerman
2024-04-26 23:16 ` [PATCH bpf 3/3] selftests/bpf: Add sockopt case to verify prog_type Stanislav Fomichev
2024-04-29 19:58 ` Eduard Zingerman
2024-04-30 18:20 ` [PATCH bpf 0/3] bpf: Add BPF_PROG_TYPE_CGROUP_SKB attach type enforcement in BPF_LINK_CREATE 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).