diff mbox series

[ovs-dev,v2] ofp-ed-props: Fix using uninitialized padding for NSH encap actions.

Message ID 20201014161346.2295287-1-i.maximets@ovn.org
State Accepted
Headers show
Series [ovs-dev,v2] ofp-ed-props: Fix using uninitialized padding for NSH encap actions. | expand

Commit Message

Ilya Maximets Oct. 14, 2020, 4:13 p.m. UTC 
OVS uses memcmp to compare actions of existing and new flows, but
'struct ofp_ed_prop_nsh_md_type' and corresponding ofpact structure has
3 bytes of padding that never initialized and passed around within OF
data structures and messages.

  Uninitialized bytes in MemcmpInterceptorCommon
    at offset 21 inside [0x7090000003f8, 136)
  WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x4a184e in bcmp (vswitchd/ovs-vswitchd+0x4a184e)
    #1 0x896c8a in ofpacts_equal lib/ofp-actions.c:9121:31
    #2 0x564403 in replace_rule_finish ofproto/ofproto.c:5650:37
    #3 0x563462 in add_flow_finish ofproto/ofproto.c:5218:13
    #4 0x54a1ff in ofproto_flow_mod_finish ofproto/ofproto.c:8091:17
    #5 0x5433b2 in handle_flow_mod__ ofproto/ofproto.c:6216:17
    #6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17
    #7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16
    #8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21
    #9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13
    #10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9
    #11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5
    #12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9
    #13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5
    #14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9
    #15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6)
    #16 0x46b92d in _start (vswitchd/ovs-vswitchd+0x46b92d)

  Uninitialized value was stored to memory at
    #0 0x4745aa in __msan_memcpy.part.0 (vswitchd/ovs-vswitchd)
    #1 0x54529f in rule_actions_create ofproto/ofproto.c:3134:5
    #2 0x54915e in ofproto_rule_create ofproto/ofproto.c:5284:11
    #3 0x55d419 in add_flow_init ofproto/ofproto.c:5123:17
    #4 0x54841f in ofproto_flow_mod_init ofproto/ofproto.c:7987:17
    #5 0x543250 in handle_flow_mod__ ofproto/ofproto.c:6206:13
    #6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17
    #7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16
    #8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21
    #9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13
    #10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9
    #11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5
    #12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9
    #13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5
    #14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9
    #15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6)

  Uninitialized value was created by an allocation of 'ofpacts_stub'
  in the stack frame of function 'handle_flow_mod'
    #0 0x569e80 in handle_flow_mod ofproto/ofproto.c:6170

This could cause issues with flow modifications or other operations.

To reproduce, some NSH tests could be run under valgrind or clang
MemorySantizer. Ex. "nsh - md1 encap over a veth link" test.

Fix that by clearing padding bytes while encoding and decoding.
OVS will still accept OF messages with non-zero padding from
controllers.

New tests added to tests/ofp-actions.at.

Fixes: 1fc11c5948cf ("Generic encap and decap support for NSH")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/ofp-ed-props.c   |  3 ++-
 tests/ofp-actions.at | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

Jan Scheurich Oct. 14, 2020, 8:58 p.m. UTC | #1 
LGTM. Please back-port to stable branches.

Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>

/Jan

