From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 9D16429C6 for ; Tue, 20 Nov 2018 17:18:49 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Nov 2018 08:18:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,257,1539673200"; d="scan'208";a="110110392" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by orsmga002.jf.intel.com with ESMTP; 20 Nov 2018 08:18:47 -0800 Received: from sivswdev01.ir.intel.com (sivswdev01.ir.intel.com [10.237.217.45]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id wAKGIkO1013897; Tue, 20 Nov 2018 16:18:46 GMT Received: from sivswdev01.ir.intel.com (localhost [127.0.0.1]) by sivswdev01.ir.intel.com with ESMTP id wAKGIktY007598; Tue, 20 Nov 2018 16:18:46 GMT Received: (from aburakov@localhost) by sivswdev01.ir.intel.com with LOCAL id wAKGIkSe007594; Tue, 20 Nov 2018 16:18:46 GMT From: Anatoly Burakov To: dev@dpdk.org Cc: thomas@monjalon.net Date: Tue, 20 Nov 2018 16:18:46 +0000 Message-Id: <69497bc41646c6dd5eebb097b3bdc8a2136aeb53.1542730524.git.anatoly.burakov@intel.com> X-Mailer: git-send-email 1.7.0.7 In-Reply-To: <3ac0562bbdfbabbcb2bdf5fec9e0ffa777475e2b.1542727150.git.anatoly.burakov@intel.com> References: <3ac0562bbdfbabbcb2bdf5fec9e0ffa777475e2b.1542727150.git.anatoly.burakov@intel.com> Subject: [dpdk-dev] [PATCH v2] ipc: fix use-after-free on failed send 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: , X-List-Received-Date: Tue, 20 Nov 2018 16:18:50 -0000 Previous fix for rte_panic has moved setting of alarm before sending the message. This means that whether we send a message, the alarm would still trigger. The comment noted that cleanup would happen in the alarm handler, but that's not what actually happened - instead, in the event of failed send we freed the memory in-place, before putting the request on the queue. This works OK when the message is sent, but when sending the message fails, the alarm would still trigger with a pointer argument that points to non-existent memory, and cause memory corruption. There probably is a "proper" fix for this issue, with correct handling of sent vs. unsent requests, however it would be simpler just to sacrifice the sent request in the (extremely unlikely) event of alarm set failing. The other process would still send a response, but it will be ignored by the sender. Fixes: 45e5f49e87fb ("ipc: remove panic in async request") Signed-off-by: Anatoly Burakov --- Notes: v2: - Move incrementing of nb_sent to before setting the alarm. This will let the API consumers know that some messages were sent, but failed to receive a response (due to failure of IPC to set the alarm). lib/librte_eal/common/eal_common_proc.c | 40 ++++++++----------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c index f65ef56c0..1c3f09aad 100644 --- a/lib/librte_eal/common/eal_common_proc.c +++ b/lib/librte_eal/common/eal_common_proc.c @@ -827,20 +827,19 @@ mp_request_async(const char *dst, struct rte_mp_msg *req, goto fail; } - /* - * set the alarm before sending message. there are two possible error - * scenarios to consider here: - * - * - if the alarm set fails, we free the memory right there - * - if the alarm set succeeds but sending message fails, then the alarm - * will trigger and clean up the memory - * - * Even if the alarm triggers too early (i.e. immediately), we're still - * holding the lock to pending requests queue, so the interrupt thread - * will just spin until we release the lock, and either release the - * memory, or doesn't find any pending requests in the queue because we - * never added any due to send message failure. - */ + ret = send_msg(dst, req, MP_REQ); + if (ret < 0) { + RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n", + dst, req->name); + ret = -1; + goto fail; + } else if (ret == 0) { + ret = 0; + goto fail; + } + param->user_reply.nb_sent++; + + /* if alarm set fails, we simply ignore the reply */ if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000, async_reply_handle, pending_req) < 0) { RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n", @@ -848,21 +847,8 @@ mp_request_async(const char *dst, struct rte_mp_msg *req, ret = -1; goto fail; } - - ret = send_msg(dst, req, MP_REQ); - if (ret < 0) { - RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n", - dst, req->name); - ret = -1; - goto fail; - } else if (ret == 0) { - ret = 0; - goto fail; - } TAILQ_INSERT_TAIL(&pending_requests.requests, pending_req, next); - param->user_reply.nb_sent++; - return 0; fail: free(pending_req); -- 2.17.1