Re: [PATCH bpf-next v4 2/6] bpf: Add verifier support for dynptrs and implement malloc dynptrs

[Daniel Borkmann <daniel@iogearbox.net>]

On 5/12/22 10:03 PM, Joanne Koong wrote:
> On Wed, May 11, 2022 at 5:05 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 5/10/22 12:42 AM, Joanne Koong wrote:
>> [...]
>>> @@ -6498,6 +6523,11 @@ struct bpf_timer {
>>>        __u64 :64;
>>>    } __attribute__((aligned(8)));
>>>
>>> +struct bpf_dynptr {
>>> +     __u64 :64;
>>> +     __u64 :64;
>>> +} __attribute__((aligned(8)));
>>> +
>>>    struct bpf_sysctl {
>>>        __u32   write;          /* Sysctl is being read (= 0) or written (= 1).
>>>                                 * Allows 1,2,4-byte read, but no write.
>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>> index 8a2398ac14c2..a4272e9239ea 100644
>>> --- a/kernel/bpf/helpers.c
>>> +++ b/kernel/bpf/helpers.c
>>> @@ -1396,6 +1396,77 @@ const struct bpf_func_proto bpf_kptr_xchg_proto = {
>>>        .arg2_btf_id  = BPF_PTR_POISON,
>>>    };
>>>
>>> +void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type,
>>> +                  u32 offset, u32 size)
>>> +{
>>> +     ptr->data = data;
>>> +     ptr->offset = offset;
>>> +     ptr->size = size;
>>> +     bpf_dynptr_set_type(ptr, type);
>>> +}
>>> +
>>> +void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr)
>>> +{
>>> +     memset(ptr, 0, sizeof(*ptr));
>>> +}
>>> +
>>> +BPF_CALL_3(bpf_dynptr_alloc, u32, size, u64, flags, struct bpf_dynptr_kern *, ptr)
>>> +{
>>> +     gfp_t gfp_flags = GFP_ATOMIC;
>>
>> nit: should also have __GFP_NOWARN
> I will add this in to v5
>>
>> I presume mem accounting cannot be done on this one given there is no real "ownership"
>> of this piece of mem?
> I'm not too familiar with memory accounting, but I think the ownership
> can get ambiguous given that the memory can be persisted in a map and
> "owned" by different bpf programs (eg the one that frees it may not be
> the same one that allocated it)

Right, it's ambiguous. My worry in particular is that where you added the
BPF_FUNC_dynptr_alloc in the bpf_base_func_proto() with this, e.g. it would
also mean that even unprivileged bpf would be able to allocate huge chunks
of DYNPTR_MAX_SIZE (resp. whatever max kmalloc memory under atomic constraints
can provide) without holding anyone accountable for it.

Thinking more about it, is there even any value for BPF_FUNC_dynptr_* for
fully unpriv BPF if these are rejected anyway by the spectre mitigations
from verifier?

>> Was planning to run some more local tests tomorrow, but from glance at selftest side
>> I haven't seen sanity checks like these:
>>
>> bpf_dynptr_alloc(8, 0, &ptr);
>> data = bpf_dynptr_data(&ptr, 0, 0);
>> bpf_dynptr_put(&ptr);
>> *(__u8 *)data = 23;
>>
>> How is this prevented? I think you do a ptr id check in the is_dynptr_ref_function
>> check on the acquire function, but with above use, would our data pointer escape, or
>> get invalidated via last put?
> 
> There's a subtest inside the dynptr_fail.c file called
> "data_slice_use_after_put" that does:
> 
> bpf_dynptr_alloc(8, 0, &ptr);
> data =bpf_dynptr_data(&ptr, 0, 8);
> bpf_dynptr_put(&ptr);
> val = *(__u8 *)data;
> 
> and checks that trying to dereference the data slice in that last line
> fails the verifier (with error msg "invalid mem access 'scalar'")
> 
> In the verifier, the call to bpf_dynptr_put will invalidate any data
> slices associated with the dyntpr. This happens in
> unmark_stack_slots_dynptr() which calls release_reference() which
> marks the data slice reg as an unknown scalar value. When you try to
> then dereference the data slice, the verifier rejects it with an
> "invalid mem access 'scalar'" message.

Got it, thanks for claryfying! While playing a bit around locally with the
selftests and in relation to above earlier statement, one thing I noticed is
that bpf_dynptr_alloc() and subsequent read is allowed:

bpf_dynptr_alloc(8, 0, &ptr);
bpf_dynptr_read(tmp, sizeof(tmp), &ptr, 0);
bpf_dynptr_put(&ptr);

bpf_dynptr_alloc(8, 0, &ptr);
data = bpf_dynptr_data(&ptr, 0, 8);
// read access uninit data[x]
bpf_dynptr_put(&ptr);

So either for alloc, we always built-in __GFP_ZERO or bpf_dynptr_alloc()
helper usage should go under perfmon_capable() where it's allowed to read
kernel mem.

Thanks,
Daniel