From patchwork Wed Aug 15 21:57:13 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 958053 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.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 mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41rNc71pXLz9sBq for ; Thu, 16 Aug 2018 07:57:26 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 1A571D4B; Wed, 15 Aug 2018 21:57:24 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id CDC14D21 for ; Wed, 15 Aug 2018 21:57:22 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 339AD7CA for ; Wed, 15 Aug 2018 21:57:21 +0000 (UTC) X-Originating-IP: 208.91.3.26 Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 53F461BF207; Wed, 15 Aug 2018 21:57:18 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Wed, 15 Aug 2018 14:57:13 -0700 Message-Id: <20180815215713.21435-1-blp@ovn.org> X-Mailer: git-send-email 2.16.1 X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH] ofp-actions: Avoid assertion failure for clone(ct(...bad actions...)). X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org decode_NXAST_RAW_CT() temporarily pulls data off the beginning of its ofpacts output ofpbuf and, on its error path, fails to push it back on. At a higher layer, decode_NXAST_RAW_CLONE() asserts, via ofpact_finish_CLONE(), that the ofpact_clone that it put is still in the place where it put it, which causes an assertion failure. The root cause here is the failure to re-push the clone header. One could fix that, but it would be pretty easy for that to go wrong again on some other obscure error path. Instead, this commit just makes the problem go away by always saving and restoring 'ofpact->data' if a decode fails. Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9862 Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- lib/ofp-actions.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 93e212f07196..6adb55d23b02 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -7500,6 +7500,7 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow, uint64_t *ofpacts_tlv_bitmap) { const struct ofp_action_header *actions; + void *orig_data = ofpacts->data; size_t orig_size = ofpacts->size; enum ofperr error; @@ -7519,14 +7520,17 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow, error = ofpacts_decode(actions, actions_len, version, vl_mff_map, ofpacts_tlv_bitmap, ofpacts); - if (error) { - ofpacts->size = orig_size; - return error; + if (!error) { + error = ofpacts_verify(ofpacts->data, ofpacts->size, allowed_ovsinsts, + outer_action); } - - error = ofpacts_verify(ofpacts->data, ofpacts->size, allowed_ovsinsts, - outer_action); if (error) { + /* Back out changes to 'ofpacts'. (Normally, decoding would only + * append to 'ofpacts', so that one would expect only to need to + * restore 'ofpacts->size', but some action parsing temporarily pulls + * off data from the start of 'ofpacts' and may not properly re-push it + * on error paths.) */ + ofpacts->data = orig_data; ofpacts->size = orig_size; } return error;