netdev - Re: [PATCH bpf-next] bpf: add End.DT6 action to bpf_lwt_seg6

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: <CAKSCvkToqKEBqU6zyoX+Y0MxYSMLMvY3tFAVn=0mu4cogFfffg@mail.gmail.com>
Date:   Wed, 25 Jul 2018 10:21:43 +0000
From:   Mathieu Xhonneux <m.xhonneux@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Martin KaFai Lau <kafai@...com>, netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [PATCH bpf-next] bpf: add End.DT6 action to bpf_lwt_seg6_action helper

Indeed, I missed this one. Thanks, sending a v2.

2018-07-25 5:40 GMT+00:00 Daniel Borkmann <daniel@...earbox.net>:
> On 07/24/2018 07:14 PM, Martin KaFai Lau wrote:
>> On Tue, Jul 24, 2018 at 04:59:54PM +0000, Mathieu Xhonneux wrote:
>>> The seg6local LWT provides the End.DT6 action, which allows to
>>> decapsulate an outer IPv6 header containing a Segment Routing Header
>>> (SRH), full specification is available here:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dfilsfils-2Dspring-2Dsrv6-2Dnetwork-2Dprogramming-2D05&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=c61PGnhPMmCUcL5lpyBsxOmsBU2mU5KFY0-Ioo-pBC4&s=mzShtRc5ofzfknAuqoehbGN1ifA17aKihiVLJVfkuZ8&e=
>>>
>>> This patch adds this action now to the seg6local BPF
>>> interface. Since it is not mandatory that the inner IPv6 header also
>>> contains a SRH, seg6_bpf_srh_state has been extended with a pointer to
>>> a possible SRH of the outermost IPv6 header. This helps assessing if the
>>> validation must be triggered or not, and avoids some calls to
>>> ipv6_find_hdr.
>>>
>>> Signed-off-by: Mathieu Xhonneux <m.xhonneux@...il.com>
> [...]
>>> +
>>>  static int input_action_end_bpf(struct sk_buff *skb,
>>>                              struct seg6_local_lwt *slwt)
>>>  {
>>>      struct seg6_bpf_srh_state *srh_state =
>>>              this_cpu_ptr(&seg6_bpf_srh_states);
>>> -    struct seg6_bpf_srh_state local_srh_state;
>>>      struct ipv6_sr_hdr *srh;
>>> -    int srhoff = 0;
>>>      int ret;
>>>
>>>      srh = get_and_validate_srh(skb);
>>> @@ -478,6 +499,7 @@ static int input_action_end_bpf(struct sk_buff *skb,
>>>       * which is also accessed by the bpf_lwt_seg6_* helpers
>>>       */
>>>      preempt_disable();
>>> +    srh_state->srh = srh;
>>>      srh_state->hdrlen = srh->hdrlen << 3;
>>>      srh_state->valid = 1;
>>>
>>> @@ -486,9 +508,6 @@ static int input_action_end_bpf(struct sk_buff *skb,
>>>      ret = bpf_prog_run_save_cb(slwt->bpf.prog, skb);
>>>      rcu_read_unlock();
>>>
>>> -    local_srh_state = *srh_state;
>>> -    preempt_enable();
>>> -
>>>      switch (ret) {
>>>      case BPF_OK:
>>>      case BPF_REDIRECT:
>>> @@ -500,24 +519,17 @@ static int input_action_end_bpf(struct sk_buff *skb,
>>>              goto drop;
>>>      }
>>>
>>> -    if (unlikely((local_srh_state.hdrlen & 7) != 0))
>>> -            goto drop;
>>> -
>>> -    if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
>>> -            goto drop;
>>> -    srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>>> -    srh->hdrlen = (u8)(local_srh_state.hdrlen >> 3);
>>> -
>>> -    if (!local_srh_state.valid &&
>>> -        unlikely(!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3)))
>>> +    if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
>>>              goto drop;
>>>
>>> +    preempt_enable();
>>>      if (ret != BPF_REDIRECT)
>>>              seg6_lookup_nexthop(skb, NULL, 0);
>>>
>>>      return dst_input(skb);
>>>
>>>  drop:
>>> +    preempt_enable();
>> For this drop case at the beginning of this function:
>>
>>       srh = get_and_validate_srh(skb);
>>       if (!srh)
>>               goto drop;
>>
>> preempt_disable() was not called yet?
>
> Agree, this is buggy.