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

Ilya Maximets i.maximets at ovn.org
Wed Oct 14 11:58:34 UTC 2020


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 at ovn.org>
>> Sent: Tuesday, 13 October, 2020 21:02
>> To: ovs-dev at openvswitch.org; Jan Scheurich <jan.scheurich at ericsson.com>
>> Cc: Ben Pfaff <blp at ovn.org>; Yi Yang <yi.y.yang at intel.com>; Ilya Maximets
>> <i.maximets at 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 at 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
> 



More information about the dev mailing list