| Message ID | 20220128131924.8020-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] pinctrl: Fix potential stack overflow in pinctrl_compose_ipv6(). | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Acked-by: Mark Michelson <mmichels@redhat.com> On 1/28/22 08:19, Dumitru Ceara wrote: > packet_set_ipv6() calls packet_rh_present() which expects L3 and L4 > offsets to be set in the packet. If not, it may incorrectly iterate > outside the bounds of the packet. > > We now call dp_packet_set_l4() before packet_set_ipv6() in > pinctrl_compose_ipv6(). This also means that now packet_set_ipv6() will > update the L4 checksums so we need to make sure we zero them out when > recomputing them, e.g., in pinctrl_handle_tcp_reset(). > > Reported by AddressSanitizer: > ==3919971==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fd9eecfc7b4 at pc 0x00000065d7a7 bp 0x7fd9eecfc580 sp 0x7fd9eecfc578 > READ of size 1 at 0x7fd9eecfc7b4 thread T1 (ovn_pinctrl0) > #0 0x65d7a6 in packet_rh_present lib/packets.c:1220 > #1 0x65d7a6 in packet_set_ipv6 lib/packets.c:1339 > #2 0x450eeb in pinctrl_compose_ipv6 controller/pinctrl.c:4577 > #3 0x4540f3 in ip_mcast_querier_send_mld controller/pinctrl.c:5408 > #4 0x478de5 in ip_mcast_querier_send controller/pinctrl.c:5463 > #5 0x478de5 in ip_mcast_querier_run controller/pinctrl.c:5486 > #6 0x478de5 in pinctrl_handler controller/pinctrl.c:3400 > #7 0x62f183 in ovsthread_wrapper lib/ovs-thread.c:422 > #8 0x7fd9f26cd298 in start_thread /usr/src/debug/glibc-2.33-20.fc34.x86_64/nptl/pthread_create.c:481 > #9 0x7fd9f24a4352 in clone (/lib64/libc.so.6+0x100352) > > Address 0x7fd9eecfc7b4 is located in stack of thread T1 (ovn_pinctrl0) at offset 292 in frame > #0 0x453eff in ip_mcast_querier_send_mld controller/pinctrl.c:5402 > > This frame has 6 object(s): > [32, 48) 'unspecified' (line 5422) > [64, 128) 'ofpacts' (line 5427) > [160, 288) 'packet_stub' (line 5404) <== Memory access at offset 292 overflows this variable > > Fixes: 6e475f9d03d9 ("pinctrl: Ensure proper alignment when using pinctrl_compose_ipv*().") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/pinctrl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index fe1358990..c61850e9f 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1779,6 +1779,7 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow, > > struct tcp_header *th = dp_packet_l4(&packet); > > + th->tcp_csum = 0; > th->tcp_dst = ip_flow->tp_src; > th->tcp_src = ip_flow->tp_dst; > > @@ -4575,8 +4576,8 @@ pinctrl_compose_ipv6(struct dp_packet *packet, struct eth_addr eth_src, > nh->ip6_nxt = ip_proto; > nh->ip6_plen = htons(ip_payload_len); > > - packet_set_ipv6(packet, ipv6_src, ipv6_dst, 0, 0, ttl); > dp_packet_set_l4(packet, nh + 1); > + packet_set_ipv6(packet, ipv6_src, ipv6_dst, 0, 0, ttl); > } > > /* >
On Tue, Feb 1, 2022 at 11:27 AM Mark Michelson <mmichels@redhat.com> wrote: > > Acked-by: Mark Michelson <mmichels@redhat.com> Thanks. I applied this patch to the main branch and branch-21.12. Do let me know if it needs to be backported further down. Numan > > On 1/28/22 08:19, Dumitru Ceara wrote: > > packet_set_ipv6() calls packet_rh_present() which expects L3 and L4 > > offsets to be set in the packet. If not, it may incorrectly iterate > > outside the bounds of the packet. > > > > We now call dp_packet_set_l4() before packet_set_ipv6() in > > pinctrl_compose_ipv6(). This also means that now packet_set_ipv6() will > > update the L4 checksums so we need to make sure we zero them out when > > recomputing them, e.g., in pinctrl_handle_tcp_reset(). > > > > Reported by AddressSanitizer: > > ==3919971==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fd9eecfc7b4 at pc 0x00000065d7a7 bp 0x7fd9eecfc580 sp 0x7fd9eecfc578 > > READ of size 1 at 0x7fd9eecfc7b4 thread T1 (ovn_pinctrl0) > > #0 0x65d7a6 in packet_rh_present lib/packets.c:1220 > > #1 0x65d7a6 in packet_set_ipv6 lib/packets.c:1339 > > #2 0x450eeb in pinctrl_compose_ipv6 controller/pinctrl.c:4577 > > #3 0x4540f3 in ip_mcast_querier_send_mld controller/pinctrl.c:5408 > > #4 0x478de5 in ip_mcast_querier_send controller/pinctrl.c:5463 > > #5 0x478de5 in ip_mcast_querier_run controller/pinctrl.c:5486 > > #6 0x478de5 in pinctrl_handler controller/pinctrl.c:3400 > > #7 0x62f183 in ovsthread_wrapper lib/ovs-thread.c:422 > > #8 0x7fd9f26cd298 in start_thread /usr/src/debug/glibc-2.33-20.fc34.x86_64/nptl/pthread_create.c:481 > > #9 0x7fd9f24a4352 in clone (/lib64/libc.so.6+0x100352) > > > > Address 0x7fd9eecfc7b4 is located in stack of thread T1 (ovn_pinctrl0) at offset 292 in frame > > #0 0x453eff in ip_mcast_querier_send_mld controller/pinctrl.c:5402 > > > > This frame has 6 object(s): > > [32, 48) 'unspecified' (line 5422) > > [64, 128) 'ofpacts' (line 5427) > > [160, 288) 'packet_stub' (line 5404) <== Memory access at offset 292 overflows this variable > > > > Fixes: 6e475f9d03d9 ("pinctrl: Ensure proper alignment when using pinctrl_compose_ipv*().") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > controller/pinctrl.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index fe1358990..c61850e9f 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -1779,6 +1779,7 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow, > > > > struct tcp_header *th = dp_packet_l4(&packet); > > > > + th->tcp_csum = 0; > > th->tcp_dst = ip_flow->tp_src; > > th->tcp_src = ip_flow->tp_dst; > > > > @@ -4575,8 +4576,8 @@ pinctrl_compose_ipv6(struct dp_packet *packet, struct eth_addr eth_src, > > nh->ip6_nxt = ip_proto; > > nh->ip6_plen = htons(ip_payload_len); > > > > - packet_set_ipv6(packet, ipv6_src, ipv6_dst, 0, 0, ttl); > > dp_packet_set_l4(packet, nh + 1); > > + packet_set_ipv6(packet, ipv6_src, ipv6_dst, 0, 0, ttl); > > } > > > > /* > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2/2/22 02:13, Numan Siddique wrote: > On Tue, Feb 1, 2022 at 11:27 AM Mark Michelson <mmichels@redhat.com> wrote: >> >> Acked-by: Mark Michelson <mmichels@redhat.com> > > Thanks. I applied this patch to the main branch and branch-21.12. Thanks! > > Do let me know if it needs to be backported further down. It's not needed, older branches don't have the issue. Regards, Dumitru
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index fe1358990..c61850e9f 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -1779,6 +1779,7 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow, struct tcp_header *th = dp_packet_l4(&packet); + th->tcp_csum = 0; th->tcp_dst = ip_flow->tp_src; th->tcp_src = ip_flow->tp_dst; @@ -4575,8 +4576,8 @@ pinctrl_compose_ipv6(struct dp_packet *packet, struct eth_addr eth_src, nh->ip6_nxt = ip_proto; nh->ip6_plen = htons(ip_payload_len); - packet_set_ipv6(packet, ipv6_src, ipv6_dst, 0, 0, ttl); dp_packet_set_l4(packet, nh + 1); + packet_set_ipv6(packet, ipv6_src, ipv6_dst, 0, 0, ttl); } /*
packet_set_ipv6() calls packet_rh_present() which expects L3 and L4 offsets to be set in the packet. If not, it may incorrectly iterate outside the bounds of the packet. We now call dp_packet_set_l4() before packet_set_ipv6() in pinctrl_compose_ipv6(). This also means that now packet_set_ipv6() will update the L4 checksums so we need to make sure we zero them out when recomputing them, e.g., in pinctrl_handle_tcp_reset(). Reported by AddressSanitizer: ==3919971==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fd9eecfc7b4 at pc 0x00000065d7a7 bp 0x7fd9eecfc580 sp 0x7fd9eecfc578 READ of size 1 at 0x7fd9eecfc7b4 thread T1 (ovn_pinctrl0) #0 0x65d7a6 in packet_rh_present lib/packets.c:1220 #1 0x65d7a6 in packet_set_ipv6 lib/packets.c:1339 #2 0x450eeb in pinctrl_compose_ipv6 controller/pinctrl.c:4577 #3 0x4540f3 in ip_mcast_querier_send_mld controller/pinctrl.c:5408 #4 0x478de5 in ip_mcast_querier_send controller/pinctrl.c:5463 #5 0x478de5 in ip_mcast_querier_run controller/pinctrl.c:5486 #6 0x478de5 in pinctrl_handler controller/pinctrl.c:3400 #7 0x62f183 in ovsthread_wrapper lib/ovs-thread.c:422 #8 0x7fd9f26cd298 in start_thread /usr/src/debug/glibc-2.33-20.fc34.x86_64/nptl/pthread_create.c:481 #9 0x7fd9f24a4352 in clone (/lib64/libc.so.6+0x100352) Address 0x7fd9eecfc7b4 is located in stack of thread T1 (ovn_pinctrl0) at offset 292 in frame #0 0x453eff in ip_mcast_querier_send_mld controller/pinctrl.c:5402 This frame has 6 object(s): [32, 48) 'unspecified' (line 5422) [64, 128) 'ofpacts' (line 5427) [160, 288) 'packet_stub' (line 5404) <== Memory access at offset 292 overflows this variable Fixes: 6e475f9d03d9 ("pinctrl: Ensure proper alignment when using pinctrl_compose_ipv*().") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/pinctrl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)