| Message ID | 20201013190206.2209670-1-i.maximets@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofp-ed-props: Fix using uninitialized padding for NSH encap actions. | expand |
Hi Ilya, Good catch. One comment below. /Jan > -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Tuesday, 13 October, 2020 21:02 > To: ovs-dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com> > Cc: Ben Pfaff <blp@ovn.org>; Yi Yang <yi.y.yang@intel.com>; Ilya Maximets > <i.maximets@ovn.org> > Subject: [PATCH] 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' 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 checking that these > bytes are all zeros on decoding. Is the latter strictly necessary? It may break existing controllers that do not initialize the padding bytes to zero. Wouldn't it be sufficient to just zero the padding bytes at reception? > > 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 | 4 ++++ > tests/ofp-actions.at | 11 +++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index > 28382e012..5a4b12d9f 100644 > --- a/lib/ofp-ed-props.c > +++ b/lib/ofp-ed-props.c > @@ -48,6 +48,9 @@ decode_ed_prop(const struct ofp_ed_prop_header > **ofp_prop, > if (len > sizeof(*opnmt) || len > *remaining) { > return OFPERR_NXBAC_BAD_ED_PROP; > } > + if (!is_all_zeros(opnmt->pad, sizeof opnmt->pad)) { > + return OFPERR_NXBRC_MUST_BE_ZERO; > + } > struct ofpact_ed_prop_nsh_md_type *pnmt = > ofpbuf_put_uninit(out, sizeof(*pnmt)); > pnmt->header.prop_class = prop_class; @@ -108,6 +111,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..18ba8206f 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. > +# bad OpenFlow13 actions: NXBRC_MUST_BE_ZERO & > ofp_actions|WARN|bad > +action at offset 0 (NXBRC_MUST_BE_ZERO): > +& 00000000 ff ff 00 18 00 00 23 20-00 2e 00 00 00 01 89 4f & 00000010 > +00 04 01 05 01 00 00 01- ffff 0018 00002320 002e 0000 0001894f 0004 01 > +05 01 000001 > + > ]) > sed '/^[[#&]]/d' < test-data > input.txt sed -n 's/^# //p; /^$/p' < test-data > > expout > -- > 2.25.4
On 10/14/20 9:52 AM, Jan Scheurich wrote: > Hi Ilya, > > Good catch. One comment below. That wasn't an easy debugging session. We need more tests for OF {en,de}coding. Surprisingly and sadly, tests/ofp-actions.at and tests/ovs-ofctl.at does not cover a lot of matches and actions. > > /Jan > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Tuesday, 13 October, 2020 21:02 >> To: ovs-dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com> >> Cc: Ben Pfaff <blp@ovn.org>; Yi Yang <yi.y.yang@intel.com>; Ilya Maximets >> <i.maximets@ovn.org> >> Subject: [PATCH] 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' 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 checking that these >> bytes are all zeros on decoding. > > Is the latter strictly necessary? It may break existing controllers that do not initialize the padding bytes to zero. > Wouldn't it be sufficient to just zero the padding bytes at reception? I do not have a strong opinion. I guess, we could not fail OF request if padding is not all zeroes for backward compatibility. Anyway, it seems like I missed one part of this change (see inline). On the other hand, AFAIU, NXOXM_NSH_ is not standardized, so, technically, we could change the rules here. As an option, we could apply the patch without checking for all-zeroes padding and backport it this way to stable branches. Afterwards, we could introduce the 'is_all_zeros' check and mention this change in release notes for the new version. Anyway OpenFlow usually requires paddings to be all-zeroes for most of matches and actions, so this should be a sane requirement for controllers. What do you think? > >> >> 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 | 4 ++++ >> tests/ofp-actions.at | 11 +++++++++++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index >> 28382e012..5a4b12d9f 100644 >> --- a/lib/ofp-ed-props.c >> +++ b/lib/ofp-ed-props.c >> @@ -48,6 +48,9 @@ decode_ed_prop(const struct ofp_ed_prop_header >> **ofp_prop, >> if (len > sizeof(*opnmt) || len > *remaining) { >> return OFPERR_NXBAC_BAD_ED_PROP; >> } >> + if (!is_all_zeros(opnmt->pad, sizeof opnmt->pad)) { >> + return OFPERR_NXBRC_MUST_BE_ZERO; >> + } >> struct ofpact_ed_prop_nsh_md_type *pnmt = >> ofpbuf_put_uninit(out, sizeof(*pnmt)); This should be 'ofpbuf_put_zeroes' because 'struct ofpact_ed_prop_nsh_md_type' contains padding too that must be cleared while constructing ofpacts. Since OVS compares decoded ofpacts' and not the original OF messages, this should do the trick. I'll send v2 with this change and will remove 'is_all_zeros' check for this fix. >> pnmt->header.prop_class = prop_class; @@ -108,6 +111,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..18ba8206f 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. >> +# bad OpenFlow13 actions: NXBRC_MUST_BE_ZERO & >> ofp_actions|WARN|bad >> +action at offset 0 (NXBRC_MUST_BE_ZERO): >> +& 00000000 ff ff 00 18 00 00 23 20-00 2e 00 00 00 01 89 4f & 00000010 >> +00 04 01 05 01 00 00 01- ffff 0018 00002320 002e 0000 0001894f 0004 01 >> +05 01 000001 >> + >> ]) >> sed '/^[[#&]]/d' < test-data > input.txt sed -n 's/^# //p; /^$/p' < test-data > >> expout >> -- >> 2.25.4 >
> >> Fix that by clearing padding bytes while encoding, and checking that > >> these bytes are all zeros on decoding. > > > > Is the latter strictly necessary? It may break existing controllers that do not > initialize the padding bytes to zero. > > Wouldn't it be sufficient to just zero the padding bytes at reception? > > I do not have a strong opinion. I guess, we could not fail OF request if > padding is not all zeroes for backward compatibility. > Anyway, it seems like I missed one part of this change (see inline). > > On the other hand, AFAIU, NXOXM_NSH_ is not standardized, so, technically, > we could change the rules here. As an option, we could apply the patch > without checking for all-zeroes padding and backport it this way to stable > branches. Afterwards, we could introduce the 'is_all_zeros' check and > mention this change in release notes for the new version. Anyway OpenFlow > usually requires paddings to be all-zeroes for most of matches and actions, so > this should be a sane requirement for controllers. > What do you think? > I think there is little to gain by enforcing strict rules on zeroed padding bytes in a future release. It just creates grief with users of OVS by unnecessarily breaking backward compatibility without any benefit for OVS. No matter if OVS is has the right to do so or not. > >> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index > >> 28382e012..5a4b12d9f 100644 > >> --- a/lib/ofp-ed-props.c > >> +++ b/lib/ofp-ed-props.c > >> @@ -48,6 +48,9 @@ decode_ed_prop(const struct ofp_ed_prop_header > >> **ofp_prop, > >> if (len > sizeof(*opnmt) || len > *remaining) { > >> return OFPERR_NXBAC_BAD_ED_PROP; > >> } > >> + if (!is_all_zeros(opnmt->pad, sizeof opnmt->pad)) { > >> + return OFPERR_NXBRC_MUST_BE_ZERO; > >> + } > >> struct ofpact_ed_prop_nsh_md_type *pnmt = > >> ofpbuf_put_uninit(out, sizeof(*pnmt)); > > This should be 'ofpbuf_put_zeroes' because 'struct > ofpact_ed_prop_nsh_md_type' > contains padding too that must be cleared while constructing ofpacts. > Since OVS compares decoded ofpacts' and not the original OF messages, this > should do the trick. Agree. > > I'll send v2 with this change and will remove 'is_all_zeros' check for this fix. Thanks, Jan
On 10/14/20 2:36 PM, Jan Scheurich wrote: >>>> Fix that by clearing padding bytes while encoding, and checking that >>>> these bytes are all zeros on decoding. >>> >>> Is the latter strictly necessary? It may break existing controllers that do not >> initialize the padding bytes to zero. >>> Wouldn't it be sufficient to just zero the padding bytes at reception? >> >> I do not have a strong opinion. I guess, we could not fail OF request if >> padding is not all zeroes for backward compatibility. >> Anyway, it seems like I missed one part of this change (see inline). >> >> On the other hand, AFAIU, NXOXM_NSH_ is not standardized, so, technically, >> we could change the rules here. As an option, we could apply the patch >> without checking for all-zeroes padding and backport it this way to stable >> branches. Afterwards, we could introduce the 'is_all_zeros' check and >> mention this change in release notes for the new version. Anyway OpenFlow >> usually requires paddings to be all-zeroes for most of matches and actions, so >> this should be a sane requirement for controllers. >> What do you think? >> > > I think there is little to gain by enforcing strict rules on zeroed padding bytes in a future release. It just creates grief with users of OVS by unnecessarily breaking backward compatibility without any benefit for OVS. No matter if OVS is has the right to do so or not. OK. Agree. > >>>> diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index >>>> 28382e012..5a4b12d9f 100644 >>>> --- a/lib/ofp-ed-props.c >>>> +++ b/lib/ofp-ed-props.c >>>> @@ -48,6 +48,9 @@ decode_ed_prop(const struct ofp_ed_prop_header >>>> **ofp_prop, >>>> if (len > sizeof(*opnmt) || len > *remaining) { >>>> return OFPERR_NXBAC_BAD_ED_PROP; >>>> } >>>> + if (!is_all_zeros(opnmt->pad, sizeof opnmt->pad)) { >>>> + return OFPERR_NXBRC_MUST_BE_ZERO; >>>> + } >>>> struct ofpact_ed_prop_nsh_md_type *pnmt = >>>> ofpbuf_put_uninit(out, sizeof(*pnmt)); >> >> This should be 'ofpbuf_put_zeroes' because 'struct >> ofpact_ed_prop_nsh_md_type' >> contains padding too that must be cleared while constructing ofpacts. >> Since OVS compares decoded ofpacts' and not the original OF messages, this >> should do the trick. > > Agree. > >> >> I'll send v2 with this change and will remove 'is_all_zeros' check for this fix. > > Thanks, Jan >
diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index 28382e012..5a4b12d9f 100644 --- a/lib/ofp-ed-props.c +++ b/lib/ofp-ed-props.c @@ -48,6 +48,9 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, if (len > sizeof(*opnmt) || len > *remaining) { return OFPERR_NXBAC_BAD_ED_PROP; } + if (!is_all_zeros(opnmt->pad, sizeof opnmt->pad)) { + return OFPERR_NXBRC_MUST_BE_ZERO; + } struct ofpact_ed_prop_nsh_md_type *pnmt = ofpbuf_put_uninit(out, sizeof(*pnmt)); pnmt->header.prop_class = prop_class; @@ -108,6 +111,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..18ba8206f 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. +# bad OpenFlow13 actions: NXBRC_MUST_BE_ZERO +& ofp_actions|WARN|bad action at offset 0 (NXBRC_MUST_BE_ZERO): +& 00000000 ff ff 00 18 00 00 23 20-00 2e 00 00 00 01 89 4f +& 00000010 00 04 01 05 01 00 00 01- +ffff 0018 00002320 002e 0000 0001894f 0004 01 05 01 000001 + ]) 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' 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 checking that these bytes are all zeros on decoding. 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 | 4 ++++ tests/ofp-actions.at | 11 +++++++++++ 2 files changed, 15 insertions(+)