Skip to content

Commit

Permalink
ofp-ed-props: Fix using uninitialized padding for NSH encap actions.
Browse files Browse the repository at this point in the history
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: 1fc11c5 ("Generic encap and decap support for NSH")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>
  • Loading branch information
igsilya committed Oct 15, 2020
1 parent b792c5e commit e46d11e
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
3 changes: 2 additions & 1 deletion lib/ofp-ed-props.c
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
11 changes: 11 additions & 0 deletions tests/ofp-actions.at
Expand Up @@ -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
Expand Down

0 comments on commit e46d11e

Please sign in to comment.