You are right, my version is buggy.
padding_len/plaintext_len are size_t, t_mtu is int. My unconditional
MIN() clamps plaintext_len below m_pkthdr.len when len > t_mtu, so
padding_len = plaintext_len - m->m_pkthdr.len wraps to ~SIZE_MAX and the
bzero() at if_wg.c:1615 runs off the buffer. Your len <= t_mtu guard is
exactly what prevents that. I withdraw my diff in favour of yours.
I think your original diff is good as-is. Thanks for catching the wrap.
I tried answering your questions below.
On Fri 07/08/2026 19:58, Zixu Wu wrote:
> The change I proposed is intended to be conservative, imposing limits on
> padding if and only if it is the reason packet length goes above the MTU,
> otherwise keeping the existing behavior.
>
> Bjorn's change aims to address the broader MTU issue (whether or not is
> caused by padding) more aggressively.
>
> One observations about the new proposal:
>
> - When `m->m_pkthdr.len` is already above `t->t_mtu` (the scenario Bjorn
> seeks to address), the calculation of `padding_len` triggers unsigned
> integer wrapping, potentially causing unintended effect on the `bzero()`
> that follows.
>
> And a few questions:
>
> 1. Why truncate not discard?
In the only reachable case (len <= t_mtu) the clamp trims padding only,
never real data, lossless, so no reason to discard.
> 2. What are all the scenarios where `m_pkthdr.len` could go beyond
> `t->t_mtu`?
Normal path: it cannot. t_mtu is if_mtu at wg_output time (if_wg.c:2286)
and IP fragments to the MTU before if_output. t_mtu == 0 is keepalives,
where len == 0 too. That is why leaving the over-MTU branch untouched
(your else) is correct.
> 3. Depending on the scenarios in 2. should ICMP need frag / ICMP6 too large
> or a debug message be generated?
I would not originate it from wg_encap. PMTU is handled a layer up, and
the mbuf here is already queued for encryption. Per (2) the branch
should not fire anyway.
> 4. Are there any pathway in OpenBSD where the `if_mtu` member `struct ifnet`
> can become less than or equal to zero?
Not for wg. if_mtu is uint32_t (if_var.h:191), SIOCSIFMTU rejects <= 0
and > 9000 (if_wg.c:2595), attach sets 1420. So t_mtu is [1,9000] or 0
for keepalive, both covered by your guard.
> Thanks.
>
>
> On 8/7/26 17:45, Bjorn Ketelaars wrote:
> > On Fri 07/08/2026 07:17, Zixu Wu wrote:
> > > Zero padding shouldn't cause the packet size to go beyond MTU.
> > >
> > > diff --git a/sys/net/if_wg.c b/sys/net/if_wg.c
> > > index 0641c4b5ba9..90ef5b808a0 100644
> > > --- a/sys/net/if_wg.c
> > > +++ b/sys/net/if_wg.c
> > > @@ -1592,6 +1592,9 @@ wg_encap(struct wg_softc *sc, struct mbuf *m)
> > > peer = t->t_peer;
> > >
> > > plaintext_len = WG_PKT_WITH_PADDING(m->m_pkthdr.len);
> > > + if (m->m_pkthdr.len <= t->t_mtu && plaintext_len > t->t_mtu) {
> > > + plaintext_len = t->t_mtu;
> > > + }
> > > padding_len = plaintext_len - m->m_pkthdr.len;
> > > out_len = sizeof(struct wg_pkt_data) + plaintext_len +
> > > NOISE_AUTHTAG_LEN;
> >
> > I think the premise of Zixu's patch is valid: with t_mtu = if_mtu = 1420
> > (default, not a multiple of 16), inner packets of 1409–1420 round up to
> > 1424 via WG_PKT_WITH_PADDING. Adding the 80-byte wg envelope pushes
> > total volume to 1504 bytes, exceeding the standard transport capacity
> > and forcing the network to split single packages into fragments.
> >
> > Zixu's proposed change:
> >
> > if (m->m_pkthdr.len <= t->t_mtu && plaintext_len > t->t_mtu) {
> > plaintext_len = t->t_mtu;
> > }
> >
> > However:
> > - If an incoming package already exceeds the limit (m->m_pkthdr.len >
> > t->t_mtu), the guard condition evaluates to false. The restriction is
> > bypassed completely, allowing padding to inflate the payload even
> > further. The MTU ceiling must apply unconditionally.
> > - System keepalives initialize with zeroed values (t_mtu = 0, len = 0).
> > Applying padding to zero yields 16 bytes, triggering the clamp back to
> > 0 only by coincidence. Explicitly checking (t->t_mtu != 0) makes the
> > logic transparent: keepalives carry no MTU constraint, whereas data
> > payloads must strictly observe it.
> >
> > Using MIN() provides an explicit, elegant safeguard while maintaining
> > clean code conventions:
> >
> > diff --git sys/net/if_wg.c sys/net/if_wg.c
> > index 0641c4b5ba9..1a00af9dc1d 100644
> > --- sys/net/if_wg.c
> > +++ sys/net/if_wg.c
> > @@ -1592,6 +1592,8 @@ wg_encap(struct wg_softc *sc, struct mbuf *m)
> > peer = t->t_peer;
> > plaintext_len = WG_PKT_WITH_PADDING(m->m_pkthdr.len);
> > + if (t->t_mtu != 0)
> > + plaintext_len = MIN(plaintext_len, t->t_mtu);
> > padding_len = plaintext_len - m->m_pkthdr.len;
> > out_len = sizeof(struct wg_pkt_data) + plaintext_len +
> > NOISE_AUTHTAG_LEN;
>
