Re: [PATCH net v2] tipc: raise the minimum bearer MTU to fit tunnel and crypto overhead

[Paolo Abeni <pabeni@redhat.com>]

On 10/13/26 16:48, Joas Antonio dos Santos wrote:
> A peer can lower the link MTU to TIPC_MIN_BEARER_MTU (100 bytes, plus
> the UDP encapsulation header) with the max_pkt field of a RESET or
> ACTIVATE message; tipc_link_proto_rcv() only checks the lower bound
> returned by tipc_bearer_min_mtu().  The same bound is applied to the
> device MTU when a bearer is enabled.
> 
> That bound predates two later reductions of the link MSS:
> 
>    tipc_link_mss() = mtu - INT_H_SIZE                    (tunnel header)
>                           - EMSG_OVERHEAD               (CONFIG_TIPC_CRYPTO)
> 
> With CONFIG_TIPC_CRYPTO=y, which is the default, an MTU of 100 gives an
> MSS of 28.  Once the link comes up, tipc_named_node_up() calls
> named_distribute(), which computes
> 
>    msg_dsz = ((tipc_node_get_mtu() - INT_H_SIZE) / ITEM_SIZE) * ITEM_SIZE;
> 
> For an MSS of 28 the subtraction is negative and the result truncates to
> 0xfffffff0; for an MSS between 40 and 59 it is 0.  named_prepare_buf()
> then allocates a buffer with no room for any item (the u32 size wraps),
> and msg_rem never reaches zero, so every cluster-scope publication on
> the node is written with publ_to_item() past the end of that one skb,
> over skb_shared_info and into adjacent memory.
> 
> Without crypto the MSS is at least 60, which still does not hold the
> first fragment built by tipc_msg_build(): INT_H_SIZE fragment header
> plus a message header of up to MAX_H_SIZE.  With pktmax below that, the
> header copies run past the pktmax bytes allocated for the fragment.
> 
> Raise TIPC_MIN_BEARER_MTU to MAX_H_SIZE + 2 * INT_H_SIZE so the MSS can
> always hold INT_H_SIZE + MAX_H_SIZE, which also leaves room for name
> table items, and add TIPC_MIN_BEARER_MTU_CRYPTO, which adds
> EMSG_OVERHEAD on top, for CONFIG_TIPC_CRYPTO=y.  Use the crypto bound
> for the device MTU check (tipc_mtu_bad_crypto()), the UDP MTU check,
> the netlink MTU setting and tipc_bearer_min_mtu(), which also bounds
> the max_pkt negotiation.
> 
> Found by manual code review, assisted by an LLM, and reproduced with
> KASAN using two network namespaces joined by a veth pair (see below).
> 
> Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
> Fixes: 2320bcdae628 ("tipc: fix changeover issues due to large packet")
> Suggested-by: Tung Nguyen <tung.quang.nguyen@est.tech>
> Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
> Assisted-by: Claude:claude-opus-5-5
> ---
> Changes in v2:
> - Derive the crypto bound from EMSG_OVERHEAD instead of a hard-coded 32:
>    TIPC_MIN_BEARER_MTU_CRYPTO in crypto.h and tipc_mtu_bad_crypto(), as
>    suggested by Tung.  Drop the BUILD_BUG_ON().
> - Apply the crypto bound to the UDP MTU check and the netlink MTU
>    setting too.
> 
> Tested on 602042bf29f6 (arm64, QEMU, KASAN); net/tipc/ builds cleanly
> with W=1 with and without CONFIG_TIPC_CRYPTO.  With the reproducer
> below (node A with 40 cluster-scope publications and MTU 1500, node B
> enabling its bearer on a veth with the given MTU):
> 
>    config            peer MTU   bearer on B         link   KASAN
>    unpatched            100     enabled              up      slab-out-of-bounds
>                                                              in publ_to_item()
>    TIPC_CRYPTO=y        100     refused (too low)    -       none
>    TIPC_CRYPTO=y        171     refused (too low)    -       none
>    TIPC_CRYPTO=y        172     enabled              up      none
>    TIPC_CRYPTO=y       1500     enabled              up      none
>    TIPC_CRYPTO=n        100     refused (too low)    -       none
>    TIPC_CRYPTO=n        139     refused (too low)    -       none
>    TIPC_CRYPTO=n        140     enabled              up      none
>    TIPC_CRYPTO=n       1500     enabled              up      none
> 
> The reproducer exercises the bearer-enable check; the max_pkt
> negotiation in tipc_link_proto_rcv() uses the same bound through
> tipc_bearer_min_mtu() but was not hit separately with forged messages.
> 
> This edits lines next to the ones touched by Yunpeng Tian's
> "tipc: bound the device MTU accepted ..." (upper bound, posted
> 2026-10-03); the two are independent and I can rebase if that one is
> applied first.
> 
> Reproducer (as root; build with "gcc -Wall -O2 -o pub pub.c", run
> "sh tipc_repro.sh [peer_mtu]"):
> 
> pub.c:
> 
>    /* Bind N TIPC services with cluster scope, then sleep. */
>    #include <linux/tipc.h>
>    #include <stdio.h>
>    #include <stdlib.h>
>    #include <sys/socket.h>
>    #include <unistd.h>
> 
>    int main(int argc, char **argv)
>    {
>    	int n = argc > 1 ? atoi(argv[1]) : 40;
>    	for (int i = 0; i < n; i++) {
>    		struct sockaddr_tipc a = {
>    			.family = AF_TIPC,
>    			.addrtype = TIPC_SERVICE_RANGE,
>    			.scope = TIPC_CLUSTER_SCOPE,
>    			.addr.nameseq = { .type = 1000 + i, .lower = 0, .upper = 0 },
>    		};
>    		int s = socket(AF_TIPC, SOCK_RDM, 0);
>    		if (s < 0 || bind(s, (struct sockaddr *)&a, sizeof(a))) {
>    			perror("tipc bind");
>    			return 1;
>    		}
>    	}
>    	printf("pub: bound %d cluster-scope services\n", n);
>    	fflush(stdout);
>    	pause();
>    	return 0;
>    }
> 
> tipc_repro.sh:
> 
>    #!/bin/sh
>    # TIPC named_distribute() overflow reproducer.
>    # Needs root, CONFIG_TIPC=y, CONFIG_TIPC_CRYPTO=y (default), KASAN recommended.
>    # Usage: ./tipc_repro.sh [peer_mtu]   (default 100; 100..131 trigger it)
>    PEER_MTU=${1:-100}
> 
>    ip netns add A
>    ip netns add B
>    ip link add va type veth peer name vb
>    ip link set va netns A
>    ip link set vb netns B
>    ip -n A link set lo up
>    ip -n B link set lo up
>    ip -n A link set va mtu 1500 up
>    ip -n B link set vb mtu "$PEER_MTU" up
> 
>    # node A: 40 cluster-scope publications (any number >= 1 overflows;
>    # more publications write further past the buffer)
>    ip netns exec A ./pub 40 &
>    sleep 2
> 
>    # node A uses MTU 1500, node B uses PEER_MTU; the link MTU is
>    # negotiated down to PEER_MTU, the link MSS becomes PEER_MTU - 72
>    ip netns exec A tipc bearer enable media eth dev va
>    ip netns exec B tipc bearer enable media eth dev vb
> 
>    # the overflow happens in tipc_named_node_up() as soon as the link
>    # comes up (a few seconds)
>    sleep 20
>    dmesg | grep -A30 -E "KASAN|BUG:|Oops"
> 
>   net/tipc/bearer.c    | 16 ++++++++++++++++
>   net/tipc/bearer.h    |  2 +-
>   net/tipc/crypto.h    | 19 +++++++++++++++++++
>   net/tipc/udp_media.h | 10 ++++++++--
>   4 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 05dcd2f9e..02b2e9f56 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -433,7 +433,11 @@ int tipc_enable_l2_media(struct net *net, struct tipc_bearer *b,
>   	dev = dev_get_by_name(net, dev_name);
>   	if (!dev)
>   		return -ENODEV;
> +#ifdef CONFIG_TIPC_CRYPTO
> +	if (tipc_mtu_bad_crypto(dev)) {
> +#else
>   	if (tipc_mtu_bad(dev)) {
> +#endif

You can (and should) avoid most of the newly introduced #ifdefs embedding
the dependency in appropriate helpers.

/P