From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7F344A04B5; Tue, 3 Dec 2019 22:15:54 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BA6271BF92; Tue, 3 Dec 2019 22:15:53 +0100 (CET) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by dpdk.org (Postfix) with ESMTP id 616681BF82 for ; Tue, 3 Dec 2019 22:15:52 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575407751; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=JEOXf1Xua6NSa1ZUGc8ZMKlpIxzAR86bprrySOY/d6E=; b=Kvm9Jc59n0iguIcxe4A+jvsYO9/d2N9vznL2lOwD+4POy3MmxLWktcd0jk0XduyKEzQyBw jYp7iLzN/W5HPRn7AkI0jDVI7eXFBcNhKvX7DCu43PwXfBtRVQItVymtkKDXIVGHUJYcdG RLGSrlyVRyjVMu+GmqNsRsZKYy+QEcA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-234-T7k2j7g_MNqmAsLmsMBFNg-1; Tue, 03 Dec 2019 16:15:50 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 53A081005516; Tue, 3 Dec 2019 21:15:49 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-123-35.rdu2.redhat.com [10.10.123.35]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9185C1001B03; Tue, 3 Dec 2019 21:15:47 +0000 (UTC) From: Aaron Conole To: dev@dpdk.org Cc: Harry van Haaren , Bruce Richardson , Pavan Nikhilesh , Gage Eads , Thomas Monjalon , David Marchand Date: Tue, 3 Dec 2019 16:15:44 -0500 Message-Id: <20191203211544.20285-1-aconole@redhat.com> In-Reply-To: <20191126145606.13626-1-aconole@redhat.com> References: <20191126145606.13626-1-aconole@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-MC-Unique: T7k2j7g_MNqmAsLmsMBFNg-1 X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Subject: [dpdk-dev] [PATCH v2] service: don't walk out of bounds when checking services X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" The service_valid call is used without properly bounds checking the input parameter. Almost all instances of the service_valid call are inside a for() loop that prevents excessive walks, but some of the public APIs don't bounds check and will pass invalid arguments. Prevent this by using SERVICE_GET_OR_ERR_RET where it makes sense, and adding a bounds check to one service_valid() use. Fixes: 8d39d3e237c2 ("service: fix race in service on app lcore function") Fixes: e9139a32f6e8 ("service: add function to run on app lcore") Fixes: e30dd31847d2 ("service: add mechanism for quiescing") Signed-off-by: Aaron Conole --- v2: - make for() loop consistent and service_run() always require a valid 's' pointer - introduce service_get() for a future patch to clean up the bare references to 'rte_services[i]' - remove some useless 'inline' specifiers on functions (they aren't needed in .c files). a future patch can clean up the others. lib/librte_eal/common/rte_service.c | 32 ++++++++++++++++++----------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rt= e_service.c index 79235c03f8..7e537b8cd2 100644 --- a/lib/librte_eal/common/rte_service.c +++ b/lib/librte_eal/common/rte_service.c @@ -137,6 +137,12 @@ service_valid(uint32_t id) =09return !!(rte_services[id].internal_flags & SERVICE_F_REGISTERED); } =20 +static struct rte_service_spec_impl * +service_get(uint32_t id) +{ +=09return &rte_services[id]; +} + /* validate ID and retrieve service pointer, or return error value */ #define SERVICE_VALID_GET_OR_ERR_RET(id, service, retval) do { \ =09if (id >=3D RTE_SERVICE_NUM_MAX || !service_valid(id)) \ @@ -344,12 +350,14 @@ rte_service_runner_do_callback(struct rte_service_spe= c_impl *s, } =20 =20 -static inline int32_t -service_run(uint32_t i, struct core_state *cs, uint64_t service_mask) +/* Expects the service 's' is valid. */ +static int32_t +service_run(uint32_t i, struct core_state *cs, uint64_t service_mask, +=09 struct rte_service_spec_impl *s) { -=09if (!service_valid(i)) +=09if (!s) =09=09return -EINVAL; -=09struct rte_service_spec_impl *s =3D &rte_services[i]; + =09if (s->comp_runstate !=3D RUNSTATE_RUNNING || =09=09=09s->app_runstate !=3D RUNSTATE_RUNNING || =09=09=09!(service_mask & (UINT64_C(1) << i))) { @@ -383,7 +391,7 @@ rte_service_may_be_active(uint32_t id) =09int32_t lcore_count =3D rte_service_lcore_list(ids, RTE_MAX_LCORE); =09int i; =20 -=09if (!service_valid(id)) +=09if (id >=3D RTE_SERVICE_NUM_MAX || !service_valid(id)) =09=09return -EINVAL; =20 =09for (i =3D 0; i < lcore_count; i++) { @@ -397,12 +405,10 @@ rte_service_may_be_active(uint32_t id) int32_t rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t serialize_mt_unsaf= e) { -=09/* run service on calling core, using all-ones as the service mask */ -=09if (!service_valid(id)) -=09=09return -EINVAL; - =09struct core_state *cs =3D &lcore_states[rte_lcore_id()]; -=09struct rte_service_spec_impl *s =3D &rte_services[id]; +=09struct rte_service_spec_impl *s; + +=09SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL); =20 =09/* Atomically add this core to the mapped cores first, then examine if =09 * we can run the service. This avoids a race condition between @@ -418,7 +424,7 @@ rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t= serialize_mt_unsafe) =09=09return -EBUSY; =09} =20 -=09int ret =3D service_run(id, cs, UINT64_MAX); +=09int ret =3D service_run(id, cs, UINT64_MAX, s); =20 =09if (serialize_mt_unsafe) =09=09rte_atomic32_dec(&s->num_mapped_cores); @@ -438,8 +444,10 @@ rte_service_runner_func(void *arg) =09=09const uint64_t service_mask =3D cs->service_mask; =20 =09=09for (i =3D 0; i < RTE_SERVICE_NUM_MAX; i++) { +=09=09=09if (!service_valid(i)) +=09=09=09=09continue; =09=09=09/* return value ignored as no change to code flow */ -=09=09=09service_run(i, cs, service_mask); +=09=09=09service_run(i, cs, service_mask, service_get(i)); =09=09} =20 =09=09cs->loops++; --=20 2.21.0