Re: [PATCH net v5 5/5] amt: harden relay authentication and teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch makes several security-relevant behaviour changes
  that the changelog does not state.
- [Medium] AMT_MAX_TUNNELS_PER_SRC is fixed at 16, but the global table
  size amt->max_tunnels can be set by the administrator.
- [Low] The subject line says "harden relay authentication and teardown",
  but the patch does not change any teardown path.
- [Low] amt_request_handler() takes the device-wide amt->lock with
  spin_lock_bh() for every Request that passes the header checks, forged…
- [Low] The new comments on amt_send_igmp_gq() ('the requesting source is
  not authenticated yet and no tunnel state exists for it') and…

--

amt: harden relay authentication and teardown

This patch makes AMT Relay Request handling stateless. The Query MAC is
now derived from the source address, source port, nonce and current
secret, and tunnel allocation moves to the Membership Update path after
the MAC is verified. Tunnels are keyed by address and port, with a global
limit, a fixed per-source limit of 16, and four secret generations
protected by a new seqcount, amt->key_seq.

> amt: harden relay authentication and teardown

[Severity: Low]
Is "teardown" accurate in the subject? This patch doesn't change
amt_tunnel_expire() or amt_dev_stop(), and the commit message body
doesn't mention teardown.

The only teardown-related edits in include/net/amt.h re-word the struct
amt_tunnel_list comments on last_seen and dead. Their meaning stays the
same, for example:

-	 * tunnel first; the other then leaves it alone, so it is freed once.
+	 * tunnel first;
+	 * the other then leaves it alone, so the tunnel is freed exactly once.

Those comments were added by the earlier patch in this series, "amt: fix
use-after-free when a Membership Update re-arms a freed tunnel".
Re-flowing them here can make backports of either patch conflict.

The one code change related to teardown is the new netif_running()
recheck under amt->lock in amt_tunnel_get_or_create():

	if (!netif_running(amt->dev)) {
		spin_unlock_bh(&amt->lock);
		return NULL;
	}

The commit message doesn't describe this recheck.

> Keep Requests stateless by deriving the Query MAC from the source address,
> source port, nonce, and current secret. Authenticate the Membership Update
> before decapsulation or allocation, key verified state by the address/port
> endpoint, enforce global and per-source limits, and silently reject an
> endpoint that cannot be admitted. Retain four secret generations so a
> gateway can renew its MAC across rotations.

[Severity: Medium]
Should the changelog describe a few more behaviour changes? The patch has
a Fixes: tag for net, so stable maintainers will need to see them.

First, amt_dev_open() now seeds amt->key before amt_socket_create()
instead of after it. In the old code, a Request that arrived in that
window reached amt_request_handler() and ran:

	tunnel->key = amt->key;

At that point amt->key was still zero on the first open, or still the
previous key on later opens, and the tunnel kept that key for its whole
lifetime. This is a separate fix, and only an in-code comment explains
it.

Second, tunnel->key used to be copied once when the tunnel was created
and was never refreshed. Rotation in amt_secret_work() never affected
existing tunnels, so their MACs never expired.

Now every MAC is valid only for the four-generation window (180-240s). If
a gateway doesn't send a new Request within that window, its Updates are
dropped as "Invalid MAC".

Third, amt_update_handler() used to accept only the nonce from the most
recent Request:

	if ((nonce == tunnel->nonce &&
	     response_mac == tunnel->mac)) {

It now accepts any (nonce, MAC) pair minted for saddr:sport under
key[0..3]. The replay window grows from "until the next Request" to
180-240s.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 26a942c2a1267..312b36b54ff8e 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -857,11 +857,16 @@ static void amt_send_request(struct amt_dev *amt, bool v6)

[ ... ]

> -static void amt_send_igmp_gq(struct amt_dev *amt,
> -			     struct amt_tunnel_list *tunnel)
> +/* The destination is passed by value rather than as a tunnel: the requesting
> + * source is not authenticated yet and no tunnel state exists for it - see
> + * amt_request_handler().
> + */
> +static void amt_send_igmp_gq(struct amt_dev *amt, __be32 daddr, __be16 dport,
> +			     __be32 nonce, u64 mac)

[ ... ]

> @@ -1198,9 +1216,16 @@ static void amt_send_multicast_data(struct amt_dev *amt,
>  			    0);
>  }
>  
> +/* Send a Membership Query to a source that has not been authenticated yet.
> + *
> + * No per-source state exists at this point and none is created: the nonce and
> + * the keyed MAC are carried in the Query and recomputed from the Membership
> + * Update when it comes back, so everything this needs is passed by value.
> + */

[Severity: Low]
Do these two comments hold on the renewal path? When a tunnel for
saddr:sport already exists, amt_request_handler() sets have, skips the
admission check, and still sends the Query:

		if (tunnel->source_port == udph->source)
			have = true;
	...
	if (!have && (full || per_src >= AMT_MAX_TUNNELS_PER_SRC))
		return true;

amt_secret_work() and amt_update_handler() rely on this periodic Request
renewal, and on that path tunnel state does exist for the destination.

The helpers work either way because every input is passed by value. The
comments still describe a precondition that the code doesn't enforce.

[ ... ]

> @@ -2586,16 +2610,107 @@ static bool amt_membership_query_handler(struct amt_dev *amt,

[ ... ]

> +/* Look up the tunnel for a source whose Membership Update has already been
> + * authenticated, creating it on first contact.
> + *
> + * This is now the only place tunnel state is allocated, so amt->max_tunnels
> + * bounds the number of *verified* gateways rather than the number of
> + * unverified Requests anyone can send.  A fixed per-source cap additionally
> + * stops one verified source address from monopolising the table.
> + */

[ ... ]

> +	if (amt->nr_tunnels >= amt->max_tunnels ||
> +	    per_src >= AMT_MAX_TUNNELS_PER_SRC) {
> +		spin_unlock_bh(&amt->lock);
> +		return NULL;
> +	}

[Severity: Medium]
Does the per-source cap still work when amt->max_tunnels is 16 or less?
amt_newlink() accepts any nonzero IFLA_AMT_MAX_TUNNELS:

	if (data[IFLA_AMT_MAX_TUNNELS] &&
	    nla_get_u32(data[IFLA_AMT_MAX_TUNNELS]))
		amt->max_tunnels = nla_get_u32(data[IFLA_AMT_MAX_TUNNELS]);

With max_tunnels <= 16, nr_tunnels reaches max_tunnels before per_src can
reach AMT_MAX_TUNNELS_PER_SRC. One real source address can then send
Requests and Updates from 16 UDP ports and take every slot.

Once the table is full, amt_request_handler() withholds the Query from
every new endpoint:

	if (!have && (full || per_src >= AMT_MAX_TUNNELS_PER_SRC))
		return true;

Legitimate gateways then never get a MAC or a tunnel. To keep its slots,
the attacker only needs to send an Update within each 3 * GMI. This
contradicts the comment above, which says the cap "stops one verified
source address from monopolising the table".

The cap also doesn't scale with the table size: two addresses can fill a
32-slot table, and eight can fill the default 128.

The commit message defers a configurable limit because that needs new
uAPI. Could the cap be derived from max_tunnels instead, for example
min(16, max(1, max_tunnels / 8)), which needs no uAPI?

[ ... ]

> @@ -2841,59 +2988,41 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	if (amtrh->reserved1 || amtrh->reserved2 || amtrh->version)
>  		return true;
>  

[ ... ]

> +	/* Snapshot the secret and limits under the lock; keep the endpoint walk
> +	 * out of the lock so spoofed Requests cannot serialize relay updates.
> +	 */
> +	spin_lock_bh(&amt->lock);
> +	key = amt->key[0];
> +	full = amt->nr_tunnels >= amt->max_tunnels;
>  	spin_unlock_bh(&amt->lock);

[Severity: Low]
Does this need amt->lock? Every Request that passes the header checks,
spoofed ones included, now takes the device-wide lock. Before the patch,
a Request from an address that already had a tunnel took no lock at all.

The same patch adds amt->key_seq, documented as "For lockless readers of
key", and uses it in amt_update_handler(). The full value is only
advisory, because amt_tunnel_get_or_create() checks the limit again under
the lock.

A key_seq read loop plus READ_ONCE(amt->nr_tunnels) would make this path
lockless. As written, a Request flood on many CPUs contends on amt->lock
with amt_tunnel_get_or_create(), amt_tunnel_expire() and
amt_secret_work(). That works against the comment's goal of keeping
spoofed Requests from serializing relay updates.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105125010.1107546-1-omar%40blockcast.net