From patchwork Tue Oct 13 19:02:06 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1381690 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4C9lKX2nS0z9sTK for ; Wed, 14 Oct 2020 06:02:24 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id E7BEC87CCA; Tue, 13 Oct 2020 19:02:22 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id S1qgK3y2UZJ8; Tue, 13 Oct 2020 19:02:20 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id D286E87CCB; Tue, 13 Oct 2020 19:02:20 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id B9D4BC07FF; Tue, 13 Oct 2020 19:02:20 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id C7BAFC0895 for ; Tue, 13 Oct 2020 19:02:17 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id B370B875F7 for ; Tue, 13 Oct 2020 19:02:17 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id WwpG+aHB5o46 for ; Tue, 13 Oct 2020 19:02:16 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by hemlock.osuosl.org (Postfix) with ESMTPS id 32BC687530 for ; Tue, 13 Oct 2020 19:02:16 +0000 (UTC) X-Originating-IP: 78.45.89.65 Received: from im-t490s.redhat.com (ip-78-45-89-65.net.upcbroadband.cz [78.45.89.65]) (Authenticated sender: i.maximets@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id D0246C0007; Tue, 13 Oct 2020 19:02:11 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org, Jan Scheurich Date: Tue, 13 Oct 2020 21:02:06 +0200 Message-Id: <20201013190206.2209670-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 Cc: Ilya Maximets Subject: [ovs-dev] [PATCH] ofp-ed-props: Fix using uninitialized padding for NSH encap actions. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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 --- 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