From fe97c6b86e4749decae25919202f0d20ee00b051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= Date: Tue, 21 Nov 2023 20:31:32 +0100 Subject: [PATCH] BUG/MEDIUM: quic: Possible crash during retransmissions and heavy load This bug could be reproduced with -dMfail and dectected by libasan as follows: $ ASAN_OPTIONS=disable_coredump=0:unmap_shadow_on_exit=1:abort_on_error=f quic-freeze.cfg -dMfail -dMno-cache -dM0x55 ================================================================= ==82989==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffc 0x560790cc4749 bp 0x7fff8e0e8e30 sp 0x7fff8e0e8e28 WRITE of size 8 at 0x7fff8e0ea338 thread T0 #0 0x560790cc4748 in qc_frm_free src/quic_frame.c:1222 #1 0x560790cc5260 in qc_release_frm src/quic_frame.c:1261 #2 0x560790d1de99 in qc_treat_acked_tx_frm src/quic_rx.c:312 #3 0x560790d1e708 in qc_ackrng_pkts src/quic_rx.c:370 #4 0x560790d22a1d in qc_parse_ack_frm src/quic_rx.c:694 #5 0x560790d25daa in qc_parse_pkt_frms src/quic_rx.c:988 #6 0x560790d2a509 in qc_treat_rx_pkts src/quic_rx.c:1373 #7 0x560790c72d45 in quic_conn_io_cb src/quic_conn.c:906 #8 0x560791207847 in run_tasks_from_lists src/task.c:596 #9 0x5607912095f0 in process_runnable_tasks src/task.c:876 #10 0x560791135564 in run_poll_loop src/haproxy.c:2966 #11 0x5607911363af in run_thread_poll_loop src/haproxy.c:3165 #12 0x56079113938c in main src/haproxy.c:3862 #13 0x7f92606edd09 in __libc_start_main ../csu/libc-start.c:308 #14 0x560790bcd529 in _start (/home/flecaille/src/haproxy/haproxy+0x Address 0x7fff8e0ea338 is located in stack of thread T0 at offset 1032 i #0 0x560790d29b52 in qc_treat_rx_pkts src/quic_rx.c:1341 This frame has 2 object(s): [32, 48) 'ar' (line 1380) [64, 1088) '_msg' (line 1368) <== Memory access at offset 1032 is inable HINT: this may be a false positive if your program uses some custom stacnism, swapcontext or vfork (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-use-after-scope src/quic_frame.c:1222 i Shadow bytes around the buggy address: 0x100071c15410: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 0x100071c15420: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 0x100071c15430: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 0x100071c15440: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 0x100071c15450: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 =>0x100071c15460: f8 f8 f8 f8 f8 f8 f8[f8]f8 f8 f8 f8 f8 f8 f3 f3 0x100071c15470: f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 00 00 0x100071c15480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100071c15490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100071c154a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100071c154b0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 04 f3 f3 f3 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==82989==ABORTING AddressSanitizer:DEADLYSIGNAL AddressSanitizer:DEADLYSIGNAL AddressSanitizer:DEADLYSIGNAL AddressSanitizer:DEADLYSIGNAL AddressSanitizer:DEADLYSIGNAL AddressSanitizer:DEADLYSIGNAL Aborted (core dumped) Note that a coredump could not always be produced with all compilers. This was always the case with clang 11. When allocating frames to be retransmitted from qc_dgrams_retransmit(), if they could not be sent for any reason, they could remain attached to a local list to qc_dgrams_retransmit() and trigger a crash with libasan when releasing the original frames they were duplicated from. To fix this, always release the frames which could not be sent during retransmissions calling qc_free_frm_list() where needed. Must be backported as far as 2.6. (cherry picked from commit dc8a20b3170206932130c5779631a6594a277ab4) [cf: Adapted with Fred's help] Signed-off-by: Christopher Faulet --- src/quic_conn.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/quic_conn.c b/src/quic_conn.c index 9bacf6f21d59..78e7c13d6b19 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -2708,7 +2708,7 @@ static void qc_dup_pkt_frms(struct quic_conn *qc, TRACE_DEVEL("built probing frame", QUIC_EV_CONN_PRSAFRM, qc, origin); if (origin->pkt) { TRACE_DEVEL("duplicated from packet", QUIC_EV_CONN_PRSAFRM, - qc, NULL, &origin->pkt->pn_node.key); + qc, dup_frm, &origin->pkt->pn_node.key); } else { /* is a frame which was sent from a packet detected as lost. */ @@ -4962,6 +4962,7 @@ int qc_send_hdshk_pkts(struct quic_conn *qc, int old_data, static int qc_dgrams_retransmit(struct quic_conn *qc) { int ret = 0; + int sret; struct quic_enc_level *iqel = &qc->els[QUIC_TLS_ENC_LEVEL_INITIAL]; struct quic_enc_level *hqel = &qc->els[QUIC_TLS_ENC_LEVEL_HANDSHAKE]; struct quic_enc_level *aqel = &qc->els[QUIC_TLS_ENC_LEVEL_APP]; @@ -4982,18 +4983,20 @@ static int qc_dgrams_retransmit(struct quic_conn *qc) iqel->pktns->tx.pto_probe = 1; if (!LIST_ISEMPTY(&hfrms)) hqel->pktns->tx.pto_probe = 1; - if (!qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms, - QUIC_TLS_ENC_LEVEL_HANDSHAKE, &hfrms)) + sret = qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms, + QUIC_TLS_ENC_LEVEL_HANDSHAKE, &hfrms); + qc_free_frm_list(&ifrms); + qc_free_frm_list(&hfrms); + if (!sret) goto leave; - /* Put back unsent frames in their packet number spaces */ - LIST_SPLICE(&iqel->pktns->tx.frms, &ifrms); - LIST_SPLICE(&hqel->pktns->tx.frms, &hfrms); } else { if (!(qc->flags & QUIC_FL_CONN_ANTI_AMPLIFICATION_REACHED)) { iqel->pktns->tx.pto_probe = 1; - if (!qc_send_hdshk_pkts(qc, 0, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms, - QUIC_TLS_ENC_LEVEL_NONE, NULL)) + sret = qc_send_hdshk_pkts(qc, 0, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms, + QUIC_TLS_ENC_LEVEL_NONE, NULL); + qc_free_frm_list(&hfrms); + if (!sret) goto leave; } } @@ -5015,12 +5018,11 @@ static int qc_dgrams_retransmit(struct quic_conn *qc) TRACE_DEVEL("Avail. ack eliciting frames", QUIC_EV_CONN_FRMLIST, qc, &frms1); if (!LIST_ISEMPTY(&frms1)) { hqel->pktns->tx.pto_probe = 1; - if (!qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_HANDSHAKE, &frms1, - QUIC_TLS_ENC_LEVEL_NONE, NULL)) + sret = qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_HANDSHAKE, &frms1, + QUIC_TLS_ENC_LEVEL_NONE, NULL); + qc_free_frm_list(&frms1); + if (!sret) goto leave; - - /* Put back unsent frames into their packet number spaces */ - LIST_SPLICE(&hqel->pktns->tx.frms, &frms1); } } TRACE_STATE("no more need to probe Handshake packet number space", @@ -5037,23 +5039,19 @@ static int qc_dgrams_retransmit(struct quic_conn *qc) TRACE_PROTO("Avail. ack eliciting frames", QUIC_EV_CONN_FRMLIST, qc, &frms2); if (!LIST_ISEMPTY(&frms1)) { aqel->pktns->tx.pto_probe = 1; - if (!qc_send_app_probing(qc, &frms1)) { - qc_free_frm_list(&frms1); + sret = qc_send_app_probing(qc, &frms1); + qc_free_frm_list(&frms1); + if (!sret) { qc_free_frm_list(&frms2); goto leave; } - - /* Put back unsent frames into their packet number spaces */ - LIST_SPLICE(&aqel->pktns->tx.frms, &frms1); } if (!LIST_ISEMPTY(&frms2)) { aqel->pktns->tx.pto_probe = 1; - if (!qc_send_app_probing(qc, &frms2)) { - qc_free_frm_list(&frms2); + sret = qc_send_app_probing(qc, &frms2); + qc_free_frm_list(&frms2); + if (!sret) goto leave; - } - /* Put back unsent frames into their packet number spaces */ - LIST_SPLICE(&aqel->pktns->tx.frms, &frms2); } TRACE_STATE("no more need to probe 01RTT packet number space", QUIC_EV_CONN_TXPKT, qc);