* [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code
@ 2022-07-07 16:02 Stanislav Fomichev
2022-07-07 18:14 ` Martin KaFai Lau
0 siblings, 1 reply; 5+ messages in thread
From: Stanislav Fomichev @ 2022-07-07 16:02 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, syzbot+5cc0730bd4b4d2c5f152
Syzkaller reports the following crash:
RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610
With the following reproducer:
bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)
Because we don't enforce expected_attach_type for XDP programs,
we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP'
part in check_return_code and follow up with testing
`prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto`
is NULL.
Add explicit prog_type check for the "Note, BPF_LSM_CGROUP that
attach ..." condition. Also, don't skip return code check for
LSM/STRUCT_OPS.
The above actually brings an issue with existing selftest which
tries to return EPERM from void inet_csk_clone. Fix the
test (and move called_socket_clone to make sure it's not
incremented in case of an error) and add a new one to explicitly
verify this condition.
v2:
- Martin: don't add new helper, check prog_type instead
- Martin: check expected_attach_type as well at the function entry
- Update selftest to verify this condition
Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
kernel/bpf/verifier.c | 2 ++
.../testing/selftests/bpf/prog_tests/lsm_cgroup.c | 12 ++++++++++++
tools/testing/selftests/bpf/progs/lsm_cgroup.c | 12 ++++++------
.../selftests/bpf/progs/lsm_cgroup_nonvoid.c | 14 ++++++++++++++
4 files changed, 34 insertions(+), 6 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index df3ec6b05f05..2bc1e7252778 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10445,6 +10445,7 @@ static int check_return_code(struct bpf_verifier_env *env)
/* LSM and struct_ops func-ptr's return type could be "void" */
if (!is_subprog &&
+ prog->expected_attach_type != BPF_LSM_CGROUP &&
(prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
prog_type == BPF_PROG_TYPE_LSM) &&
!prog->aux->attach_func_proto->type)
@@ -10572,6 +10573,7 @@ static int check_return_code(struct bpf_verifier_env *env)
if (!tnum_in(range, reg->var_off)) {
verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
if (prog->expected_attach_type == BPF_LSM_CGROUP &&
+ prog_type == BPF_PROG_TYPE_LSM &&
!prog->aux->attach_func_proto->type)
verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
return -EINVAL;
diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
index c542d7e80a5b..1102e4f42d2d 100644
--- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
@@ -6,6 +6,7 @@
#include <bpf/btf.h>
#include "lsm_cgroup.skel.h"
+#include "lsm_cgroup_nonvoid.skel.h"
#include "cgroup_helpers.h"
#include "network_helpers.h"
@@ -293,9 +294,20 @@ static void test_lsm_cgroup_functional(void)
lsm_cgroup__destroy(skel);
}
+static void test_lsm_cgroup_nonvoid(void)
+{
+ struct lsm_cgroup_nonvoid *skel = NULL;
+
+ skel = lsm_cgroup_nonvoid__open_and_load();
+ ASSERT_NULL(skel, "open succeeds");
+ lsm_cgroup_nonvoid__destroy(skel);
+}
+
void test_lsm_cgroup(void)
{
if (test__start_subtest("functional"))
test_lsm_cgroup_functional();
+ if (test__start_subtest("nonvoid"))
+ test_lsm_cgroup_nonvoid();
btf__free(btf);
}
diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup.c b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
index 89f3b1e961a8..4f2d60b87b75 100644
--- a/tools/testing/selftests/bpf/progs/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
@@ -156,25 +156,25 @@ int BPF_PROG(socket_clone, struct sock *newsk, const struct request_sock *req)
{
int prio = 234;
- called_socket_clone++;
-
if (!newsk)
return 1;
/* Accepted request sockets get a different priority. */
if (bpf_setsockopt(newsk, SOL_SOCKET, SO_PRIORITY, &prio, sizeof(prio)))
- return 0; /* EPERM */
+ return 1;
/* Make sure bpf_getsockopt is allowed and works. */
prio = 0;
if (bpf_getsockopt(newsk, SOL_SOCKET, SO_PRIORITY, &prio, sizeof(prio)))
- return 0; /* EPERM */
+ return 1;
if (prio != 234)
- return 0; /* EPERM */
+ return 1;
/* Can access cgroup local storage. */
if (!test_local_storage())
- return 0; /* EPERM */
+ return 1;
+
+ called_socket_clone++;
return 1;
}
diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c b/tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
new file mode 100644
index 000000000000..6cb0f161f417
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+SEC("lsm_cgroup/inet_csk_clone")
+int BPF_PROG(nonvoid_socket_clone, struct sock *newsk, const struct request_sock *req)
+{
+ /* Can not return any errors from void LSM hooks. */
+ return 0;
+}
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code
2022-07-07 16:02 [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code Stanislav Fomichev
@ 2022-07-07 18:14 ` Martin KaFai Lau
2022-07-07 19:18 ` sdf
0 siblings, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2022-07-07 18:14 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa, syzbot+5cc0730bd4b4d2c5f152
On Thu, Jul 07, 2022 at 09:02:33AM -0700, Stanislav Fomichev wrote:
> Syzkaller reports the following crash:
> RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
> RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
> RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610
>
> With the following reproducer:
> bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)
>
> Because we don't enforce expected_attach_type for XDP programs,
> we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP'
> part in check_return_code and follow up with testing
> `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto`
> is NULL.
>
> Add explicit prog_type check for the "Note, BPF_LSM_CGROUP that
> attach ..." condition. Also, don't skip return code check for
> LSM/STRUCT_OPS.
>
> The above actually brings an issue with existing selftest which
> tries to return EPERM from void inet_csk_clone. Fix the
> test (and move called_socket_clone to make sure it's not
> incremented in case of an error) and add a new one to explicitly
> verify this condition.
>
> v2:
> - Martin: don't add new helper, check prog_type instead
> - Martin: check expected_attach_type as well at the function entry
> - Update selftest to verify this condition
>
> Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
> Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> kernel/bpf/verifier.c | 2 ++
> .../testing/selftests/bpf/prog_tests/lsm_cgroup.c | 12 ++++++++++++
> tools/testing/selftests/bpf/progs/lsm_cgroup.c | 12 ++++++------
> .../selftests/bpf/progs/lsm_cgroup_nonvoid.c | 14 ++++++++++++++
> 4 files changed, 34 insertions(+), 6 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index df3ec6b05f05..2bc1e7252778 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10445,6 +10445,7 @@ static int check_return_code(struct bpf_verifier_env *env)
>
> /* LSM and struct_ops func-ptr's return type could be "void" */
> if (!is_subprog &&
> + prog->expected_attach_type != BPF_LSM_CGROUP &&
BPF_PROG_TYPE_STRUCT_OPS also uses the expected_attach_type,
so the expected_attach_type check should only be done for LSM prog alone.
Others lgtm.
> (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
> prog_type == BPF_PROG_TYPE_LSM) &&
> !prog->aux->attach_func_proto->type)
> @@ -10572,6 +10573,7 @@ static int check_return_code(struct bpf_verifier_env *env)
> if (!tnum_in(range, reg->var_off)) {
> verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
> if (prog->expected_attach_type == BPF_LSM_CGROUP &&
> + prog_type == BPF_PROG_TYPE_LSM &&
> !prog->aux->attach_func_proto->type)
> verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
> return -EINVAL;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code
2022-07-07 18:14 ` Martin KaFai Lau
@ 2022-07-07 19:18 ` sdf
2022-07-07 22:08 ` Martin KaFai Lau
0 siblings, 1 reply; 5+ messages in thread
From: sdf @ 2022-07-07 19:18 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa, syzbot+5cc0730bd4b4d2c5f152
On 07/07, Martin KaFai Lau wrote:
> On Thu, Jul 07, 2022 at 09:02:33AM -0700, Stanislav Fomichev wrote:
> > Syzkaller reports the following crash:
> > RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
> > RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
> > RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610
> >
> > With the following reproducer:
> > bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3,
> &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"],
> &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b,
> 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)
> >
> > Because we don't enforce expected_attach_type for XDP programs,
> > we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP'
> > part in check_return_code and follow up with testing
> > `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto`
> > is NULL.
> >
> > Add explicit prog_type check for the "Note, BPF_LSM_CGROUP that
> > attach ..." condition. Also, don't skip return code check for
> > LSM/STRUCT_OPS.
> >
> > The above actually brings an issue with existing selftest which
> > tries to return EPERM from void inet_csk_clone. Fix the
> > test (and move called_socket_clone to make sure it's not
> > incremented in case of an error) and add a new one to explicitly
> > verify this condition.
> >
> > v2:
> > - Martin: don't add new helper, check prog_type instead
> > - Martin: check expected_attach_type as well at the function entry
> > - Update selftest to verify this condition
> >
> > Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
> > Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > kernel/bpf/verifier.c | 2 ++
> > .../testing/selftests/bpf/prog_tests/lsm_cgroup.c | 12 ++++++++++++
> > tools/testing/selftests/bpf/progs/lsm_cgroup.c | 12 ++++++------
> > .../selftests/bpf/progs/lsm_cgroup_nonvoid.c | 14 ++++++++++++++
> > 4 files changed, 34 insertions(+), 6 deletions(-)
> > create mode 100644
> tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index df3ec6b05f05..2bc1e7252778 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10445,6 +10445,7 @@ static int check_return_code(struct
> bpf_verifier_env *env)
> >
> > /* LSM and struct_ops func-ptr's return type could be "void" */
> > if (!is_subprog &&
> > + prog->expected_attach_type != BPF_LSM_CGROUP &&
> BPF_PROG_TYPE_STRUCT_OPS also uses the expected_attach_type,
> so the expected_attach_type check should only be done for LSM prog alone.
> Others lgtm.
In this case, something like the following should be sufficient?
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2bc1e7252778..6702a5fc12e6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10445,11 +10445,13 @@ static int check_return_code(struct
bpf_verifier_env *env)
/* LSM and struct_ops func-ptr's return type could be "void" */
if (!is_subprog &&
- prog->expected_attach_type != BPF_LSM_CGROUP &&
- (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
- prog_type == BPF_PROG_TYPE_LSM) &&
- !prog->aux->attach_func_proto->type)
- return 0;
+ !prog->aux->attach_func_proto->type) {
+ if (prog_type == BPF_PROG_TYPE_STRUCT_OPS)
+ return 0;
+ if (prog_type == BPF_PROG_TYPE_LSM &&
+ prog->expected_attach_type != BPF_LSM_CGROUP)
+ return 0;
+ }
/* eBPF calling convention is such that R0 is used
* to return the value from eBPF program.
> > (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
> > prog_type == BPF_PROG_TYPE_LSM) &&
> > !prog->aux->attach_func_proto->type)
> > @@ -10572,6 +10573,7 @@ static int check_return_code(struct
> bpf_verifier_env *env)
> > if (!tnum_in(range, reg->var_off)) {
> > verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
> > if (prog->expected_attach_type == BPF_LSM_CGROUP &&
> > + prog_type == BPF_PROG_TYPE_LSM &&
> > !prog->aux->attach_func_proto->type)
> > verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks
> can't modify return value!\n");
> > return -EINVAL;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code
2022-07-07 19:18 ` sdf
@ 2022-07-07 22:08 ` Martin KaFai Lau
2022-07-07 23:07 ` Stanislav Fomichev
0 siblings, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2022-07-07 22:08 UTC (permalink / raw)
To: sdf
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa, syzbot+5cc0730bd4b4d2c5f152
On Thu, Jul 07, 2022 at 12:18:33PM -0700, sdf@google.com wrote:
> On 07/07, Martin KaFai Lau wrote:
> > On Thu, Jul 07, 2022 at 09:02:33AM -0700, Stanislav Fomichev wrote:
> > > Syzkaller reports the following crash:
> > > RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
> > > RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
> > > RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610
> > >
> > > With the following reproducer:
> > > bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3,
> > &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"],
> > &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b,
> > 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)
> > >
> > > Because we don't enforce expected_attach_type for XDP programs,
> > > we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP'
> > > part in check_return_code and follow up with testing
> > > `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto`
> > > is NULL.
> > >
> > > Add explicit prog_type check for the "Note, BPF_LSM_CGROUP that
> > > attach ..." condition. Also, don't skip return code check for
> > > LSM/STRUCT_OPS.
> > >
> > > The above actually brings an issue with existing selftest which
> > > tries to return EPERM from void inet_csk_clone. Fix the
> > > test (and move called_socket_clone to make sure it's not
> > > incremented in case of an error) and add a new one to explicitly
> > > verify this condition.
> > >
> > > v2:
> > > - Martin: don't add new helper, check prog_type instead
> > > - Martin: check expected_attach_type as well at the function entry
> > > - Update selftest to verify this condition
> > >
> > > Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
> > > Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > > kernel/bpf/verifier.c | 2 ++
> > > .../testing/selftests/bpf/prog_tests/lsm_cgroup.c | 12 ++++++++++++
> > > tools/testing/selftests/bpf/progs/lsm_cgroup.c | 12 ++++++------
> > > .../selftests/bpf/progs/lsm_cgroup_nonvoid.c | 14 ++++++++++++++
> > > 4 files changed, 34 insertions(+), 6 deletions(-)
> > > create mode 100644
> > tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index df3ec6b05f05..2bc1e7252778 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -10445,6 +10445,7 @@ static int check_return_code(struct
> > bpf_verifier_env *env)
> > >
> > > /* LSM and struct_ops func-ptr's return type could be "void" */
> > > if (!is_subprog &&
> > > + prog->expected_attach_type != BPF_LSM_CGROUP &&
> > BPF_PROG_TYPE_STRUCT_OPS also uses the expected_attach_type,
> > so the expected_attach_type check should only be done for LSM prog alone.
> > Others lgtm.
>
> In this case, something like the following should be sufficient?
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2bc1e7252778..6702a5fc12e6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10445,11 +10445,13 @@ static int check_return_code(struct
> bpf_verifier_env *env)
>
> /* LSM and struct_ops func-ptr's return type could be "void" */
> if (!is_subprog &&
> - prog->expected_attach_type != BPF_LSM_CGROUP &&
> - (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
> - prog_type == BPF_PROG_TYPE_LSM) &&
> - !prog->aux->attach_func_proto->type)
> - return 0;
> + !prog->aux->attach_func_proto->type) {
prog_type check has to be done first since prog->aux->attach_func_proto
depends on the prog_type.
How about a small tweak on top of yours ?
/* LSM and struct_ops func-ptr's return type could be "void" */
if (!is_subprog) {
switch (prog_type) {
case BPF_PROG_TYPE_LSM:
if (prog->expected_attach_type == BPF_LSM_CGROUP)
/* cgroup prog needs to return 0 or 1 */
break;
fallthrough;
case BPF_PROG_TYPE_STRUCT_OPS:
if (!prog->aux->attach_func_proto->type)
return 0;
break;
default:
break;
}
}
> + if (prog_type == BPF_PROG_TYPE_STRUCT_OPS)
> + return 0;
> + if (prog_type == BPF_PROG_TYPE_LSM &&
> + prog->expected_attach_type != BPF_LSM_CGROUP)
> + return 0;
> + }
>
> /* eBPF calling convention is such that R0 is used
> * to return the value from eBPF program.
>
> > > (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
> > > prog_type == BPF_PROG_TYPE_LSM) &&
> > > !prog->aux->attach_func_proto->type)
> > > @@ -10572,6 +10573,7 @@ static int check_return_code(struct
> > bpf_verifier_env *env)
> > > if (!tnum_in(range, reg->var_off)) {
> > > verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
> > > if (prog->expected_attach_type == BPF_LSM_CGROUP &&
> > > + prog_type == BPF_PROG_TYPE_LSM &&
> > > !prog->aux->attach_func_proto->type)
> > > verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks
> > can't modify return value!\n");
> > > return -EINVAL;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code
2022-07-07 22:08 ` Martin KaFai Lau
@ 2022-07-07 23:07 ` Stanislav Fomichev
0 siblings, 0 replies; 5+ messages in thread
From: Stanislav Fomichev @ 2022-07-07 23:07 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa, syzbot+5cc0730bd4b4d2c5f152
On Thu, Jul 7, 2022 at 3:09 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Thu, Jul 07, 2022 at 12:18:33PM -0700, sdf@google.com wrote:
> > On 07/07, Martin KaFai Lau wrote:
> > > On Thu, Jul 07, 2022 at 09:02:33AM -0700, Stanislav Fomichev wrote:
> > > > Syzkaller reports the following crash:
> > > > RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
> > > > RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
> > > > RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610
> > > >
> > > > With the following reproducer:
> > > > bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3,
> > > &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"],
> > > &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b,
> > > 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)
> > > >
> > > > Because we don't enforce expected_attach_type for XDP programs,
> > > > we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP'
> > > > part in check_return_code and follow up with testing
> > > > `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto`
> > > > is NULL.
> > > >
> > > > Add explicit prog_type check for the "Note, BPF_LSM_CGROUP that
> > > > attach ..." condition. Also, don't skip return code check for
> > > > LSM/STRUCT_OPS.
> > > >
> > > > The above actually brings an issue with existing selftest which
> > > > tries to return EPERM from void inet_csk_clone. Fix the
> > > > test (and move called_socket_clone to make sure it's not
> > > > incremented in case of an error) and add a new one to explicitly
> > > > verify this condition.
> > > >
> > > > v2:
> > > > - Martin: don't add new helper, check prog_type instead
> > > > - Martin: check expected_attach_type as well at the function entry
> > > > - Update selftest to verify this condition
> > > >
> > > > Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
> > > > Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com
> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > ---
> > > > kernel/bpf/verifier.c | 2 ++
> > > > .../testing/selftests/bpf/prog_tests/lsm_cgroup.c | 12 ++++++++++++
> > > > tools/testing/selftests/bpf/progs/lsm_cgroup.c | 12 ++++++------
> > > > .../selftests/bpf/progs/lsm_cgroup_nonvoid.c | 14 ++++++++++++++
> > > > 4 files changed, 34 insertions(+), 6 deletions(-)
> > > > create mode 100644
> > > tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
> > > >
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index df3ec6b05f05..2bc1e7252778 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -10445,6 +10445,7 @@ static int check_return_code(struct
> > > bpf_verifier_env *env)
> > > >
> > > > /* LSM and struct_ops func-ptr's return type could be "void" */
> > > > if (!is_subprog &&
> > > > + prog->expected_attach_type != BPF_LSM_CGROUP &&
> > > BPF_PROG_TYPE_STRUCT_OPS also uses the expected_attach_type,
> > > so the expected_attach_type check should only be done for LSM prog alone.
> > > Others lgtm.
> >
> > In this case, something like the following should be sufficient?
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 2bc1e7252778..6702a5fc12e6 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10445,11 +10445,13 @@ static int check_return_code(struct
> > bpf_verifier_env *env)
> >
> > /* LSM and struct_ops func-ptr's return type could be "void" */
> > if (!is_subprog &&
> > - prog->expected_attach_type != BPF_LSM_CGROUP &&
> > - (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
> > - prog_type == BPF_PROG_TYPE_LSM) &&
> > - !prog->aux->attach_func_proto->type)
> > - return 0;
> > + !prog->aux->attach_func_proto->type) {
> prog_type check has to be done first since prog->aux->attach_func_proto
> depends on the prog_type.
>
> How about a small tweak on top of yours ?
Looks good, thanks! Will test and resend sometime tomorrow.
> /* LSM and struct_ops func-ptr's return type could be "void" */
> if (!is_subprog) {
> switch (prog_type) {
> case BPF_PROG_TYPE_LSM:
> if (prog->expected_attach_type == BPF_LSM_CGROUP)
> /* cgroup prog needs to return 0 or 1 */
> break;
> fallthrough;
> case BPF_PROG_TYPE_STRUCT_OPS:
> if (!prog->aux->attach_func_proto->type)
> return 0;
> break;
> default:
> break;
> }
> }
>
> > + if (prog_type == BPF_PROG_TYPE_STRUCT_OPS)
> > + return 0;
> > + if (prog_type == BPF_PROG_TYPE_LSM &&
> > + prog->expected_attach_type != BPF_LSM_CGROUP)
> > + return 0;
> > + }
> >
> > /* eBPF calling convention is such that R0 is used
> > * to return the value from eBPF program.
> >
> > > > (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
> > > > prog_type == BPF_PROG_TYPE_LSM) &&
> > > > !prog->aux->attach_func_proto->type)
> > > > @@ -10572,6 +10573,7 @@ static int check_return_code(struct
> > > bpf_verifier_env *env)
> > > > if (!tnum_in(range, reg->var_off)) {
> > > > verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
> > > > if (prog->expected_attach_type == BPF_LSM_CGROUP &&
> > > > + prog_type == BPF_PROG_TYPE_LSM &&
> > > > !prog->aux->attach_func_proto->type)
> > > > verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks
> > > can't modify return value!\n");
> > > > return -EINVAL;
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-07 23:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07 16:02 [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code Stanislav Fomichev
2022-07-07 18:14 ` Martin KaFai Lau
2022-07-07 19:18 ` sdf
2022-07-07 22:08 ` Martin KaFai Lau
2022-07-07 23:07 ` Stanislav Fomichev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.