> -----Original Message-----
> From: Ilya Maximets <i.maximets@ovn.org>
> Sent: Wednesday, 14 October, 2020 18:14
> To: ovs-dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com>
> Cc: Ben Pfaff <blp@ovn.org>; Ilya Maximets <i.maximets@ovn.org>
> Subject: [PATCH v2] ofp-ed-props: Fix using uninitialized padding for NSH
> encap actions.
> 
> OVS uses memcmp to compare actions of existing and new flows, but 'struct
> ofp_ed_prop_nsh_md_type' and corresponding ofpact structure has
> 3 bytes of padding that never initialized and passed around within OF data
> structures and messages.
> 
>   Uninitialized bytes in MemcmpInterceptorCommon
>     at offset 21 inside [0x7090000003f8, 136)
>   WARNING: MemorySanitizer: use-of-uninitialized-value
>     #0 0x4a184e in bcmp (vswitchd/ovs-vswitchd+0x4a184e)
>     #1 0x896c8a in ofpacts_equal lib/ofp-actions.c:9121:31
>     #2 0x564403 in replace_rule_finish ofproto/ofproto.c:5650:37
>     #3 0x563462 in add_flow_finish ofproto/ofproto.c:5218:13
>     #4 0x54a1ff in ofproto_flow_mod_finish ofproto/ofproto.c:8091:17
>     #5 0x5433b2 in handle_flow_mod__ ofproto/ofproto.c:6216:17
>     #6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17
>     #7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16
>     #8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21
>     #9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13
>     #10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9
>     #11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5
>     #12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9
>     #13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5
>     #14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9
>     #15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6)
>     #16 0x46b92d in _start (vswitchd/ovs-vswitchd+0x46b92d)
> 
>   Uninitialized value was stored to memory at
>     #0 0x4745aa in __msan_memcpy.part.0 (vswitchd/ovs-vswitchd)
>     #1 0x54529f in rule_actions_create ofproto/ofproto.c:3134:5
>     #2 0x54915e in ofproto_rule_create ofproto/ofproto.c:5284:11
>     #3 0x55d419 in add_flow_init ofproto/ofproto.c:5123:17
>     #4 0x54841f in ofproto_flow_mod_init ofproto/ofproto.c:7987:17
>     #5 0x543250 in handle_flow_mod__ ofproto/ofproto.c:6206:13
>     #6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17
>     #7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16
>     #8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21
>     #9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13
>     #10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9
>     #11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5
>     #12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9
>     #13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5
>     #14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9
>     #15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6)
> 
>   Uninitialized value was created by an allocation of 'ofpacts_stub'
>   in the stack frame of function 'handle_flow_mod'
>     #0 0x569e80 in handle_flow_mod ofproto/ofproto.c:6170
> 
> This could cause issues with flow modifications or other operations.
> 
> To reproduce, some NSH tests could be run under valgrind or clang
> MemorySantizer. Ex. "nsh - md1 encap over a veth link" test.
> 
> Fix that by clearing padding bytes while encoding and decoding.
> OVS will still accept OF messages with non-zero padding from controllers.
> 
> New tests added to tests/ofp-actions.at.
> 
> Fixes: 1fc11c5948cf ("Generic encap and decap support for NSH")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  lib/ofp-ed-props.c   |  3 ++-
>  tests/ofp-actions.at | 11 +++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index
> 28382e012..02a9235d5 100644
> --- a/lib/ofp-ed-props.c
> +++ b/lib/ofp-ed-props.c
> @@ -49,7 +49,7 @@ decode_ed_prop(const struct ofp_ed_prop_header
> **ofp_prop,
>                  return OFPERR_NXBAC_BAD_ED_PROP;
>              }
>              struct ofpact_ed_prop_nsh_md_type *pnmt =
> -                    ofpbuf_put_uninit(out, sizeof(*pnmt));
> +                    ofpbuf_put_zeros(out, sizeof *pnmt);
>              pnmt->header.prop_class = prop_class;
>              pnmt->header.type = prop_type;
>              pnmt->header.len = len;
> @@ -108,6 +108,7 @@ encode_ed_prop(const struct ofpact_ed_prop
> **prop,
>              opnmt->header.len =
>                      offsetof(struct ofp_ed_prop_nsh_md_type, pad);
>              opnmt->md_type = pnmt->md_type;
> +            memset(opnmt->pad, 0, sizeof opnmt->pad);
>              prop_len = sizeof(*pnmt);
>              break;
>          }
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at index
> 28b2099a0..c79d7d0e2 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -769,6 +769,17 @@ dnl Check OpenFlow v1.3.4 Conformance Test:
> 430.510.
>  & 00000010  00 00 00 10 00 00 00 01-
>  0019 0010 80000807 000102030405 000000000010 00000001
> 
> +dnl Check NSH encap (experimenter extension).
> +# actions=encap(nsh(md_type=1))
> +ffff 0018 00002320 002e 0000 0001894f 0004 01 05 01 000000
> +
> +dnl NSH encap with non-zero padding.
> +# actions=encap(nsh(md_type=1))
> +# 21: 12 -> 00
> +# 22: 34 -> 00
> +# 23: 56 -> 00
> +ffff 0018 00002320 002e 0000 0001894f 0004 01 05 01 123456
> +
>  ])
>  sed '/^[[#&]]/d' < test-data > input.txt  sed -n 's/^# //p; /^$/p' < test-data >
> expout
> --
> 2.25.4
Ilya Maximets Oct. 19, 2020, 1:15 p.m. UTC | #2 
On 10/14/20 10:58 PM, Jan Scheurich wrote:
> LGTM. Please back-port to stable branches.
> 
> Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>

