lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 29 Nov 2018 14:18:16 +0900 From: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp> To: Mauricio Vasquez <mauricio.vasquez@...ito.it>, Daniel Borkmann <daniel@...earbox.net>, Alexei Starovoitov <ast@...nel.org> Cc: netdev@...r.kernel.org Subject: Re: [PATCH bpf 2/2] bpf: test_verifier, test for lookup on queue/stack maps On 11/28/2018 10:06 PM, Mauricio Vasquez wrote: > > On 11/28/18 3:45 AM, Daniel Borkmann wrote: >> On 11/28/2018 08:51 AM, Prashant Bhole wrote: >>> This patch adds tests to check whether bpf verifier prevents lookup >>> on queue/stack maps >>> >>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp> >>> --- >>> tools/testing/selftests/bpf/test_verifier.c | 52 +++++++++++++++++++++ >>> 1 file changed, 52 insertions(+) >>> >>> diff --git a/tools/testing/selftests/bpf/test_verifier.c >>> b/tools/testing/selftests/bpf/test_verifier.c >>> index 550b7e46bf4a..becd9f4f3980 100644 >>> --- a/tools/testing/selftests/bpf/test_verifier.c >>> +++ b/tools/testing/selftests/bpf/test_verifier.c >>> @@ -74,6 +74,8 @@ struct bpf_test { >>> int fixup_map_in_map[MAX_FIXUPS]; >>> int fixup_cgroup_storage[MAX_FIXUPS]; >>> int fixup_percpu_cgroup_storage[MAX_FIXUPS]; >>> + int fixup_map_queue[MAX_FIXUPS]; >>> + int fixup_map_stack[MAX_FIXUPS]; >>> const char *errstr; >>> const char *errstr_unpriv; >>> uint32_t retval, retval_unpriv; >>> @@ -4611,6 +4613,38 @@ static struct bpf_test tests[] = { >>> .errstr = "cannot pass map_type 7 into func >>> bpf_map_lookup_elem", >>> .prog_type = BPF_PROG_TYPE_PERF_EVENT, >>> }, >>> + { >>> + "prevent map lookup in queue map", >>> + .insns = { >>> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), >>> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), >>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), >>> + BPF_LD_MAP_FD(BPF_REG_1, 0), >>> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, >>> + BPF_FUNC_map_lookup_elem), >>> + BPF_EXIT_INSN(), >>> + }, >>> + .fixup_map_queue = { 3 }, >>> + .result = REJECT, >>> + .errstr = "invalid stack type R2 off=-8 access_size=0", >>> + .prog_type = BPF_PROG_TYPE_XDP, >> Hmm, the approach in patch 1 is very fragile, and we're lucky in this >> case >> that the verifier bailed out with 'invalid stack type R2 off=-8 >> access_size=0' >> because of key size being zero. If this would have not been the case then >> the added ERR_PTR(-EOPNOTSUPP) would basically be seen as a valid >> pointer and >> the program could read/write into it. Instead, this needs to be >> prevented much >> earlier like check_map_func_compatibility(), > > Actually it is prevented in check_map_func_compatibility(), but stack > boundary check is done before in the verifier. > >> and I would like to have a split >> on these approaches to make verifier more robust. While you want >> ERR_PTR(-EOPNOTSUPP) >> for user space syscall side, > > In the case of QUEUE and STACK maps this is not relevant because the > lookup syscall is mapped into peek operation. > > In fact queue_stack_map_lookup_elem() & queue_stack_map_update_elem() > should be never called, I think we can remove them safely. Got it. Shall we keep these verifier tests (patch 2)? Thanks, Prashant
Powered by blists - more mailing lists