Re: [PATCH net v2 14/14] batman-adv: tt: limit local table to single TVLV response size

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message describes the pre-patch behaviour wrongly.
- [Low] Two kernel-doc comments no longer match the code after this
  patch.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the same 16-bit
  tvlv_len limit is still not enforced when a node answers a TT_REQUEST…
- [Low] This problem wasn't introduced by this patch, but the size limit
  is read before the lock is taken.

--

batman-adv: tt: limit local table to single TVLV response size

This patch adds batadv_tt_local_transmit_size_max(). It caps the local TT
reservation and the resize-to-MTU limit at the smaller of packet_size_max
and the largest full table response that fits in a single unicast TVLV
packet with a 16 bit tvlv_len. The reservation logic now computes the new
totals in locals and writes the counters only when the reservation
succeeds.

> The size limit was already missing the 16 bit tvlv_len cap since the
> original packet_size_max check was introduced. But this check was only
> reachable with fragmentation disabled and a hard interface MTU larger than
> the maximum unicast TVLV packet. In this case,
> batadv_tt_prepare_tvlv_local_data() refuses to build the full table
> response and no oversized packet is created.

[Severity: Low]

Does this describe the old behaviour accurately?

batadv_tt_prepare_tvlv_local_data() only bounds the TT value with a u16,
and does not count the 4 byte struct batadv_tvlv_hdr:

	if (check_add_overflow(*tt_len, change_offset, &tvlv_len)) {

batadv_tvlv_unicast_send() then adds the header and stores the result
without a range check:

	tvlv_len = sizeof(*tvlv_hdr) + tvlv_value_len;
	...
	unicast_tvlv_packet->tvlv_len = htons(tvlv_len);

All TT components are multiples of 4, so a 65532 byte TT value is
reachable. For example, 1 VLAN and 5460 entries gives 4 + 8 + 65520.

Before this patch, the reservation allowed that table whenever
packet_size_max >= 65556. The full table response would then be built and
sent with tvlv_len = 65536, which htons() truncates to 0. So an oversized,
malformed packet does seem to have been created. The receiver in
batadv_recv_unicast_tvlv() then just processes no TVLVs.

The code change handles this for the local table: table_size includes the
TVLV header and is capped at sizeof(struct batadv_unicast_tvlv_packet) +
U16_MAX.

The stable+noautosel reasoning depends on how the old behaviour is
described. Could the commit message be made more precise here?

> diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> index f0100ccf93deb..46304069b3b75 100644
> --- a/net/batman-adv/translation-table.c
> +++ b/net/batman-adv/translation-table.c

[ ... ]

> @@ -266,6 +269,25 @@ static int batadv_tt_local_transmit_size(u16 num_vlan, u16 num_entries)
>  	return hdr_size + batadv_tt_len(num_entries);
>  }
>  
> +/**
> + * batadv_tt_local_transmit_size_max() - get the maximum size of a full local table
> + *  response
> + * @bat_priv: the bat priv with all the mesh interface information
> + *
> + * The full table response is sent as a single unicast TVLV packet. Its
> + * tvlv_len, which covers the TVLV header and the TT TVLV, is only 16 bit wide.
> + * This limits the response size even when packet_size_max is larger.
> + *
> + * Return: maximum size of a full local table response in bytes
> + */
> +static int batadv_tt_local_transmit_size_max(struct batadv_priv *bat_priv)
> +{
> +	int tvlv_size_max = sizeof(struct batadv_unicast_tvlv_packet) + U16_MAX;
> +	int packet_size_max = READ_ONCE(bat_priv->packet_size_max);
> +
> +	return min(packet_size_max, tvlv_size_max);
> +}

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch. The
same 16 bit tvlv_len limit does not seem to be enforced when a node
answers a TT_REQUEST on behalf of another originator.
batadv_send_other_tt_response() only checks this:

	/* Don't send the response, if larger than fragmented packet. */
	tt_len = sizeof(struct batadv_unicast_tvlv_packet) + tvlv_len;
	if (tt_len > READ_ONCE(bat_priv->packet_size_max)) {

That check leaves out the 4 byte batadv_tvlv_hdr. It also has no cap at
sizeof(struct batadv_unicast_tvlv_packet) + U16_MAX.
batadv_tt_prepare_tvlv_global_data() has the same u16-only
check_add_overflow() as the local variant.

A remote originator's global entries come from OGM TT diffs. The local
reserve_lock accounting does not limit them.

Take fragmentation disabled and a hard interface MTU >= 65552. A relayed
table with a 65532 byte TT value (1 VLAN and 5460 entries) passes both
checks. batadv_tvlv_unicast_send() then sends it with tvlv_len truncated
from 65536 to 0.

Only the local reservation and resize paths use the new
batadv_tt_local_transmit_size_max(). Would a check in
batadv_tvlv_unicast_send() that rejects tvlv_value_len >
U16_MAX - sizeof(struct batadv_tvlv_hdr) cover every caller?

> @@ -290,22 +312,23 @@ static int batadv_tt_local_transmit_size(u16 num_vlan, u16 num_entries)
>  static bool batadv_tt_local_reserve(struct batadv_priv *bat_priv, u16 num_vlan,
>  				    u16 num_entries, int *table_size)
>  {
> -	int packet_size_max = READ_ONCE(bat_priv->packet_size_max);
> +	int size_max = batadv_tt_local_transmit_size_max(bat_priv);
> +	unsigned int entries;
> +	unsigned int vlans;
>  
>  	scoped_guard(spinlock_bh, &bat_priv->tt.reserve_lock) {

[Severity: Low]

This isn't a bug introduced by this patch, but the limit is still read
before reserve_lock is taken. The old code had the same READ_ONCE() before
the guard. batadv_tt_local_resize_to_mtu() also reads the limit before
taking commit_lock.

Can a concurrent MTU decrease leave the local table larger than the new
packet_size_max?

CPU0                                CPU1
batadv_tt_local_add()
 batadv_tt_local_reserve_entry()
  batadv_tt_local_reserve()
   size_max = <old, larger limit>
                                    batadv_update_min_mtu()
                                     batadv_hardif_min_mtu()
                                      WRITE_ONCE(packet_size_max, ...)
                                     batadv_tt_local_resize_to_mtu()
                                      /* table fits, nothing purged */
   scoped_guard(reserve_lock)
    reservation committed against
    the stale size_max
 entry inserted

The purge decision uses batadv_tt_local_table_transmit_size(), which only
counts vlan->tt.num_entries. It does not see in-flight reservations
(tt.reserved_entries and tt.reserved_vlans) or entries that still carry
BATADV_TT_CLIENT_NEW.

Nothing checks the table again until entries age out, so full table
responses may be dropped until then.

[ ... ]

> @@ -4727,16 +4752,16 @@ void batadv_tt_local_resize_to_mtu(struct net_device *mesh_iface)
>  	struct batadv_priv *bat_priv = netdev_priv(mesh_iface);
>  	int timeout = BATADV_TT_LOCAL_TIMEOUT / 2;
>  	bool reduced = false;
> -	int packet_size_max;
>  	int table_size;
> +	int size_max;
>  
> -	packet_size_max = READ_ONCE(bat_priv->packet_size_max);
> +	size_max = batadv_tt_local_transmit_size_max(bat_priv);
>  
>  	spin_lock_bh(&bat_priv->tt.commit_lock);
>  
>  	while (timeout) {
>  		table_size = batadv_tt_local_table_transmit_size(bat_priv);
> -		if (packet_size_max >= table_size)
> +		if (size_max >= table_size)
>  			break;

[Severity: Low]

Should the kernel-doc be updated to match? The kernel-doc for
batadv_tt_local_resize_to_mtu() still says:

 * batadv_tt_local_resize_to_mtu() - resize the local translation table fit the
 *  maximum packet size that can be transported through the mesh

The loop now compares against batadv_tt_local_transmit_size_max(), and the
log message below already says "maximum table response size".

The kernel-doc for batadv_tt_local_reserve() also still describes the old
add-then-drop handling:

 * Add the requested number of VLANs and local TT entries to the reservation
 * counters and check whether the local translation table would then still fit
 * in a single full table response. The reservation is dropped again when it
 * would not.

The new body computes the totals in vlans and entries and writes the
counters only on success. Nothing is dropped any more.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030135105.2555887-2-sw%40simonwunderlich.de