]> git.ipfire.org Git - thirdparty/haproxy.git/commitdiff
BUG/MAJOR: quic: Possible crash with full congestion control window
authorFrédéric Lécaille <flecaille@haproxy.com>
Sun, 13 Mar 2022 18:19:12 +0000 (19:19 +0100)
committerWilly Tarreau <w@1wt.eu>
Tue, 15 Mar 2022 09:38:48 +0000 (10:38 +0100)
This commit reverts this one:
  "d5066dd9d BUG/MEDIUM: quic: qc_prep_app_pkts() retries on qc_build_pkt() failures"

After having filled the congestion control window, qc_build_pkt() always fails.
Then depending on the relative position of the writer and  reader indexes for the
TX buffer, this could lead this function to try to reuse the buffer even if not full.
In such case, we do not always mark the end of the data in this TX buffer. This
is something the reader cannot understand: it reads a false datagram length,
then a wrong packet address from the TX buffer, leading to an invalid pointer
dereferencing.

src/xprt_quic.c

index bfe888f3b2bf8e3b0ea8eaf71083611c6dde5e2f..34fc08423af488916aac66ebd314d33ec0e1eb6a 100644 (file)
@@ -2528,7 +2528,11 @@ static int qc_prep_app_pkts(struct quic_conn *qc, struct qring *qr,
                case -2:
                        goto err;
                case -1:
-                       goto stop_build;
+                       /* As we provide qc_build_pkt() with an enough big buffer to fulfill an
+                        * MTU, we are here because of the congestion control window. There is
+                        * no need to try to reuse this buffer.
+                        */
+                       goto out;
                default:
                        break;
                }
@@ -2542,7 +2546,6 @@ static int qc_prep_app_pkts(struct quic_conn *qc, struct qring *qr,
                qc_set_dg(cbuf, pkt->len, pkt);
        }
 
- stop_build:
        /* Reset <wr> writer index if in front of <rd> index */
        if (end_buf - pos < (int)qc->path->mtu + dg_headlen) {
                int rd = HA_ATOMIC_LOAD(&cbuf->rd);
@@ -5204,6 +5207,10 @@ static inline void free_quic_tx_packet(struct quic_tx_packet *pkt)
  *
  * Return -2 if the packet could not be allocated or encrypted for any reason,
  * -1 if there was not enough room to build a packet.
+ * XXX NOTE XXX
+ * If you provide provide qc_build_pkt() with a big enough buffer to build a packet as big as
+ * possible (to fill an MTU), the unique reason why this function may fail is the congestion
+ * control window limitation.
  */
 static struct quic_tx_packet *qc_build_pkt(unsigned char **pos,
                                            const unsigned char *buf_end,