diff mbox series

selftest/bpf: fix IPV6FR handling in flow dissector

Message ID X6rJ7c1C95uNZ/xV@santucci.pierpaolo
State New
Headers show
Series selftest/bpf: fix IPV6FR handling in flow dissector | expand

Commit Message

Santucci Pierpaolo Nov. 10, 2020, 5:12 p.m. UTC
From second fragment on, IPV6FR program must stop the dissection of IPV6
fragmented packet. This is the same approach used for IPV4 fragmentation.

Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com>
---
 tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andrii Nakryiko Nov. 11, 2020, 4:48 a.m. UTC | #1
On Tue, Nov 10, 2020 at 9:12 AM Santucci Pierpaolo
<santucci@epigenesys.com> wrote:
>
> From second fragment on, IPV6FR program must stop the dissection of IPV6
> fragmented packet. This is the same approach used for IPV4 fragmentation.
>

Jakub, can you please take a look as well?

> Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com>
> ---
>  tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c
> index 5a65f6b51377..95a5a0778ed7 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_flow.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c
> @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb)
>                  */
>                 if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG))
>                         return export_flow_keys(keys, BPF_OK);
> +       } else {
> +               return export_flow_keys(keys, BPF_OK);
>         }
>
>         return parse_ipv6_proto(skb, fragh->nexthdr);
> --
> 2.29.2
>
Jakub Sitnicki Nov. 11, 2020, 11:17 a.m. UTC | #2
On Wed, Nov 11, 2020 at 05:48 AM CET, Andrii Nakryiko wrote:
> On Tue, Nov 10, 2020 at 9:12 AM Santucci Pierpaolo
> <santucci@epigenesys.com> wrote:
>>
>> From second fragment on, IPV6FR program must stop the dissection of IPV6
>> fragmented packet. This is the same approach used for IPV4 fragmentation.
>>
>
> Jakub, can you please take a look as well?

I'm not initimately familiar with this test, but looking at the change
I'd consider that Destinations Options and encapsulation headers can
follow the Fragment Header.

With enough of Dst Opts or levels of encapsulation, transport header
could be pushed to the 2nd fragment. So I'm not sure if the assertion
from the IPv4 dissector that 2nd fragment and following doesn't contain
any parseable header holds.

Taking a step back... what problem are we fixing here?

>
>> Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com>
>> ---
>>  tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c
>> index 5a65f6b51377..95a5a0778ed7 100644
>> --- a/tools/testing/selftests/bpf/progs/bpf_flow.c
>> +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c
>> @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb)
>>                  */
>>                 if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG))
>>                         return export_flow_keys(keys, BPF_OK);
>> +       } else {
>> +               return export_flow_keys(keys, BPF_OK);
>>         }
>>
>>         return parse_ipv6_proto(skb, fragh->nexthdr);
>> --
>> 2.29.2
>>
Santucci Pierpaolo Nov. 11, 2020, 2:12 p.m. UTC | #3
Hi Jakub,

thanks for your reply.

Let me explain the problem with an example.

Please consider the PCAP file:
https://github.com/named-data/ndn-tools/blob/master/tests/dissect-wireshark/ipv6-udp-fragmented.pcap
Let's assume that the dissector is invoked without the flag:
BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL.
 
Without the proposed patch, the flow keys for the second fragment (packet
timestamp 0.256997) will contain the value 0x6868 for the source and
destination port fields: this is obviously wrong.
The same happens for the third fragment (packet timestamp 0.256998) and for
the fourth fragment (packet timestamp 0.257001).

So it seems that the correct thing to do is to stop the dissector after the
IPV6 fragmentation header for all fragments from the second on.

Regards,
    Pierpaolo Santucci

On Wed, Nov 11, 2020 at 12:17:06PM +0100, Jakub Sitnicki wrote:
> On Wed, Nov 11, 2020 at 05:48 AM CET, Andrii Nakryiko wrote:
> > On Tue, Nov 10, 2020 at 9:12 AM Santucci Pierpaolo
> > <santucci@epigenesys.com> wrote:
> >>
> >> From second fragment on, IPV6FR program must stop the dissection of IPV6
> >> fragmented packet. This is the same approach used for IPV4 fragmentation.
> >>
> >
> > Jakub, can you please take a look as well?
> 
> I'm not initimately familiar with this test, but looking at the change
> I'd consider that Destinations Options and encapsulation headers can
> follow the Fragment Header.
> 
> With enough of Dst Opts or levels of encapsulation, transport header
> could be pushed to the 2nd fragment. So I'm not sure if the assertion
> from the IPv4 dissector that 2nd fragment and following doesn't contain
> any parseable header holds.
> 
> Taking a step back... what problem are we fixing here?
> 
> >
> >> Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com>
> >> ---
> >>  tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c
> >> index 5a65f6b51377..95a5a0778ed7 100644
> >> --- a/tools/testing/selftests/bpf/progs/bpf_flow.c
> >> +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c
> >> @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb)
> >>                  */
> >>                 if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG))
> >>                         return export_flow_keys(keys, BPF_OK);
> >> +       } else {
> >> +               return export_flow_keys(keys, BPF_OK);
> >>         }
> >>
> >>         return parse_ipv6_proto(skb, fragh->nexthdr);
> >> --
> >> 2.29.2
> >>
Daniel Borkmann Nov. 11, 2020, 11:06 p.m. UTC | #4
On 11/11/20 3:12 PM, Santucci Pierpaolo wrote:
> Hi Jakub,
> 
> thanks for your reply.

