netdev - Re: [PATCH bpf 2/2] bpf: test_verifier, test for lookup on queue/stack maps

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <07192338-9945-0076-6424-cfa0cad5ca92@lab.ntt.co.jp>
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