[Openvpn-devel,v5] Change buffer allocation calculation and checks to be more static

Message ID 20220124025459.1042317-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v5] Change buffer allocation calculation and checks to be more static
Related show

Commit Message

Arne Schwabe Jan. 24, 2022, 2:54 a.m.
Currently we use half dynamic buffer sizes where we use have a fixed
overhead for crypto (crypto_max_overhead) but use a dynamic overhead
for the the other small header sizes.

Patch v3: rebase
Patch v4: add size of ack array to control channel frame size
Patch v5: fix calculation of compression overhead calculated over 0 instead
          of payload size

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/comp-lz4.c   |   4 +-
 src/openvpn/crypto.c     |   4 +-
 src/openvpn/forward.c    |   8 +--
 src/openvpn/init.c       | 110 +++++++++++++++++++++++++++++++++++----
 src/openvpn/lzo.c        |   2 +-
 src/openvpn/mtu.c        |   7 ++-
 src/openvpn/mtu.h        |  74 +++++++++++++-------------
 src/openvpn/multi.c      |   4 +-
 src/openvpn/multi.h      |   2 +-
 src/openvpn/occ.c        |   4 +-
 src/openvpn/options.c    |   2 +-
 src/openvpn/ping.c       |   2 +-
 src/openvpn/reliable.c   |   2 -
 src/openvpn/reliable.h   |   3 ++
 src/openvpn/ssl.c        |  38 +++++++++++---
 src/openvpn/ssl_common.h |   3 +-
 16 files changed, 196 insertions(+), 73 deletions(-)

Comments

Gert Doering Feb. 2, 2022, 12:09 p.m. | #1
Hi,

On Mon, Jan 24, 2022 at 03:54:59AM +0100, Arne Schwabe wrote:
> Currently we use half dynamic buffer sizes where we use have a fixed
> overhead for crypto (crypto_max_overhead) but use a dynamic overhead
> for the the other small header sizes.
> 
> Patch v3: rebase
> Patch v4: add size of ack array to control channel frame size
> Patch v5: fix calculation of compression overhead calculated over 0 instead
>           of payload size

Generally this looks okayish, and *most* t_client / t_server tests work
beautifully.

It does break --tls-client --proto tcp for me, for big packets, though...

The client is called like this:

openvpn --ca ... --cert ... --key ... --comp-lzo --verb 3 --tls-client --dev tap --proto tcp-client --remote gentoo.ov.greenie.net 51204 --ifconfig 10.204.9.2 255.255.255.0 --comp-lzo --tun-ipv6 --ifconfig-ipv6 fd00:abcd:204:9::2/64 fd00:abcd:204:9::1 --route 10.204.0.0 255.255.0.0 10.204.9.1 --route-ipv6 fd00:abcd:204::/48 --data-ciphers BF-CBC

and will do

2022-02-02 12:56:52 peer info: IV_CIPHERS=BF-CBC:AES-256-GCM:AES-128-GCM
2022-02-02 12:56:52 peer info: IV_PROTO=42
2022-02-02 12:56:52 WARNING: 'tun-mtu' is used inconsistently, local='tun-mtu 1500', remote='tun-mtu 1532'
2022-02-02 12:56:52 P2P mode NCP negotiation result: TLS_export=1, DATA_v2=1, peer-id 897556, cipher=BF-CBC
2022-02-02 12:56:52 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 2048 bit RSA, signature: RSA-SHA1
2022-02-02 12:56:52 [server] Peer Connection Initiated with [AF_INET6]2001:608:0:814::f000:11:51204
2022-02-02 12:56:53 OPTIONS IMPORT: adjusting link_mtu to 1579
2022-02-02 12:56:53 Outgoing Data Channel: Cipher 'BF-CBC' initialized with 128 bit key
2022-02-02 12:56:53 Outgoing Data Channel: Using 160 bit message hash 'SHA1' for HMAC authentication


when sending 1440 byte pings (t_client test) it will complain

