Re: if_wg.c: Zero Padding Missing MTU Check

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;
>