Re: if_wg.c: Zero Padding Missing MTU Check

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?

2. What are all the scenarios where `m_pkthdr.len` could go beyond 
`t->t_mtu`?

3. Depending on the scenarios in 2. should ICMP need frag / ICMP6 too 
large or a debug message be generated?

4. Are there any pathway in OpenBSD where the `if_mtu` member `struct 
ifnet` can become less than or equal to zero?

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;