From patchwork Fri Nov 10 04:45:39 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Grant Erickson X-Patchwork-Id: 13452124 Received: from mohas.pair.com (mohas.pair.com [209.68.5.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 84E11137F for ; Fri, 10 Nov 2023 04:45:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=nuovations.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=nuovations.com Authentication-Results: smtp.subspace.kernel.org; dkim=none Received: from mohas.pair.com (localhost [127.0.0.1]) by mohas.pair.com (Postfix) with ESMTP id 9679C7315A for ; Thu, 9 Nov 2023 23:45:40 -0500 (EST) Received: from [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44] (unknown [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mohas.pair.com (Postfix) with ESMTPSA id 61E64731F7 for ; Thu, 9 Nov 2023 23:45:40 -0500 (EST) From: Grant Erickson Precedence: bulk X-Mailing-List: connman@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: [PATCH 1/4] wispr: Add DBG statement to 'free_connman_wispr_portal_context'. Date: Thu, 9 Nov 2023 20:45:39 -0800 References: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> To: connman@lists.linux.dev In-Reply-To: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> Message-Id: X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Scanned-By: mailmunge 3.11 on 209.68.5.112 This adds a ‘DBG' statement to 'free_connman_wispr_portal_context' to aid in debugging WISPr/portal reference counting errors and correlating WISPr online checks in flight in a multi-technology environment with "EnableOnlineToReadyTransition" asserted. --- src/wispr.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/wispr.c b/src/wispr.c index 57e74a83..5f60bed7 100644 --- a/src/wispr.c +++ b/src/wispr.c @@ -164,6 +164,13 @@ static void free_wispr_routes(struct connman_wispr_portal_context *wp_context) static void free_connman_wispr_portal_context( struct connman_wispr_portal_context *wp_context) { + DBG("wispr/portal context %p service %p (%s) type %d (%s)", + wp_context, + wp_context->service, + connman_service_get_identifier(wp_context->service), + wp_context->type, + __connman_ipconfig_type2string(wp_context->type)); + if (wp_context->wispr_portal) { if (wp_context->wispr_portal->ipv4_context == wp_context) wp_context->wispr_portal->ipv4_context = NULL; From patchwork Fri Nov 10 04:46:01 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Grant Erickson X-Patchwork-Id: 13452125 Received: from mohas.pair.com (mohas.pair.com [209.68.5.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EAB94111B for ; Fri, 10 Nov 2023 04:46:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=nuovations.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=nuovations.com Authentication-Results: smtp.subspace.kernel.org; dkim=none Received: from mohas.pair.com (localhost [127.0.0.1]) by mohas.pair.com (Postfix) with ESMTP id 05AE07312C for ; Thu, 9 Nov 2023 23:46:02 -0500 (EST) Received: from [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44] (unknown [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mohas.pair.com (Postfix) with ESMTPSA id B8E2A731F3 for ; Thu, 9 Nov 2023 23:46:01 -0500 (EST) From: Grant Erickson Precedence: bulk X-Mailing-List: connman@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: [PATCH 2/4] wispr: Document 'wispr_portal_hash'. Date: Thu, 9 Nov 2023 20:46:01 -0800 References: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> To: connman@lists.linux.dev In-Reply-To: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> Message-Id: <34261BE2-28DF-4FD2-A552-39FC32000E46@nuovations.com> X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Scanned-By: mailmunge 3.11 on 209.68.5.112 This adds a documentation comment for the 'wispr_portal_hash' global. --- src/wispr.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wispr.c b/src/wispr.c index 5f60bed7..96c5f454 100644 --- a/src/wispr.c +++ b/src/wispr.c @@ -93,6 +93,11 @@ struct connman_wispr_portal { static bool wispr_portal_web_result(GWebResult *result, gpointer user_data); +/** + * A dictionary / hash table of network interface indices to + * active / outstanding WISPr / portal request contexts. + * + */ static GHashTable *wispr_portal_hash = NULL; static char *online_check_ipv4_url = NULL; From patchwork Fri Nov 10 04:46:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Grant Erickson X-Patchwork-Id: 13452126 Received: from mohas.pair.com (mohas.pair.com [209.68.5.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 26EA91845 for ; Fri, 10 Nov 2023 04:46:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=nuovations.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=nuovations.com Authentication-Results: smtp.subspace.kernel.org; dkim=none Received: from mohas.pair.com (localhost [127.0.0.1]) by mohas.pair.com (Postfix) with ESMTP id 50C0F731F7 for ; Thu, 9 Nov 2023 23:46:27 -0500 (EST) Received: from [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44] (unknown [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mohas.pair.com (Postfix) with ESMTPSA id 1A31073214 for ; Thu, 9 Nov 2023 23:46:27 -0500 (EST) From: Grant Erickson Precedence: bulk X-Mailing-List: connman@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: [PATCH 3/4] wispr: Add documentation about reference counting. Date: Thu, 9 Nov 2023 20:46:26 -0800 References: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> To: connman@lists.linux.dev In-Reply-To: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> Message-Id: <7DC71CAA-74A1-4437-8956-4F0B8AB8E13C@nuovations.com> X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Scanned-By: mailmunge 3.11 on 209.68.5.112 This adds documentation about the balanced retain/release reference counting done to handle the weak reference to the WISPr/portal context maintained by 'g_web_request_get'. --- src/wispr.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/wispr.c b/src/wispr.c index 96c5f454..b496b7f9 100644 --- a/src/wispr.c +++ b/src/wispr.c @@ -574,6 +574,11 @@ static void wispr_portal_request_portal( connman_service_get_identifier(wp_context->service), wp_context->type, __connman_ipconfig_type2string(wp_context->type)); + /* + * Retain a reference to the WISPr/portal context to account for + * 'g_web_request_get' maintaining a weak reference to it. This will + * be released in the 'wispr_portal_web_result' callback. + */ wispr_portal_context_ref(wp_context); wp_context->request_id = g_web_request_get(wp_context->web, wp_context->status_url, @@ -873,6 +878,12 @@ static bool wispr_portal_web_result(GWebResult *result, gpointer user_data) wp_context->request_id = 0; done: wp_context->wispr_msg.message_type = -1; + + /* + * Release a reference to the WISPr/portal context to balance the + * earlier reference retained to account for 'g_web_request_get' + * maintaining a weak reference to it. + */ wispr_portal_context_unref(wp_context); return false; } From patchwork Fri Nov 10 04:47:17 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Grant Erickson X-Patchwork-Id: 13452127 Received: from mohas.pair.com (mohas.pair.com [209.68.5.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 157B61845 for ; Fri, 10 Nov 2023 04:47:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=nuovations.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=nuovations.com Authentication-Results: smtp.subspace.kernel.org; dkim=none Received: from mohas.pair.com (localhost [127.0.0.1]) by mohas.pair.com (Postfix) with ESMTP id 78BF97314E for ; Thu, 9 Nov 2023 23:47:18 -0500 (EST) Received: from [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44] (unknown [IPv6:2601:647:5a00:15c1:8c0c:4eeb:e12e:6e44]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mohas.pair.com (Postfix) with ESMTPSA id 42595731CE for ; Thu, 9 Nov 2023 23:47:18 -0500 (EST) From: Grant Erickson Precedence: bulk X-Mailing-List: connman@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: [PATCH 4/4] wispr: Address unbalanced reference counting with proxy handling. Date: Thu, 9 Nov 2023 20:47:17 -0800 References: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> To: connman@lists.linux.dev In-Reply-To: <0CA760E5-A632-43E0-924A-EB4363ABE1BF@nuovations.com> Message-Id: X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Scanned-By: mailmunge 3.11 on 209.68.5.112 This balances previously-unbalanced WISPr/portal context reference counting associated with proxy handling that can lead to a heap-use- after-free fault. Prior to this change, 'proxy_callback' contained an unbalanced release on the reference to a WISPr/portal context that could lead to its premature destructio and a subsequent heap-use-after-free fault in 'free_wispr_routes'. This situation could occur, due to this imbalance, when a new WISPr/portal request for given service and IP configuration combination displaced a prior, but unclosed and pending within 'g_web_request_get' request, WISPr/portal context leading to its premature deletion. Later, when the prior 'g_web_request_get' completed, either successfully or unsuccessfully, this resulted in a different, yet balanced release on the reference at the end of the 'wispr_portal_web_result' callback to 'g_web_request_get'. This also resulted in its (now redundant) destruction. The heap-use-after-free actually occurred BEFORE the reference release, in 'free_wispr_routes' since in 'wispr_portal_web_result', 'free_wispr_routes' is invoked BEFORE the reference is released. This imbalance is addressed by balancing the reference release in 'proxy_callback' with a reference retain prior to either invoking 'connman_proxy_lookup' with the 'proxy_callback' callback or invoking 'g_idle_add' with the 'no_proxy_callback', which ultimately calls 'proxy_callback'. The address sanitizer report associated with the potential heap-use- after-free is as follows: ================================================================= ==2188==ERROR: AddressSanitizer: heap-use-after-free on address 0x72b0f4b4 at pc 0x000f36b5 bp 0x7eac98c8 sp 0x7eac98cc READ of size 4 at 0x72b0f4b4 thread T0 #0 0xf36b2 in free_wispr_routes src/wispr.c:140 #1 0xf5fde in wispr_portal_web_result src/wispr.c:871 #2 0x3285e in call_result_func gweb/gweb.c:465 #3 0x3285e in received_data gweb/gweb.c:920 0x72b0f4b4 is located 100 bytes inside of 108-byte region [0x72b0f450,0x72b0f4bc) freed by thread T0 here: #0 0x76abf264 in __interceptor_free /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52 #1 0xf3916 in free_connman_wispr_portal_context src/wispr.c:211 #2 0xf4706 in wispr_portal_context_unref_debug src/wispr.c:239 #3 0xf6408 in __connman_wispr_start src/wispr.c:1101 #4 0x9830a in __connman_service_wispr_start src/service.c:2421 #5 0x990d2 in start_wispr_if_connected src/service.c:2375 #6 0x9ae2c in default_changed src/service.c:2637 #7 0x9afb6 in __connman_service_indicate_default src/service.c:7660 #8 0x856de in set_default_gateway src/connection.c:509 #9 0x86e3a in __connman_connection_update_gateway src/connection.c:1053 #10 0x9b0aa in service_update_preferred_order src/service.c:7320 #11 0x9b2d2 in service_indicate_state src/service.c:7400 #12 0x9baaa in __connman_service_ipconfig_indicate_state src/service.c:7848 #13 0x9ed28 in complete_online_check src/service.c:2219 #14 0xf44fc in portal_manage_success_status src/wispr.c:516 #15 0xf60ac in wispr_portal_web_result src/wispr.c:820 #16 0x3285e in call_result_func gweb/gweb.c:465 #17 0x3285e in received_data gweb/gweb.c:920 previously allocated by thread T0 here: #0 0x76abfa6c in __interceptor_calloc /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77 SUMMARY: AddressSanitizer: heap-use-after-free src/wispr.c:140 in free_wispr_routes --- src/wispr.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/wispr.c b/src/wispr.c index b496b7f9..f3ce123d 100644 --- a/src/wispr.c +++ b/src/wispr.c @@ -1033,9 +1033,18 @@ static int wispr_portal_detect(struct connman_wispr_portal_context *wp_context) * connman_proxy_lookup, since the former will always result in * a WISPr request "falling down a hole" that will only ever * result in a failure completion. + * + * Whether following the connman_proxy_lookup / proxy_callback + * path or the g_idle_add / no_proxy_callback path, both funnel to + * proxy_callback. Retain a reference to the WISPr/portal context + * here and release it there (that is, in proxy_callback) such + * that the context is retained while the lookup or the idle + * timeout are in flight. */ if (proxy_method != CONNMAN_SERVICE_PROXY_METHOD_DIRECT && proxy_method != CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN) { + wispr_portal_context_ref(wp_context); + wp_context->token = connman_proxy_lookup(interface, wp_context->status_url, wp_context->service, @@ -1046,6 +1055,8 @@ static int wispr_portal_detect(struct connman_wispr_portal_context *wp_context) wispr_portal_context_unref(wp_context); } } else if (wp_context->timeout == 0) { + wispr_portal_context_ref(wp_context); + wp_context->timeout = g_idle_add(no_proxy_callback, wp_context); }