From patchwork Thu May 2 23:36:37 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1930893 X-Patchwork-Delegate: i.maximets@samsung.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=patchwork.ozlabs.org) Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) (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 4VVr0s1Qw3z20fb for ; Fri, 3 May 2024 09:36:12 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 3B3B541E67; Thu, 2 May 2024 23:36:10 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id RWxDG0MDT7kf; Thu, 2 May 2024 23:36:08 +0000 (UTC) X-Comment: SPF check N/A for local connections - client-ip=140.211.9.56; helo=lists.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver= DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 7422741E4C Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id 7422741E4C; Thu, 2 May 2024 23:36:08 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 448F5C007C; Thu, 2 May 2024 23:36:08 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 43E90C0037 for ; Thu, 2 May 2024 23:36:06 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 1E2C740298 for ; Thu, 2 May 2024 23:36:06 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id C3pi31NJOc0j for ; Thu, 2 May 2024 23:36:05 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=2001:4b98:dc4:8::225; helo=relay5-d.mail.gandi.net; envelope-from=i.maximets@ovn.org; receiver= DMARC-Filter: OpenDMARC Filter v1.4.2 smtp2.osuosl.org 117AC4000B Authentication-Results: smtp2.osuosl.org; dmarc=none (p=none dis=none) header.from=ovn.org DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 117AC4000B Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::225]) by smtp2.osuosl.org (Postfix) with ESMTPS id 117AC4000B for ; Thu, 2 May 2024 23:36:03 +0000 (UTC) Received: by mail.gandi.net (Postfix) with ESMTPSA id DAE831C0005; Thu, 2 May 2024 23:36:00 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Fri, 3 May 2024 01:36:37 +0200 Message-ID: <20240502233642.700289-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.44.0 MIME-Version: 1.0 X-GND-Sasl: i.maximets@ovn.org Cc: Ilya Maximets Subject: [ovs-dev] [PATCH] ofproto-dpif-trace: Fix access to an out-of-scope stack memory. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" While tracing NAT actions, pointer to the action may be stored in the recirculation node for future reference. However, while translating actions for the group bucket in xlate_group_bucket, the action list is allocated temporarily on stack. So, in case the group translation leads to NAT, the stack pointer can be stored in the recirculation node and accessed later by the tracing mechanism when this stack memory is long gone: ==396230==ERROR: AddressSanitizer: stack-use-after-return on address 0x191844 at pc 0x64222a bp 0xa5da10 sp 0xa5da08 READ of size 1 at 0x191844 thread T0 0 0x642229 in ofproto_trace_recirc_node ofproto/ofproto-dpif-trace.c:704:49 1 0x642229 in ofproto_trace ofproto/ofproto-dpif-trace.c:867:9 2 0x6434c1 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:489:9 3 0xc1e491 in process_command lib/unixctl.c:310:13 4 0xc1e491 in run_connection lib/unixctl.c:344:17 5 0xc1e491 in unixctl_server_run lib/unixctl.c:395:21 6 0x53eedf in main ovs/vswitchd/ovs-vswitchd.c:131:9 7 0x2be087 in __libc_start_call_main 8 0x2be14a in __libc_start_main@GLIBC_2.2.5 9 0x42dee4 in _start (vswitchd/ovs-vswitchd+0x42dee4) Address 0x191844 is located in stack of thread T0 at offset 68 in frame 0 0x6d391f in xlate_group_bucket ofproto/ofproto-dpif-xlate.c:4751 This frame has 3 object(s): [32, 1056) 'action_list_stub' (line 4760) <== Memory access at offset 68 is inside this variable [1184, 1248) 'action_list' (line 4761) [1280, 1344) 'action_set' (line 4762) SUMMARY: AddressSanitizer: stack-use-after-return ofproto/ofproto-dpif-trace.c:704:49 in ofproto_trace_recirc_node Fix that by copying the action. Fixes: d072d2de011b ("ofproto-dpif-trace: Improve NAT tracing.") Reported-by: Ales Musil Signed-off-by: Ilya Maximets Reviewed-by: Adrian Moreno Reviewed-by: Adrian Moreno Acked-by: Eelco Chaudron --- ofproto/ofproto-dpif-trace.c | 3 ++- ofproto/ofproto-dpif-trace.h | 2 +- tests/ofproto-dpif.at | 22 ++++++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index 87506aa78..e43d9f88c 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -102,7 +102,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, node->flow = *flow; node->flow.recirc_id = recirc_id; node->flow.ct_zone = zone; - node->nat_act = ofn; + node->nat_act = ofn ? xmemdup(ofn, sizeof *ofn) : NULL; node->packet = packet ? dp_packet_clone(packet) : NULL; return true; @@ -113,6 +113,7 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) { if (node) { recirc_free_id(node->recirc_id); + free(node->nat_act); dp_packet_delete(node->packet); free(node); } diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h index f579a5ca4..f023b10cd 100644 --- a/ofproto/ofproto-dpif-trace.h +++ b/ofproto/ofproto-dpif-trace.h @@ -73,7 +73,7 @@ struct oftrace_recirc_node { uint32_t recirc_id; struct flow flow; struct dp_packet *packet; - const struct ofpact_nat *nat_act; + struct ofpact_nat *nat_act; }; /* A node within a next_ct_states list. */ diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 3eaccb13a..0b23fd6c5 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -947,6 +947,28 @@ AT_CHECK([tail -1 stdout], [0], OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - group with ct and dnat recirculation in action list]) +OVS_VSWITCHD_START +add_of_ports br0 1 10 +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 \ + 'group_id=1234,type=all,bucket=ct(nat(dst=10.10.10.7:80),commit,table=2)']) +AT_DATA([flows.txt], [dnl +table=0 ip,ct_state=-trk actions=group:1234 +table=2 ip,ct_state=+trk actions=output:10 +]) +AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt]) +AT_CHECK([ovs-appctl ofproto/trace br0 ' + in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800, + nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,nw_frag=no, + icmp_type=8,icmp_code=0 +'], [0], [stdout]) +AT_CHECK([grep 'Datapath actions' stdout], [0], [dnl +Datapath actions: ct(commit,nat(dst=10.10.10.7:80)),recirc(0x1) +Datapath actions: 10 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - group actions have no effect afterwards]) OVS_VSWITCHD_START add_of_ports br0 1 10