From patchwork Thu Jul 18 16:23:49 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Davide Caratti X-Patchwork-Id: 1962166 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; secure) header.d=lists.infradead.org header.i=@lists.infradead.org header.a=rsa-sha256 header.s=bombadil.20210309 header.b=YTgHYHkm; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20230601 header.b=H9Lyy0s6; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.infradead.org (client-ip=2607:7c80:54:3::133; helo=bombadil.infradead.org; envelope-from=hostap-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org; receiver=patchwork.ozlabs.org) Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:3::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4WPyrL4T6Hz20B2 for ; Fri, 19 Jul 2024 02:27:14 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date:Subject:Cc :To:From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=tZZ0P2W0FwbKHuVZmU0d0wwx2zfIF89A41OCCbjN0rA=; b=YTgHYHkm/elW3L EOAwyKAjD1zJwG00ZHadZdPDSH/ryq/x9u6GryQMscbL8wUMUAQjhqqoN5Lg+5rlTBnOKtJnh/+/j 0cz4wLfH1pLdG9hkm67CTd4mpIlTvJild29AWGGEmUaYwPTph3Xk/gKR9h37040ZdvnDA/cIY/hQY 6yk1CVFvu39mHlVB9u+yMitcFbkUQuJJcjW+0jppp6OzYufRzMGTk0PrJQ0mpu3Xog91v56S6q1ov ev7BWkjWgs9XV+Xo+cChd3yMwKAWf5SU3dJm27NuUrlQUXAPaGt/IRLSPddrDXEtSHl3PcW5aDyBI 5+aL4ZMbcaaXUe0ZfpKA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sUTxv-0000000HZlf-25X2; Thu, 18 Jul 2024 16:26:47 +0000 Received: from mail-wr1-x431.google.com ([2a00:1450:4864:20::431]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sUTxr-0000000HZkQ-1wyc for hostap@lists.infradead.org; Thu, 18 Jul 2024 16:26:44 +0000 Received: by mail-wr1-x431.google.com with SMTP id ffacd0b85a97d-368557c9e93so6569f8f.2 for ; Thu, 18 Jul 2024 09:26:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1721320001; x=1721924801; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=jWr2hnbPwmluvsZBsjtGAnIemjStDk3+JoEL7EX1koI=; b=H9Lyy0s6UzaflXKhvax5Wyw/MCFAgJdcLDFIyc0XZQPo4I/QfYdp2VKNFdC4d6dMUv e1p6oB+o3yjhCH/qJuWP52dJLQUetKVNfKZLK5dLZJOZfogtWEOQa8cmWDgdb1yeBW8k AxwVgWeyv4N6Kml4/riOL4oiKFr1R2KWVglosVq2LZI6BGQRwHQh/XqL0QuuAtslI6Hv HV1zTVyFZA7JJhCAoTFpj8vsjonC8h2c7M7lxXxUd52zvVFcvd1ojiXJIcXuCV5fxVGI DcGYe0Pb9fr91F9dLITvsZBxyigeqY2a7mT7fvZ9tN+jqsTruCqnaM/ReilFrdYpVOLt F6SQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721320001; x=1721924801; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=jWr2hnbPwmluvsZBsjtGAnIemjStDk3+JoEL7EX1koI=; b=ISz4QMCpWPa5N6DcEeRcjqfgb7B0+gv2JDpWqGapuNVbPuoxtl+UU8nE6JEWIBh5fG Q9Vm7Y4yvpdDpqB0lsJkUlIXHRygf7WZPhn5WRpKIRebvigCAC8iDUgFQlHNd8s/D/0V iuxk6D44sztnHHZQcGH6gGAoZeso5OmCjVyWs7qhBbILj2zLq32VwT2ZAZlAnQvdV/NI qZfOEPLboiC9N5ecMuyb60qNhPO9hHW05kLYp8OsGGkrb8u2H1XjC5TmT75I664NDG1y 4M0K+ZLuspoPhJiQrpE+QMxkoswOSSeiasdIE7Hxpuq7IuUIerKUBmN7nNPHo/4+pEZt EgZQ== X-Gm-Message-State: AOJu0Yy+kJ38mR1L37WT4Hm6I62Uf+w4WRyq1X2app5ul7Cb/TPnMWUn qMgMOAXSdpIZy17lX7FIfgrpoOy66ZuudPXrMcyiXWbJxLEO8YorWhE6TA== X-Google-Smtp-Source: AGHT+IGVk/MU2XFRmaxvWVH2JLKlhzoUjoe0WoM99t/NIxS4o48EAUfaAJFFmi9l5wrrOI/e/B4C7g== X-Received: by 2002:a5d:6704:0:b0:368:12ef:92d0 with SMTP id ffacd0b85a97d-3683171da9amr3099801f8f.51.1721320000729; Thu, 18 Jul 2024 09:26:40 -0700 (PDT) Received: from dcaratti.users.ipa.redhat.com ([2a01:e11:1007:ea0:8374:5c74:dd98:a7b2]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-427d292077dsm21758415e9.0.2024.07.18.09.26.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Jul 2024 09:26:40 -0700 (PDT) From: Davide Caratti To: Jouni Malinen Cc: hostap@lists.infradead.org Subject: [PATCH v2] P2P: fix memory leak when dbus provides bonjour params while adding a UPnP service Date: Thu, 18 Jul 2024 18:23:49 +0200 Message-ID: X-Mailer: git-send-email 2.45.2 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240718_092643_532372_D64DB6C6 X-CRM114-Status: GOOD ( 17.14 ) X-Spam-Score: -2.1 (--) X-Spam-Report: Spam detection software, running on the system "bombadil.infradead.org", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: Using D-Bus, it is possible to add a valid UPnP service where 'query' and 'response' are specified. In this case, memory for 'query' and 'response' is allocated but not used nor freed. Valgrind compla [...] Content analysis details: (-2.1 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [2a00:1450:4864:20:0:0:0:431 listed in] [list.dnswl.org] 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider [davide.caratti(at)gmail.com] X-BeenThere: hostap@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Hostap" Errors-To: hostap-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org Using D-Bus, it is possible to add a valid UPnP service where 'query' and 'response' are specified. In this case, memory for 'query' and 'response' is allocated but not used nor freed. Valgrind complains as follows: 42 bytes in 1 blocks are definitely lost in loss record 32 of 75 at 0x484C214: calloc (vg_replace_malloc.c:1675) by 0x41C673: wpabuf_alloc (wpabuf.c:124) by 0x41C673: wpabuf_alloc_copy (wpabuf.c:162) by 0x54F41A: wpas_dbus_handler_p2p_add_service (dbus_new_handlers_p2p.c:2762) by 0x53B9A2: msg_method_handler (dbus_new_helpers.c:356) by 0x53B9A2: message_handler (dbus_new_helpers.c:412) by 0x4EAB4B8: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.19.13) by 0x5495DF: dispatch_data (dbus_common.c:37) by 0x5495DF: process_watch (dbus_common.c:73) by 0x5495DF: process_watch_read (dbus_common.c:89) by 0x41EE8E: eloop_sock_table_dispatch.part.0 (eloop.c:603) by 0x41FA46: eloop_sock_table_dispatch (eloop.c:597) by 0x41FA46: eloop_run (eloop.c:1233) by 0x56A3CE: wpa_supplicant_run (wpa_supplicant.c:8074) by 0x40DB06: main (main.c:393) 49 bytes in 1 blocks are definitely lost in loss record 37 of 75 at 0x484C214: calloc (vg_replace_malloc.c:1675) by 0x41C673: wpabuf_alloc (wpabuf.c:124) by 0x41C673: wpabuf_alloc_copy (wpabuf.c:162) by 0x54F348: wpas_dbus_handler_p2p_add_service (dbus_new_handlers_p2p.c:2755) by 0x53B9A2: msg_method_handler (dbus_new_helpers.c:356) by 0x53B9A2: message_handler (dbus_new_helpers.c:412) by 0x4EAB4B8: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.19.13) by 0x5495DF: dispatch_data (dbus_common.c:37) by 0x5495DF: process_watch (dbus_common.c:73) by 0x5495DF: process_watch_read (dbus_common.c:89) by 0x41EE8E: eloop_sock_table_dispatch.part.0 (eloop.c:603) by 0x41FA46: eloop_sock_table_dispatch (eloop.c:597) by 0x41FA46: eloop_run (eloop.c:1233) by 0x56A3CE: wpa_supplicant_run (wpa_supplicant.c:8074) by 0x40DB06: main (main.c:393) Fix this ensuring that query and resp are freed both in the error and non-error path of wpas_dbus_handler_p2p_add_service(). Also, add a test in test_dbus.py to verify the correct behavior. Signed-off-by: Davide Caratti --- tests/hwsim/test_dbus.py | 14 +++++- wpa_supplicant/ctrl_iface.c | 49 +++++++++------------ wpa_supplicant/dbus/dbus_new_handlers_p2p.c | 12 ++--- wpa_supplicant/p2p_supplicant_sd.c | 14 +++++- 4 files changed, 52 insertions(+), 37 deletions(-) diff --git a/tests/hwsim/test_dbus.py b/tests/hwsim/test_dbus.py index 2c59d7fb7..454c1c86f 100644 --- a/tests/hwsim/test_dbus.py +++ b/tests/hwsim/test_dbus.py @@ -3406,12 +3406,22 @@ def test_dbus_p2p_service_discovery(dev, apdev): bonjour_query = dbus.ByteArray(binascii.unhexlify('0b5f6166706f766572746370c00c000c01')) bonjour_response = dbus.ByteArray(binascii.unhexlify('074578616d706c65c027')) + tests = [{'service_type': 'bonjour', + 'query': bonjour_query, + 'response': bonjour_response}, + {'service_type': 'upnp', + 'version': 0x10, + 'service': 'uuid:6859dede-8574-59ab-9332-123456789012::upnp:rootdevice', + 'query': bonjour_query, + 'response': bonjour_response}] + for args in tests: + p2p.AddService(args) + p2p.FlushService() + args = {'service_type': 'bonjour', 'query': bonjour_query, 'response': bonjour_response} p2p.AddService(args) - p2p.FlushService() - p2p.AddService(args) try: p2p.DeleteService(args) diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c index bc013ad99..b77d18479 100644 --- a/wpa_supplicant/ctrl_iface.c +++ b/wpa_supplicant/ctrl_iface.c @@ -6716,47 +6716,42 @@ static int p2p_ctrl_service_add_bonjour(struct wpa_supplicant *wpa_s, char *pos; size_t len; struct wpabuf *query, *resp; + int ret = -1; pos = os_strchr(cmd, ' '); if (pos == NULL) - return -1; + return ret; *pos++ = '\0'; len = os_strlen(cmd); if (len & 1) - return -1; + return ret; len /= 2; query = wpabuf_alloc(len); if (query == NULL) - return -1; - if (hexstr2bin(cmd, wpabuf_put(query, len), len) < 0) { - wpabuf_free(query); - return -1; - } - + return ret; + ret = hexstr2bin(cmd, wpabuf_put(query, len), len); + if (ret < 0) + goto err_query; + ret = -1; len = os_strlen(pos); - if (len & 1) { - wpabuf_free(query); - return -1; - } + if (len & 1) + goto err_query; len /= 2; resp = wpabuf_alloc(len); - if (resp == NULL) { - wpabuf_free(query); - return -1; - } - if (hexstr2bin(pos, wpabuf_put(resp, len), len) < 0) { - wpabuf_free(query); - wpabuf_free(resp); - return -1; - } + if (resp == NULL) + goto err_query; + ret = hexstr2bin(pos, wpabuf_put(resp, len), len); + if (ret < 0) + goto err_resp; - if (wpas_p2p_service_add_bonjour(wpa_s, query, resp) < 0) { - wpabuf_free(query); - wpabuf_free(resp); - return -1; - } - return 0; + ret = wpas_p2p_service_add_bonjour(wpa_s, query, resp); + +err_resp: + wpabuf_free(resp); +err_query: + wpabuf_free(query); + return ret; } diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c index d001c0154..b9eebb3d9 100644 --- a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c +++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c @@ -2778,20 +2778,20 @@ DBusMessage * wpas_dbus_handler_p2p_add_service(DBusMessage *message, if (wpas_p2p_service_add_bonjour(wpa_s, query, resp) < 0) goto error; - query = NULL; - resp = NULL; } else goto error; +out: os_free(service); + wpabuf_free(query); + wpabuf_free(resp); + return reply; error_clear: wpa_dbus_dict_entry_clear(&entry); error: - os_free(service); - wpabuf_free(query); - wpabuf_free(resp); - return wpas_dbus_error_invalid_args(message, NULL); + reply = wpas_dbus_error_invalid_args(message, NULL); + goto out; } diff --git a/wpa_supplicant/p2p_supplicant_sd.c b/wpa_supplicant/p2p_supplicant_sd.c index b400cbaca..1e2a733a4 100644 --- a/wpa_supplicant/p2p_supplicant_sd.c +++ b/wpa_supplicant/p2p_supplicant_sd.c @@ -1213,12 +1213,22 @@ int wpas_p2p_service_add_bonjour(struct wpa_supplicant *wpa_s, bsrv = os_zalloc(sizeof(*bsrv)); if (bsrv == NULL) return -1; - bsrv->query = query; - bsrv->resp = resp; + bsrv->query = wpabuf_dup(query); + if (!bsrv->query) + goto error_bsrv; + bsrv->resp = wpabuf_dup(resp); + if (!bsrv->resp) + goto error_query; dl_list_add(&wpa_s->global->p2p_srv_bonjour, &bsrv->list); wpas_p2p_sd_service_update(wpa_s); return 0; + +error_query: + wpabuf_free(bsrv->query); +error_bsrv: + os_free(bsrv); + return -1; }