Re: [PATCH bpf-next v2] bpf: check attach_func_proto more carefully in check_return_code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
>  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;



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux