diff mbox

[ovs-dev] odp: Fix crash in parse_8021q_onward().

Message ID 1497613864-88383-1-git-send-email-nic@opencloud.tech
State Accepted
Headers show

Commit Message

nickcooper-zhangtonghao June 16, 2017, 11:51 a.m. UTC 
When we use the 'ovs-appctl ofproto/trace' to send packets,
which include the 'vlan' field, but exclude the 'encap',
the ovs-vswitchd will crash. We should check 'encap' field
in parse_8021q_onward(), before using it.

ovs-appctl ofproto/trace ovs-system  \
    'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),
    eth_type(0x8100),vlan(vid=99,pcp=0)'

    #0  nl_attr_get_size (nla=nla@entry=0x0) at lib/netlink.c:567
    #1  parse_8021q_onward (src_flow=0x7ffd0ec77540, key_len=40,
        key=0x1207e00, flow=0x7ffd0ec77540, expected_attrs=<optimized out>,
        out_of_range_attr=0, present_attrs=120, attrs=0x7ffd0ec77170)
        at lib/odp-util.c:5359
    #2  odp_flow_key_to_flow__ (key=0x1207e00, key_len=40,
        flow=flow@entry=0x7ffd0ec77540, src_flow=src_flow@entry=0x7ffd0ec77540)
        at lib/odp-util.c:5520
    #3  odp_flow_key_to_flow (key=<optimized out>, key_len=<optimized out>,
        flow=flow@entry=0x7ffd0ec77540) at lib/odp-util.c:5555
    #4  parse_flow_and_packet (argc=3, argv=0x12b2220,
        ofprotop=ofprotop@entry=0x7ffd0ec77510, flow=flow@entry=0x7ffd0ec77540,
        packetp=packetp@entry=0x7ffd0ec77518)
        at ofproto/ofproto-dpif-trace.c:211
    #5  ofproto_unixctl_trace (conn=0x1268c20, argc=<optimized out>,
        argv=<optimized out>, aux=<optimized out>) at ofproto/ofproto-dpif-trace.c:309
    #6  process_command (request=<optimized out>, conn=0x1268c20) at lib/unixctl.c:313
    #7  run_connection (conn=0x1268c20) at lib/unixctl.c:347
    #8  unixctl_server_run (server=0x1180970) at lib/unixctl.c:400
    #9  main (argc=5, argv=0x7ffd0ec779c8) at vswitchd/ovs-vswitchd.c:120

Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>
---
 lib/odp-util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Eric Garver July 3, 2017, 8:17 p.m. UTC | #1 
On Fri, Jun 16, 2017 at 04:51:04AM -0700, nickcooper-zhangtonghao wrote:
> When we use the 'ovs-appctl ofproto/trace' to send packets,
> which include the 'vlan' field, but exclude the 'encap',
> the ovs-vswitchd will crash. We should check 'encap' field
> in parse_8021q_onward(), before using it.
> 
> ovs-appctl ofproto/trace ovs-system  \
>     'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),
>     eth_type(0x8100),vlan(vid=99,pcp=0)'
> 
>     #0  nl_attr_get_size (nla=nla@entry=0x0) at lib/netlink.c:567
>     #1  parse_8021q_onward (src_flow=0x7ffd0ec77540, key_len=40,
>         key=0x1207e00, flow=0x7ffd0ec77540, expected_attrs=<optimized out>,
>         out_of_range_attr=0, present_attrs=120, attrs=0x7ffd0ec77170)
>         at lib/odp-util.c:5359
>     #2  odp_flow_key_to_flow__ (key=0x1207e00, key_len=40,
>         flow=flow@entry=0x7ffd0ec77540, src_flow=src_flow@entry=0x7ffd0ec77540)
>         at lib/odp-util.c:5520
>     #3  odp_flow_key_to_flow (key=<optimized out>, key_len=<optimized out>,
>         flow=flow@entry=0x7ffd0ec77540) at lib/odp-util.c:5555
>     #4  parse_flow_and_packet (argc=3, argv=0x12b2220,
>         ofprotop=ofprotop@entry=0x7ffd0ec77510, flow=flow@entry=0x7ffd0ec77540,
>         packetp=packetp@entry=0x7ffd0ec77518)
>         at ofproto/ofproto-dpif-trace.c:211
>     #5  ofproto_unixctl_trace (conn=0x1268c20, argc=<optimized out>,
>         argv=<optimized out>, aux=<optimized out>) at ofproto/ofproto-dpif-trace.c:309
>     #6  process_command (request=<optimized out>, conn=0x1268c20) at lib/unixctl.c:313
>     #7  run_connection (conn=0x1268c20) at lib/unixctl.c:347
>     #8  unixctl_server_run (server=0x1180970) at lib/unixctl.c:400
>     #9  main (argc=5, argv=0x7ffd0ec779c8) at vswitchd/ovs-vswitchd.c:120
> 
> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>
> ---
>  lib/odp-util.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index dea5ab2..4aaf8e1 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -5335,7 +5335,8 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
>                          ? nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN])
>                          : htons(0));
>          if (!is_mask) {
> -            if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN))) {
> +            if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) ||
> +                !(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP))) {
>                  return ODP_FIT_TOO_LITTLE;
>              } else if (flow->vlans[encaps].tci == htons(0)) {
>                  /* Corner case for a truncated 802.1Q header. */
> -- 
> 1.8.3.1

This looks correct to me. ENCAP should be present, we even added it to
the expected_attrs above this code.

Acked-by: Eric Garver <e@erig.me>

--

Looking more, I think check_expectations() should be covering this, but
we don't check/use that fitness value until the very end. I'm not sure
why, but it's always been that way.
Joe Stringer July 5, 2017, 9:36 a.m. UTC | #2 
On 3 July 2017 at 13:17, Eric Garver <e@erig.me> wrote:
> On Fri, Jun 16, 2017 at 04:51:04AM -0700, nickcooper-zhangtonghao wrote:
>> When we use the 'ovs-appctl ofproto/trace' to send packets,
>> which include the 'vlan' field, but exclude the 'encap',
>> the ovs-vswitchd will crash. We should check 'encap' field
>> in parse_8021q_onward(), before using it.
>>
>> ovs-appctl ofproto/trace ovs-system  \
>>     'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),
>>     eth_type(0x8100),vlan(vid=99,pcp=0)'
>>
>>     #0  nl_attr_get_size (nla=nla@entry=0x0) at lib/netlink.c:567
>>     #1  parse_8021q_onward (src_flow=0x7ffd0ec77540, key_len=40,
>>         key=0x1207e00, flow=0x7ffd0ec77540, expected_attrs=<optimized out>,
>>         out_of_range_attr=0, present_attrs=120, attrs=0x7ffd0ec77170)
>>         at lib/odp-util.c:5359
>>     #2  odp_flow_key_to_flow__ (key=0x1207e00, key_len=40,
>>         flow=flow@entry=0x7ffd0ec77540, src_flow=src_flow@entry=0x7ffd0ec77540)
>>         at lib/odp-util.c:5520
>>     #3  odp_flow_key_to_flow (key=<optimized out>, key_len=<optimized out>,
>>         flow=flow@entry=0x7ffd0ec77540) at lib/odp-util.c:5555
>>     #4  parse_flow_and_packet (argc=3, argv=0x12b2220,
>>         ofprotop=ofprotop@entry=0x7ffd0ec77510, flow=flow@entry=0x7ffd0ec77540,
>>         packetp=packetp@entry=0x7ffd0ec77518)
>>         at ofproto/ofproto-dpif-trace.c:211
>>     #5  ofproto_unixctl_trace (conn=0x1268c20, argc=<optimized out>,
>>         argv=<optimized out>, aux=<optimized out>) at ofproto/ofproto-dpif-trace.c:309
>>     #6  process_command (request=<optimized out>, conn=0x1268c20) at lib/unixctl.c:313
>>     #7  run_connection (conn=0x1268c20) at lib/unixctl.c:347
>>     #8  unixctl_server_run (server=0x1180970) at lib/unixctl.c:400
>>     #9  main (argc=5, argv=0x7ffd0ec779c8) at vswitchd/ovs-vswitchd.c:120
>>
>> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>
>> ---
>>  lib/odp-util.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index dea5ab2..4aaf8e1 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -5335,7 +5335,8 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
>>                          ? nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN])
>>                          : htons(0));
>>          if (!is_mask) {
>> -            if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN))) {
>> +            if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) ||
>> +                !(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP))) {
>>                  return ODP_FIT_TOO_LITTLE;
>>              } else if (flow->vlans[encaps].tci == htons(0)) {
>>                  /* Corner case for a truncated 802.1Q header. */
>> --
>> 1.8.3.1
>
> This looks correct to me. ENCAP should be present, we even added it to
> the expected_attrs above this code.
>
> Acked-by: Eric Garver <e@erig.me>

Thanks, applied to master.
diff mbox

Patch

diff --git a/lib/odp-util.c b/lib/odp-util.c
index dea5ab2..4aaf8e1 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -5335,7 +5335,8 @@  parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                         ? nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN])
                         : htons(0));
         if (!is_mask) {
-            if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN))) {
+            if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) ||
+                !(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP))) {
                 return ODP_FIT_TOO_LITTLE;
             } else if (flow->vlans[encaps].tci == htons(0)) {
                 /* Corner case for a truncated 802.1Q header. */