2022-02-02 12:56:15 TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:11:51204 (tried=1520,max=1499)
2022-02-02 12:56:15 TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:11:51204 (tried=1520,max=1499)
2022-02-02 12:56:15 TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:11:51204 (tried=1520,max=1499)


soo...  is this something that "should be fixed" by a later patch in the
series, or do we need a v6 of this one?


The same test works correctly with master as of right now (5b3c8ca86976).

gert
Gert Doering Feb. 2, 2022, 1:20 p.m. | #2
Acked-by: Gert Doering <gert@greenie.muc.de>

Tested on client and server side, all "regular" stuff works, but
it breaks my TCP/P2P/P2P-NCP test case for big packets, namely...

2022-02-02 12:57:19 TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:11:51204 (tried=1520,max=1499)

(expected result: oversized packet, fragment/segment on outer layer)

After discussion on IRC, decided to go forward anyway, as this is only
one specific corner case (P2P NCP), and it gets fixed for good in 13/14 
- removing EXPANDED_SIZE() in forward.c, replacing it with just 
"buf.payload_size", which always has "sufficient headroom" - tested,
fixes this corner case, doesn't break anything else.


(For reference, this was 10/21 in v1 of the patchset, 03/14 in v3, and
standalone patches v4+v5 - but neither of the previous ones got an ACK
or NAK, just MaxF finding ways to crash v3 and v4 ;-) )

Your patch has been applied to the master branch.

commit 65a21eb14f4afd80864e88ff425f5d9ef8d8fdec
Author: Arne Schwabe
Date:   Mon Jan 24 03:54:59 2022 +0100

     Change buffer allocation calculation and checks to be more static

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220124025459.1042317-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23638.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Feb. 2, 2022, 1:21 p.m. | #3
Acked-by: Gert Doering <gert@greenie.muc.de>

Tested on client and server side, all "regular" stuff works, but
it breaks my TCP/P2P/P2P-NCP test case for big packets, namely...

2022-02-02 12:57:19 TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:11:51204 (tried=1520,max=1499)

(expected result: oversized packet, fragment/segment on outer layer)

After discussion on IRC, decided to go forward anyway, as this is only
one specific corner case (P2P NCP), and it gets fixed for good in 13/14 
- removing EXPANDED_SIZE() in forward.c, replacing it with just 
"buf.payload_size", which always has "sufficient headroom" - tested,
fixes this corner case, doesn't break anything else.


(For reference, this was 10/21 in v1 of the patchset, 03/14 in v3, and
standalone patches v4+v5 - but neither of the previous ones got an ACK
or NAK, just MaxF finding ways to crash v3 and v4 ;-) )

Your patch has been applied to the master branch.

