From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Tue, 29 Mar 2022 09:51:17 +0000 (+0200)
Subject: BUG/MEDIUM: quic: do not use qcs from quic_stream on ACK parsing
X-Git-Tag: v2.6-dev5~90
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8d5def0babf5dcf3b2478d3a39ae039636452617;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: quic: do not use qcs from quic_stream on ACK parsing

The quic_stream frame stores the qcs instance. On ACK parsing, qcs is
accessed to clear the stream buffer. This can cause a segfault if the
MUX or the qcs is already released.

Consider the following scenario :

1. a STREAM frame is generated by the MUX
   transport layer emits the frame with PKN=1
   upper layer has finished the transfer so related qcs is detached

2. transport layer reemits the frame with PKN=2 because ACK was not
   received

3. ACK for PKN=1 is received, stream buffer is cleared
   at this stage, qcs may be freed by the MUX as it is detached

4. ACK for PKN=2 is received
   qcs for STREAM frame is dereferenced which will lead to a crash

To prevent this, qcs is never accessed from the quic_stream during ACK
parsing. Instead, a lookup is done on the MUX streams tree. If the MUX
is already released, no lookup is done. These checks prevents a possible
segfault.

This change may have an impact on the perf as now we are forced to use a
tree lookup operation. If this is the case, an alternative solution may
be to implement a refcount on qcs instances.
---

diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index d0beb500f0..c2003704ac 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -1455,8 +1455,30 @@ static inline void qc_treat_acked_tx_frm(struct quic_conn *qc,
 	switch (frm->type) {
 	case QUIC_FT_STREAM_8 ... QUIC_FT_STREAM_F:
 	{
-		struct qcs *qcs = frm->stream.qcs;
 		struct quic_stream *strm = &frm->stream;
+		struct eb64_node *node = NULL;
+		struct qcs *qcs = NULL;
+
+		/* do not use strm->qcs as the qcs instance might be freed at
+		 * this stage. Use the id to do a proper lookup.
+		 *
+		 * TODO if lookup operation impact on the perf is noticeable,
+		 * implement a refcount on qcs instances.
+		 */
+		if (qc->mux_state == QC_MUX_READY) {
+			node = eb64_lookup(&qc->qcc->streams_by_id, strm->id);
+			qcs = eb64_entry(node, struct qcs, by_id);
+		}
+
+		if (!qcs) {
+			TRACE_PROTO("acked stream for released stream", QUIC_EV_CONN_ACKSTRM, qc, strm);
+			LIST_DELETE(&frm->list);
+			quic_tx_packet_refdec(frm->pkt);
+			pool_free(pool_head_quic_frame, frm);
+
+			/* early return */
+			return;
+		}
 
 		TRACE_PROTO("acked stream", QUIC_EV_CONN_ACKSTRM, qc, strm, qcs);
 		if (strm->offset.key <= qcs->tx.ack_offset) {