(Santucci, please do not top-post but always reply inline which makes it
  easier for discussions to follow.)

> Let me explain the problem with an example.
> 
> Please consider the PCAP file:
> https://github.com/named-data/ndn-tools/blob/master/tests/dissect-wireshark/ipv6-udp-fragmented.pcap
> Let's assume that the dissector is invoked without the flag:
> BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL.
>   
> Without the proposed patch, the flow keys for the second fragment (packet
> timestamp 0.256997) will contain the value 0x6868 for the source and
> destination port fields: this is obviously wrong.
> The same happens for the third fragment (packet timestamp 0.256998) and for
> the fourth fragment (packet timestamp 0.257001).
> 
> So it seems that the correct thing to do is to stop the dissector after the
> IPV6 fragmentation header for all fragments from the second on.
> 
[...]
>>
>> I'm not initimately familiar with this test, but looking at the change
>> I'd consider that Destinations Options and encapsulation headers can
>> follow the Fragment Header.
>>
>> With enough of Dst Opts or levels of encapsulation, transport header
>> could be pushed to the 2nd fragment. So I'm not sure if the assertion
>> from the IPv4 dissector that 2nd fragment and following doesn't contain
>> any parseable header holds.

Hm, staring at rfc8200, it says that the first fragment packet must include
the upper-layer header (e.g. tcp, udp). The patch here should probably add a
comment wrt to the rfc.

Thanks,
Daniel
Jakub Sitnicki Nov. 12, 2020, 9 a.m. UTC | #5
On Thu, Nov 12, 2020 at 12:06 AM CET, Daniel Borkmann wrote:

[...]

>>> I'm not initimately familiar with this test, but looking at the change
>>> I'd consider that Destinations Options and encapsulation headers can
>>> follow the Fragment Header.
>>>
>>> With enough of Dst Opts or levels of encapsulation, transport header
>>> could be pushed to the 2nd fragment. So I'm not sure if the assertion
>>> from the IPv4 dissector that 2nd fragment and following doesn't contain
>>> any parseable header holds.
>
> Hm, staring at rfc8200, it says that the first fragment packet must include
> the upper-layer header (e.g. tcp, udp). The patch here should probably add a
> comment wrt to the rfc.

You're right, it clearly says so. Nevermind my worries about malformed
packets then. Change LGTM:

Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
John Fastabend Nov. 13, 2020, 12:50 a.m. UTC | #6
Jakub Sitnicki wrote:
> On Thu, Nov 12, 2020 at 12:06 AM CET, Daniel Borkmann wrote:
> 
> [...]
> 
> >>> I'm not initimately familiar with this test, but looking at the change
> >>> I'd consider that Destinations Options and encapsulation headers can
> >>> follow the Fragment Header.
> >>>
> >>> With enough of Dst Opts or levels of encapsulation, transport header
> >>> could be pushed to the 2nd fragment. So I'm not sure if the assertion
> >>> from the IPv4 dissector that 2nd fragment and following doesn't contain
> >>> any parseable header holds.
> >
> > Hm, staring at rfc8200, it says that the first fragment packet must include
> > the upper-layer header (e.g. tcp, udp). The patch here should probably add a
> > comment wrt to the rfc.
> 
> You're right, it clearly says so. Nevermind my worries about malformed
> packets then. Change LGTM:
> 
> Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>

Also please add some of the details discussed here to the commit msg so
we can remember this next time. 

Thanks!
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c
index 5a65f6b51377..95a5a0778ed7 100644
--- a/tools/testing/selftests/bpf/progs/bpf_flow.c
+++ b/tools/testing/selftests/bpf/progs/bpf_flow.c
@@ -368,6 +368,8 @@  PROG(IPV6FR)(struct __sk_buff *skb)
 		 */
 		if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG))
 			return export_flow_keys(keys, BPF_OK);
+	} else {
+		return export_flow_keys(keys, BPF_OK);
 	}
 
 	return parse_ipv6_proto(skb, fragh->nexthdr);