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 |
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
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 --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
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(-)