From patchwork Wed Aug 29 00:46:14 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Pettit X-Patchwork-Id: 963183 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 420Rld2cWcz9rvt for ; Wed, 29 Aug 2018 10:46:53 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 5EB3CC21; Wed, 29 Aug 2018 00:46:28 +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 3AD8ABB6 for ; Wed, 29 Aug 2018 00:46:27 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id A8674466 for ; Wed, 29 Aug 2018 00:46:26 +0000 (UTC) X-Originating-IP: 76.21.1.228 Received: from localhost.localdomain (unknown [76.21.1.228]) (Authenticated sender: jpettit@ovn.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id BB79BE0003 for ; Wed, 29 Aug 2018 00:46:24 +0000 (UTC) From: Justin Pettit To: dev@openvswitch.org Date: Tue, 28 Aug 2018 17:46:14 -0700 Message-Id: <20180829004614.129668-2-jpettit@ovn.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180829004614.129668-1-jpettit@ovn.org> References: <20180829004614.129668-1-jpettit@ovn.org> 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 Subject: [ovs-dev] [PATCH 2/2] dpif-netdev: Prevent unsafe access when retrieving meter stats. 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 dpif_netdev_meter_get() retrieved a pointer to a meter entry without holding a lock. It's possible that another thread could have deleted that entry between retrieving the pointer and dereferencing the pointer. This makes the function hold the lock the entire time the meter entry is needed. Found by inspection. Signed-off-by: Justin Pettit Acked-by: Flavio Leitner --- lib/dpif-netdev.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 8b0b3745860b..7c0300cc554a 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -5243,20 +5243,22 @@ dpif_netdev_meter_get(const struct dpif *dpif, struct ofputil_meter_stats *stats, uint16_t n_bands) { const struct dp_netdev *dp = get_dp_netdev(dpif); - const struct dp_meter *meter; uint32_t meter_id = meter_id_.uint32; + int retval = 0; if (meter_id >= MAX_METERS) { return EFBIG; } - meter = dp->meters[meter_id]; + + meter_lock(dp, meter_id); + const struct dp_meter *meter = dp->meters[meter_id]; if (!meter) { - return ENOENT; + retval = ENOENT; + goto done; } if (stats) { int i = 0; - meter_lock(dp, meter_id); stats->packet_in_count = meter->packet_count; stats->byte_in_count = meter->byte_count; @@ -5264,11 +5266,13 @@ dpif_netdev_meter_get(const struct dpif *dpif, stats->bands[i].packet_count = meter->bands[i].packet_count; stats->bands[i].byte_count = meter->bands[i].byte_count; } - meter_unlock(dp, meter_id); stats->n_bands = i; } - return 0; + +done: + meter_unlock(dp, meter_id); + return retval; } static int