On 5/3/24 1:36 AM, Ilya Maximets wrote:
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 <amu...@redhat.com>
Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
---
ofproto/ofproto-dpif-trace.c | 3 ++-
ofproto/ofproto-dpif-trace.h | 2 +-
tests/ofproto-dpif.at | 22 ++++++++++++++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
Reviewed-by: Adrian Moreno <amore...@redhat.com>
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
Reviewed-by: Adrian Moreno <amore...@redhat.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