Thanks!

Applied to master and backported down to 2.8.

Best regards, Ilya Maximets.

> 
> /Jan
> 
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets@ovn.org>
>> Sent: Wednesday, 14 October, 2020 18:14
>> To: ovs-dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com>
>> Cc: Ben Pfaff <blp@ovn.org>; Ilya Maximets <i.maximets@ovn.org>
>> Subject: [PATCH v2] ofp-ed-props: Fix using uninitialized padding for NSH
>> encap actions.
>>
>> OVS uses memcmp to compare actions of existing and new flows, but 'struct
>> ofp_ed_prop_nsh_md_type' and corresponding ofpact structure has
>> 3 bytes of padding that never initialized and passed around within OF data
>> structures and messages.
>>
>>   Uninitialized bytes in MemcmpInterceptorCommon
>>     at offset 21 inside [0x7090000003f8, 136)
>>   WARNING: MemorySanitizer: use-of-uninitialized-value
>>     #0 0x4a184e in bcmp (vswitchd/ovs-vswitchd+0x4a184e)
>>     #1 0x896c8a in ofpacts_equal lib/ofp-actions.c:9121:31
>>     #2 0x564403 in replace_rule_finish ofproto/ofproto.c:5650:37
>>     #3 0x563462 in add_flow_finish ofproto/ofproto.c:5218:13
>>     #4 0x54a1ff in ofproto_flow_mod_finish ofproto/ofproto.c:8091:17
>>     #5 0x5433b2 in handle_flow_mod__ ofproto/ofproto.c:6216:17
>>     #6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17
>>     #7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16
>>     #8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21
>>     #9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13
>>     #10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9
>>     #11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5
>>     #12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9
>>     #13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5
>>     #14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9
>>     #15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6)
>>     #16 0x46b92d in _start (vswitchd/ovs-vswitchd+0x46b92d)
>>
>>   Uninitialized value was stored to memory at
>>     #0 0x4745aa in __msan_memcpy.part.0 (vswitchd/ovs-vswitchd)
>>     #1 0x54529f in rule_actions_create ofproto/ofproto.c:3134:5
>>     #2 0x54915e in ofproto_rule_create ofproto/ofproto.c:5284:11
>>     #3 0x55d419 in add_flow_init ofproto/ofproto.c:5123:17
>>     #4 0x54841f in ofproto_flow_mod_init ofproto/ofproto.c:7987:17
>>     #5 0x543250 in handle_flow_mod__ ofproto/ofproto.c:6206:13
>>     #6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17
>>     #7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16
>>     #8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21
>>     #9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13
>>     #10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9
>>     #11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5
>>     #12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9
>>     #13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5
>>     #14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9
>>     #15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6)
>>
>>   Uninitialized value was created by an allocation of 'ofpacts_stub'
>>   in the stack frame of function 'handle_flow_mod'
>>     #0 0x569e80 in handle_flow_mod ofproto/ofproto.c:6170
>>
>> This could cause issues with flow modifications or other operations.
>>
>> To reproduce, some NSH tests could be run under valgrind or clang
>> MemorySantizer. Ex. "nsh - md1 encap over a veth link" test.
>>
>> Fix that by clearing padding bytes while encoding and decoding.
>> OVS will still accept OF messages with non-zero padding from controllers.
>>
>> New tests added to tests/ofp-actions.at.
>>
>> Fixes: 1fc11c5948cf ("Generic encap and decap support for NSH")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>> ---
>>  lib/ofp-ed-props.c   |  3 ++-
>>  tests/ofp-actions.at | 11 +++++++++++
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index
>> 28382e012..02a9235d5 100644
>> --- a/lib/ofp-ed-props.c
>> +++ b/lib/ofp-ed-props.c
>> @@ -49,7 +49,7 @@ decode_ed_prop(const struct ofp_ed_prop_header
>> **ofp_prop,
>>                  return OFPERR_NXBAC_BAD_ED_PROP;
>>              }
>>              struct ofpact_ed_prop_nsh_md_type *pnmt =
>> -                    ofpbuf_put_uninit(out, sizeof(*pnmt));
>> +                    ofpbuf_put_zeros(out, sizeof *pnmt);
>>              pnmt->header.prop_class = prop_class;
>>              pnmt->header.type = prop_type;
>>              pnmt->header.len = len;
>> @@ -108,6 +108,7 @@ encode_ed_prop(const struct ofpact_ed_prop
>> **prop,
>>              opnmt->header.len =
>>                      offsetof(struct ofp_ed_prop_nsh_md_type, pad);
>>              opnmt->md_type = pnmt->md_type;
>> +            memset(opnmt->pad, 0, sizeof opnmt->pad);
>>              prop_len = sizeof(*pnmt);
>>              break;
>>          }
>> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at index
>> 28b2099a0..c79d7d0e2 100644
>> --- a/tests/ofp-actions.at
>> +++ b/tests/ofp-actions.at
>> @@ -769,6 +769,17 @@ dnl Check OpenFlow v1.3.4 Conformance Test:
>> 430.510.
>>  & 00000010  00 00 00 10 00 00 00 01-
>>  0019 0010 80000807 000102030405 000000000010 00000001
>>
>> +dnl Check NSH encap (experimenter extension).
>> +# actions=encap(nsh(md_type=1))
>> +ffff 0018 00002320 002e 0000 0001894f 0004 01 05 01 000000
>> +
>> +dnl NSH encap with non-zero padding.
>> +# actions=encap(nsh(md_type=1))
>> +# 21: 12 -> 00
>> +# 22: 34 -> 00
>> +# 23: 56 -> 00
>> +ffff 0018 00002320 002e 0000 0001894f 0004 01 05 01 123456
>> +
>>  ])
>>  sed '/^[[#&]]/d' < test-data > input.txt  sed -n 's/^# //p; /^$/p' < test-data >
>> expout
>> --
>> 2.25.4
>
diff mbox series