commit 65a21eb14f4afd80864e88ff425f5d9ef8d8fdec
Author: Arne Schwabe
Date:   Mon Jan 24 03:54:59 2022 +0100

     Change buffer allocation calculation and checks to be more static

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220124025459.1042317-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23638.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/comp-lz4.c b/src/openvpn/comp-lz4.c
index bceca5e2..aa83ea80 100644
--- a/src/openvpn/comp-lz4.c
+++ b/src/openvpn/comp-lz4.c
@@ -213,7 +213,7 @@  lz4_decompress(struct buffer *buf, struct buffer work,
                struct compress_context *compctx,
                const struct frame *frame)
 {
-    size_t zlen_max = EXPANDED_SIZE(frame);
+    size_t zlen_max = frame->buf.payload_size;
     uint8_t c;          /* flag indicating whether or not our peer compressed */
 
     if (buf->len <= 0)
@@ -250,7 +250,7 @@  lz4v2_decompress(struct buffer *buf, struct buffer work,
                  struct compress_context *compctx,
                  const struct frame *frame)
 {
-    size_t zlen_max = EXPANDED_SIZE(frame);
+    size_t zlen_max = frame->buf.payload_size;
     uint8_t c;          /* flag indicating whether or not our peer compressed */
 
     if (buf->len <= 0)
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 0da71a32..40b15ad7 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1065,7 +1065,7 @@  test_crypto(struct crypto_options *co, struct frame *frame)
 {
     int i, j;
     struct gc_arena gc = gc_new();
-    struct buffer src = alloc_buf_gc(TUN_MTU_SIZE(frame), &gc);
+    struct buffer src = alloc_buf_gc(frame->buf.payload_size, &gc);
     struct buffer work = alloc_buf_gc(BUF_SIZE(frame), &gc);
     struct buffer encrypt_workspace = alloc_buf_gc(BUF_SIZE(frame), &gc);
     struct buffer decrypt_workspace = alloc_buf_gc(BUF_SIZE(frame), &gc);
@@ -1096,7 +1096,7 @@  test_crypto(struct crypto_options *co, struct frame *frame)
     }
 
     msg(M_INFO, "Entering " PACKAGE_NAME " crypto self-test mode.");
-    for (i = 1; i <= TUN_MTU_SIZE(frame); ++i)
+    for (i = 1; i <= frame->buf.payload_size; ++i)
     {
         update_time();
 
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index a905f993..d50766e3 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1120,8 +1120,8 @@  read_incoming_tun(struct context *c)
     }
 #else  /* ifdef _WIN32 */
     ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
-    ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
-    c->c2.buf.len = read_tun(c->c1.tuntap, BPTR(&c->c2.buf), MAX_RW_SIZE_TUN(&c->c2.frame));
+    ASSERT(buf_safe(&c->c2.buf, c->c2.frame.buf.payload_size));
+    c->c2.buf.len = read_tun(c->c1.tuntap, BPTR(&c->c2.buf), c->c2.frame.buf.payload_size);
 #endif /* ifdef _WIN32 */
 
 #ifdef PACKET_TRUNCATION_CHECK
@@ -1710,7 +1710,7 @@  process_outgoing_tun(struct context *c)
                       PIP_MSSFIX | PIPV4_EXTRACT_DHCP_ROUTER | PIPV4_CLIENT_NAT | PIP_OUTGOING,
                       &c->c2.to_tun);
 
-    if (c->c2.to_tun.len <= MAX_RW_SIZE_TUN(&c->c2.frame))
+    if (c->c2.to_tun.len <= c->c2.frame.buf.payload_size)
     {
         /*
          * Write to TUN/TAP device.
@@ -1770,7 +1770,7 @@  process_outgoing_tun(struct context *c)
          */
         msg(D_LINK_ERRORS, "tun packet too large on write (tried=%d,max=%d)",
             c->c2.to_tun.len,
-            MAX_RW_SIZE_TUN(&c->c2.frame));
+            c->c2.frame.buf.payload_size);
     }
 
     buf_reset(&c->c2.to_tun);
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 4e37b885..1ddc0d3c 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -740,7 +740,7 @@  init_port_share(struct context *c)
     {
         port_share = port_share_open(c->options.port_share_host,
                                      c->options.port_share_port,
-                                     MAX_RW_SIZE_LINK(&c->c2.frame),
+                                     c->c2.frame.buf.payload_size,
                                      c->options.port_share_journal_dir);
         if (port_share == NULL)
         {
@@ -2441,6 +2441,35 @@  do_startup_pause(struct context *c)
     }
 }
 
+static size_t
+get_frame_mtu(struct context *c, const struct options *o)
+{
+    size_t mtu;
+
+    if (o->ce.link_mtu_defined)
+    {
+        ASSERT(o->ce.link_mtu_defined);
+        /* if we have a link mtu defined we calculate what the old code
+         * would have come up with as tun-mtu */
+        size_t overhead = frame_calculate_protocol_header_size(&c->c1.ks.key_type,
+                                                               o, true);
+        mtu = o->ce.link_mtu - overhead;
+
+    }
+    else
+    {
+        ASSERT(o->ce.tun_mtu_defined);
+        mtu = o->ce.tun_mtu;
+    }
+
+    if (mtu < TUN_MTU_MIN)
+    {
+        msg(M_WARN, "TUN MTU value (%lu) must be at least %d", mtu, TUN_MTU_MIN);
+        frame_print(&c->c2.frame, M_FATAL, "MTU is too small");
+    }
+    return mtu;
+}
+
 /*
  * Finalize MTU parameters based on command line or config file options.
  */
@@ -2452,12 +2481,71 @@  frame_finalize_options(struct context *c, const struct options *o)
         o = &c->options;
     }
 
-    frame_add_to_extra_buffer(&c->c2.frame, PAYLOAD_ALIGN);
-    frame_finalize(&c->c2.frame,
+    struct frame *frame = &c->c2.frame;
+
+    frame->tun_mtu = get_frame_mtu(c, o);
+
+    /* We always allow at least 1500 MTU packets to be received in our buffer
+     * space */
+    size_t payload_size = max_int(1500, frame->tun_mtu);
+
+    /* The extra tun needs to be added to the payload size */
+    if (o->ce.tun_mtu_defined)
+    {
+        payload_size += o->ce.tun_mtu_extra;
+    }
+
+    /* Add 100 byte of extra space in the buffer to account for slightly
+     * mismatched MUTs between peers */
+    payload_size += 100;
+
+
+    /* the space that is reserved before the payload to add extra headers to it
+    * we always reserve the space for the worst case */
+    size_t headroom = 0;
+
+    /* includes IV and packet ID */
+    headroom += crypto_max_overhead();
+
+    /* peer id + opcode */
+    headroom += 4;
+
+    /* socks proxy header */
+    headroom += 10;
+
+    /* compression header and fragment header (part of the encrypted payload) */
+    headroom += 1 + 1;
+
+    /* Round up headroom to the next multiple of 4 to ensure alignment */
+    headroom = (headroom + 3) & ~3;
+
+    /* Add the headroom to the payloadsize as a received (IP) packet can have
+     * all the extra headers in it */
+    payload_size += headroom;
+
+    /* the space after the payload, this needs some extra buffer space for
+     * encryption so headroom is probably too much but we do not really care
+     * the few extra bytes */
+    size_t tailroom = headroom;
+
+#ifdef USE_COMP
+    msg(D_MTU_DEBUG, "MTU: adding %lu buffer tailroom for compression for %lu "
+                     "bytes of payload",
+                     COMP_EXTRA_BUFFER(payload_size), payload_size);
+    tailroom += COMP_EXTRA_BUFFER(payload_size);
+#endif
+
+    frame->buf.payload_size = payload_size;
+    frame->buf.headroom = headroom;
+    frame->buf.tailroom = tailroom;
+
+    /* Kept to still update/calculate the other fields for now */
+    frame_finalize(frame,
                    o->ce.link_mtu_defined,
                    o->ce.link_mtu,
                    o->ce.tun_mtu_defined,
                    o->ce.tun_mtu);
+
 }
 
 /*
@@ -3224,17 +3312,19 @@  init_context_buffers(const struct frame *frame)
 
     ALLOC_OBJ_CLEAR(b, struct context_buffers);
 
-    b->read_link_buf = alloc_buf(BUF_SIZE(frame));
-    b->read_tun_buf = alloc_buf(BUF_SIZE(frame));
+    size_t buf_size = BUF_SIZE(frame);
+
+    b->read_link_buf = alloc_buf(buf_size);
+    b->read_tun_buf = alloc_buf(buf_size);
 
-    b->aux_buf = alloc_buf(BUF_SIZE(frame));
+    b->aux_buf = alloc_buf(buf_size);
 
-    b->encrypt_buf = alloc_buf(BUF_SIZE(frame));
-    b->decrypt_buf = alloc_buf(BUF_SIZE(frame));
+    b->encrypt_buf = alloc_buf(buf_size);
+    b->decrypt_buf = alloc_buf(buf_size);
 
 #ifdef USE_COMP
-    b->compress_buf = alloc_buf(BUF_SIZE(frame));
-    b->decompress_buf = alloc_buf(BUF_SIZE(frame));
+    b->compress_buf = alloc_buf(buf_size);
+    b->decompress_buf = alloc_buf(buf_size);
 #endif
 
     return b;
diff --git a/src/openvpn/lzo.c b/src/openvpn/lzo.c
index 8d572684..e7e89655 100644
--- a/src/openvpn/lzo.c
+++ b/src/openvpn/lzo.c
@@ -213,7 +213,7 @@  lzo_decompress(struct buffer *buf, struct buffer work,
                struct compress_context *compctx,
                const struct frame *frame)
 {
-    lzo_uint zlen = EXPANDED_SIZE(frame);
+    lzo_uint zlen = frame->buf.payload_size;
     int err;
     uint8_t c;          /* flag indicating whether or not our peer compressed */
 
diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c
index cc7c95e4..1648c8fe 100644
--- a/src/openvpn/mtu.c
+++ b/src/openvpn/mtu.c
@@ -48,7 +48,7 @@  alloc_buf_sock_tun(struct buffer *buf,
     /* allocate buffer for overlapped I/O */
     *buf = alloc_buf(BUF_SIZE(frame));
     ASSERT(buf_init(buf, FRAME_HEADROOM(frame)));
-    buf->len = tuntap_buffer ? MAX_RW_SIZE_TUN(frame) : MAX_RW_SIZE_LINK(frame);
+    buf->len = frame->buf.payload_size;
     ASSERT(buf_safe(buf, 0));
 }
 
@@ -296,6 +296,11 @@  frame_print(const struct frame *frame,
         buf_printf(&out, "%s ", prefix);
     }
     buf_printf(&out, "[");
+    buf_printf(&out, " mss_fix:%d", frame->mss_fix);
+    buf_printf(&out, " tun_mtu:%d", frame->tun_mtu);
+    buf_printf(&out, " headroom:%d", frame->buf.headroom);
+    buf_printf(&out, " payload:%d", frame->buf.payload_size);
+    buf_printf(&out, " tailroom:%d", frame->buf.tailroom);
     buf_printf(&out, " L:%d", frame->link_mtu);
     buf_printf(&out, " D:%d", frame->link_mtu_dynamic);
     buf_printf(&out, " EF:%d", frame->extra_frame);
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index c83d8816..930c4b73 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -91,6 +91,25 @@ 
  * Packet geometry parameters.
  */
 struct frame {
+    struct {
+        /* This struct holds all the information about the buffers that are
+         * allocated to match this frame */
+        int payload_size;       /**< the maximum size that a payload that our
+                                 *   buffers can hold from either tun device
+                                 *   or network link.
+                                 */
+
+
+        int headroom;           /**< the headroom in the buffer, this is choosen
+                                 *   to allow all potential header to be added
+                                 *   before the packet */
+
+        int tailroom;            /**< the tailroom in the buffer. Chosen large
+                                  *  enough to also accompany any extrea header
+                                  *  or work space required by
+                                  *  decryption/encryption or compression. */
+    } buf;
+
     int link_mtu;               /**< Maximum packet size to be sent over
                                  *   the external network interface. */
 
@@ -110,6 +129,17 @@  struct frame {
                                  *   @endcode
                                  */
 
+    int tun_mtu;                /**< the (user) configured tun-mtu. This is used
+                                 *   in configuring the tun interface or
+                                 *   in calculations that use the desired size
+                                 *   of the payload in the buffer.
+                                 *
+                                 *   This variable is also used in control
+                                 *   frame context to set the desired maximum
+                                 *   control frame payload (although most of
+                                 *   code ignores it)
+                                 */
+
     int extra_buffer;           /**< Maximum number of bytes that
                                  *   processing steps could expand the
                                  *   internal work buffer.
@@ -165,8 +195,8 @@  struct options;
  * a tap device ifconfiged to an MTU of 1200 might actually want
  * to return a packet size of 1214 on a read().
  */
-#define PAYLOAD_SIZE(f)          ((f)->link_mtu - (f)->extra_frame)
 #define PAYLOAD_SIZE_DYNAMIC(f)  ((f)->link_mtu_dynamic - (f)->extra_frame)
+#define PAYLOAD_SIZE(f)          ((f)->buf.payload_size)
 
 /*
  * Max size of a payload packet after encryption, compression, etc.
@@ -176,35 +206,23 @@  struct options;
 #define EXPANDED_SIZE_DYNAMIC(f) ((f)->link_mtu_dynamic)
 #define EXPANDED_SIZE_MIN(f)     (TUN_MTU_MIN + TUN_LINK_DELTA(f))
 
-/*
- * These values are used as maximum size constraints
- * on read() or write() from TUN/TAP device or TCP/UDP port.
- */
-#define MAX_RW_SIZE_TUN(f)       (PAYLOAD_SIZE(f))
-#define MAX_RW_SIZE_LINK(f)      (EXPANDED_SIZE(f) + (f)->extra_link)
-
 /*
  * Control buffer headroom allocations to allow for efficient prepending.
  */
-#define FRAME_HEADROOM_BASE(f)     (TUN_LINK_DELTA(f) + (f)->extra_buffer + (f)->extra_link)
-/* Same as FRAME_HEADROOM_BASE but rounded up to next multiple of PAYLOAD_ALIGN */
-#define FRAME_HEADROOM(f)          frame_headroom(f)
 
 /*
  * Max size of a buffer used to build a packet for output to
- * the TCP/UDP port.
- *
- * the FRAME_HEADROOM_BASE(f) * 2 should not be necessary but it looks that at
- * some point in the past we seem to have lost the information what parts of
- * the extra space we need to have before the data and which we need after
- * the data. So we ensure we have the FRAME_HEADROOM before and after the
- * actual data.
+ * the TCP/UDP port or to read a packet from a tap/tun device.
  *
  * Most of our code only prepends headers but compression needs the extra bytes
  * *after* the data as compressed data might end up larger than the original
- * data (and max compression overhead is part of extra_buffer)
+ * data (and max compression overhead is part of extra_buffer). Also crypto
+ * needs an extra block for encryption. Therefore tailroom is larger than the
+ * headroom.
  */
-#define BUF_SIZE(f)              (TUN_MTU_SIZE(f) + FRAME_HEADROOM_BASE(f) * 2)
+#define BUF_SIZE(f) ((f)->buf.headroom + (f)->buf.payload_size + (f)->buf.tailroom)
+
+#define FRAME_HEADROOM(f)          ((f)->buf.headroom)
 
 /*
  * Function prototypes.
@@ -326,20 +344,6 @@  const char *format_extended_socket_error(int fd, int *mtu, struct gc_arena *gc);
 
 #endif
 
-/*
- * Calculate a starting offset into a buffer object, dealing with
- * headroom and alignment issues.
- */
-static inline int
-frame_headroom(const struct frame *f)
-{
-    const int offset = FRAME_HEADROOM_BASE(f);
-    /* These two lines just pad offset to next multiple of PAYLOAD_ALIGN in
-     * a complicated and confusing way */
-    const int delta = ((PAYLOAD_ALIGN << 24) - offset) & (PAYLOAD_ALIGN - 1);
-    return offset + delta;
-}
-
 /*
  * frame member adjustment functions
  */
@@ -383,7 +387,7 @@  frame_add_to_extra_buffer(struct frame *frame, const int increment)
 static inline bool
 frame_defined(const struct frame *frame)
 {
-    return frame->link_mtu > 0;
+    return frame->buf.payload_size > 0;
 }
 
 #endif /* ifndef MTU_H */
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 103e882e..e5ffebff 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3495,7 +3495,7 @@  gremlin_flood_clients(struct multi_context *m)
         int i;
 
         ASSERT(buf_init(&buf, FRAME_HEADROOM(&m->top.c2.frame)));
-        parm.packet_size = min_int(parm.packet_size, MAX_RW_SIZE_TUN(&m->top.c2.frame));
+        parm.packet_size = min_int(parm.packet_size, m->top.c2.frame.buf.payload_size);
 
         msg(D_GREMLIN, "GREMLIN_FLOOD_CLIENTS: flooding clients with %d packets of size %d",
             parm.n_packets,
@@ -3557,7 +3557,7 @@  multi_process_per_second_timers_dowork(struct multi_context *m)
 }
 
 void
-multi_top_init(struct multi_context *m, const struct context *top)
+multi_top_init(struct multi_context *m, struct context *top)
 {
     inherit_context_top(&m->top, top);
     m->top.c2.buffers = init_context_buffers(&top->c2.frame);
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 6e85c21c..c2b085e3 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -257,7 +257,7 @@  void multi_init(struct multi_context *m, struct context *t, bool tcp_mode);
 
 void multi_uninit(struct multi_context *m);
 
-void multi_top_init(struct multi_context *m, const struct context *top);
+void multi_top_init(struct multi_context *m, struct context *top);
 
 void multi_top_free(struct multi_context *m);
 
diff --git a/src/openvpn/occ.c b/src/openvpn/occ.c
index 610c05f5..c4e7c1be 100644
--- a/src/openvpn/occ.c
+++ b/src/openvpn/occ.c
@@ -219,7 +219,7 @@  check_send_occ_msg_dowork(struct context *c)
 
     c->c2.buf = c->c2.buffers->aux_buf;
     ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
-    ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
+    ASSERT(buf_safe(&c->c2.buf, c->c2.frame.buf.payload_size));
     ASSERT(buf_write(&c->c2.buf, occ_magic, OCC_STRING_SIZE));
 
     switch (c->c2.occ_op)
@@ -319,7 +319,7 @@  check_send_occ_msg_dowork(struct context *c)
                  OCC_STRING_SIZE,
                  (int) sizeof(uint8_t),
                  EXTRA_FRAME(&c->c2.frame),
-                 MAX_RW_SIZE_TUN(&c->c2.frame),
+                 c->c2.frame.buf.payload_size,
                  BLEN(&c->c2.buf));
             doit = true;
         }
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 1fd9b78c..c7a86bd2 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3833,7 +3833,7 @@  options_string(const struct options *o,
     buf_printf(&out, ",link-mtu %u",
                (unsigned int) calc_options_string_link_mtu(o, frame));
 
-    buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
+    buf_printf(&out, ",tun-mtu %d", frame->tun_mtu);
     buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
 
     bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
diff --git a/src/openvpn/ping.c b/src/openvpn/ping.c
index 67bbca14..a28f347f 100644
--- a/src/openvpn/ping.c
+++ b/src/openvpn/ping.c
@@ -80,7 +80,7 @@  check_ping_send_dowork(struct context *c)
 {
     c->c2.buf = c->c2.buffers->aux_buf;
     ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
-    ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
+    ASSERT(buf_safe(&c->c2.buf, c->c2.frame.buf.payload_size));
     ASSERT(buf_write(&c->c2.buf, ping_string, sizeof(ping_string)));
 
     /*
diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c
index 08c9ab19..ef17aae7 100644
--- a/src/openvpn/reliable.c
+++ b/src/openvpn/reliable.c
@@ -205,8 +205,6 @@  error:
     return false;
 }
 
-#define ACK_SIZE(n) (sizeof(uint8_t) + ((n) ? SID_SIZE : 0) + sizeof(packet_id_type) * (n))
-
 /* write a packet ID acknowledgement record to buf, */
 /* removing all acknowledged entries from ack */
 bool
diff --git a/src/openvpn/reliable.h b/src/openvpn/reliable.h
index 693abb3c..02484d1e 100644
--- a/src/openvpn/reliable.h
+++ b/src/openvpn/reliable.h
@@ -66,6 +66,9 @@  struct reliable_ack
     packet_id_type packet_id[RELIABLE_ACK_SIZE];
 };
 
+/* The size of the ACK header */
+#define ACK_SIZE(n) (sizeof(uint8_t) + ((n) ? SID_SIZE : 0) + sizeof(packet_id_type) * (n))
+
 /**
  * The structure in which the reliability layer stores a single incoming
  * or outgoing packet.
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 891355fb..772d17c5 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -332,6 +332,35 @@  tls_init_control_channel_frame_parameters(const struct frame *data_channel_frame
     /* set dynamic link MTU to cap control channel packets at 1250 bytes */
     ASSERT(TUN_LINK_DELTA(frame) < min_int(frame->link_mtu, 1250));
     frame->link_mtu_dynamic = min_int(frame->link_mtu, 1250) - TUN_LINK_DELTA(frame);
+
+    /* calculate the maximum overhead that control channel frames may have */
+    int overhead = 0;
+
+    /* Socks */
+    overhead += 10;
+
+    /* tls-auth and tls-crypt */
+    overhead += max_int(tls_crypt_buf_overhead(),
+                        packet_id_size(true) + OPENVPN_MAX_HMAC_SIZE);
+
+    /* TCP length field and opcode */
+    overhead+= 3;
+
+    /* ACK array and remote SESSION ID (part of the ACK array) */
+    overhead += ACK_SIZE(RELIABLE_ACK_SIZE);
+
+    /* Previous OpenVPN version calculated the maximum size and buffer of a
+     * control frame depending on the overhead of the data channel frame
+     * overhead and limited its maximum size to 1250. We always allocate the
+     * 1250 buffer size since a lot of code blindly assumes a large buffer
+     * (e.g. PUSH_BUNDLE_SIZE) and set frame->mtu_mtu as suggestion for the
+     * size */
+    frame->buf.payload_size = 1250 + overhead;
+
+    frame->buf.headroom = overhead;
+    frame->buf.tailroom = overhead;
+
+    frame->tun_mtu = min_int(data_channel_frame->tun_mtu, 1250);
 }
 
 void
@@ -1875,13 +1904,6 @@  tls_session_update_crypto_params_do_work(struct tls_session *session,
         msg(D_HANDSHAKE, "Data Channel: using negotiated cipher '%s'",
             options->ciphername);
     }
-    else
-    {
-        /* Very hacky workaround and quick fix for frame calculation
-         * different when adjusting frame size when the original and new cipher
-         * are identical to avoid a regression with client without NCP */
-        return tls_session_generate_data_channel_keys(session);
-    }
 
     init_key_type(&session->opt->key_type, options->ciphername,
                   options->authname, true, true);
@@ -2964,7 +2986,7 @@  tls_process(struct tls_multi *multi,
             buf = reliable_get_buf_output_sequenced(ks->send_reliable);
             if (buf)
             {
-                int status = key_state_read_ciphertext(&ks->ks_ssl, buf, PAYLOAD_SIZE_DYNAMIC(&multi->opt.frame));
+                int status = key_state_read_ciphertext(&ks->ks_ssl, buf, multi->opt.frame.tun_mtu);
                 if (status == -1)
                 {
                     msg(D_TLS_ERRORS,
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index f851bd2b..ada68b4b 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -221,8 +221,9 @@  struct key_state
     struct reliable *rec_reliable; /* order incoming ciphertext packets before we pass to TLS */
     struct reliable_ack *rec_ack; /* buffers all packet IDs we want to ACK back to sender */
 
+    /** Holds outgoing message for the control channel until ks->state reaches
+     * S_ACTIVE */
     struct buffer_list *paybuf;
-
     counter_type n_bytes;                /* how many bytes sent/recvd since last key exchange */
     counter_type n_packets;              /* how many packets sent/recvd since last key exchange */