On 08/20/2018 04:13 PM, Willem de Bruijn wrote:
> On Mon, Aug 20, 2018 at 1:47 AM Song Liu <liu.song....@gmail.com> wrote:
>> On Thu, Aug 16, 2018 at 4:14 PM, Petar Penkov <ppen...@google.com> wrote:
>>> On Thu, Aug 16, 2018 at 3:40 PM, Song Liu <liu.song....@gmail.com> wrote:
>>>> On Thu, Aug 16, 2018 at 9:44 AM, Petar Penkov <peterpenko...@gmail.com>
>>>> wrote:
>>>>> From: Petar Penkov <ppen...@google.com>
>>>>>
>>>>> Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
>>>>> attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
>>>>> path. The BPF program is kept as a global variable so it is
>>>>> accessible to all flow dissectors.
>>>>>
>>>>> Signed-off-by: Petar Penkov <ppen...@google.com>
>>>>> Signed-off-by: Willem de Bruijn <will...@google.com>
>
>>>>> @@ -658,6 +698,42 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>>>> FLOW_DISSECTOR_KEY_BASIC,
>>>>> target_container);
>>>>>
>>>>> + rcu_read_lock();
>>>>> + attached = rcu_dereference(flow_dissector_prog);
>>>>> + if (attached) {
>>>>> + /* Note that even though the const qualifier is discarded
>>>>> + * throughout the execution of the BPF program, all
>>>>> changes(the
>>>>> + * control block) are reverted after the BPF program
>>>>> returns.
>>>>> + * Therefore, __skb_flow_dissect does not alter the skb.
>>>>> + */
>>>>> + struct bpf_flow_dissect_cb *cb;
>>>>> + u8 cb_saved[BPF_SKB_CB_LEN];
>>>>> + u32 result;
>>>>> +
>>>>> + cb = (struct bpf_flow_dissect_cb *)(bpf_skb_cb((struct
>>>>> sk_buff *)skb));
>>>>> +
>>>>> + /* Save Control Block */
>>>>> + memcpy(cb_saved, cb, sizeof(cb_saved));
>>>>> + memset(cb, 0, sizeof(cb_saved));
>>>>> +
>>>>> + /* Pass parameters to the BPF program */
>>>>> + cb->nhoff = nhoff;
>>>>> + cb->target_container = target_container;
>>>>> + cb->flow_dissector = flow_dissector;
>>>>> +
>>>>> + bpf_compute_data_pointers((struct sk_buff *)skb);
>>>>> + result = BPF_PROG_RUN(attached, skb);
>>>>> +
>>>>> + /* Restore state */
>>>>> + memcpy(cb, cb_saved, sizeof(cb_saved));
>>>>> +
>>>>> + key_control->thoff = min_t(u16, key_control->thoff,
>>>>> + skb ? skb->len : hlen);
>>>>> + rcu_read_unlock();
>>>>> + return result == BPF_OK;
>>>>> + }
>>>>
>>>> If the BPF program cannot handle certain protocol, shall we fall back
>>>> to the built-in logic? Otherwise, all BPF programs need to have some
>>>> code for all protocols.
>>>>
>>>> Song
>>>
>>> I believe that if we fall back to the built-in logic we lose all security
>>> guarantees from BPF and this is why the code does not support
>>> fall back.
>>>
>>> Petar
>>
>> I am not really sure we are on the same page. I am proposing 3
>> different return values from BPF_PROG_RUN(), and they should be
>> handled as
>>
>> 1. result == BPF_OK => return true;
>> 2. result == BPF_DROP => return false;
>> 3. result == something else => fall back.
>>
>> Does this proposal make any sense?
>>
>> Thanks,
>> Song
>
> It certainly makes sense. We debated it initially, as well.
>
> In the short term, it allows for simpler BPF programs, as they can
> off-load some protocols to the C implementation.
>
> But the RFC patchset already implements most protocols in BPF.
> I had not expected that when we started out.
>
> Eventually, I think it is preferable to just deprecate the C
> implementation. Which is not possible if we make this opt-out
> a part of the BPF flow dissector interface.
+1
> There is also the lesser issue that a buggy BPF program might
> accidentally pass the third value and unknowing open itself up
> to the large attack surface. Without this option, the security
> audit is much simpler.
Fully agree, I'm all for dropping such option.
Thanks,
Daniel