Patch

diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c
index 28382e012..02a9235d5 100644
--- a/lib/ofp-ed-props.c
+++ b/lib/ofp-ed-props.c
@@ -49,7 +49,7 @@  decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
                 return OFPERR_NXBAC_BAD_ED_PROP;
             }
             struct ofpact_ed_prop_nsh_md_type *pnmt =
-                    ofpbuf_put_uninit(out, sizeof(*pnmt));
+                    ofpbuf_put_zeros(out, sizeof *pnmt);
             pnmt->header.prop_class = prop_class;
             pnmt->header.type = prop_type;
             pnmt->header.len = len;
@@ -108,6 +108,7 @@  encode_ed_prop(const struct ofpact_ed_prop **prop,
             opnmt->header.len =
                     offsetof(struct ofp_ed_prop_nsh_md_type, pad);
             opnmt->md_type = pnmt->md_type;
+            memset(opnmt->pad, 0, sizeof opnmt->pad);
             prop_len = sizeof(*pnmt);
             break;
         }
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 28b2099a0..c79d7d0e2 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -769,6 +769,17 @@  dnl Check OpenFlow v1.3.4 Conformance Test: 430.510.
 & 00000010  00 00 00 10 00 00 00 01-
 0019 0010 80000807 000102030405 000000000010 00000001
 
+dnl Check NSH encap (experimenter extension).
+# actions=encap(nsh(md_type=1))
+ffff 0018 00002320 002e 0000 0001894f 0004 01 05 01 000000
+
+dnl NSH encap with non-zero padding.
+# actions=encap(nsh(md_type=1))
+# 21: 12 -> 00
+# 22: 34 -> 00
+# 23: 56 -> 00
+ffff 0018 00002320 002e 0000 0001894f 0004 01 05 01 123456
+
 ])
 sed '/^[[#&]]/d' < test-data > input.txt
 sed -n 's/^# //p; /^$/p' < test-data > expout