| Message ID | 20240502233642.700289-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | b91f6788c4be0dd35b9f5edae14f372d68fced08 |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-trace: Fix access to an out-of-scope stack memory. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <amusil@redhat.com> > Signed-off-by: Ilya Maximets <i.maximets@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 <amorenoz@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 <amorenoz@redhat.com>
On 3 May 2024, at 1:36, 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 <amusil@redhat.com> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Thanks for the patch, and adding a test case. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 5/3/24 11:29, Eelco Chaudron wrote: > > > On 3 May 2024, at 1:36, 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 <amusil@redhat.com> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > Thanks for the patch, and adding a test case. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Thanks, Adrian and Eelco! Applied and backported down to 2.17. Best regards, Ilya Maximets.
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
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 <amusil@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@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(-)