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

[Omar Ramadan]

On Mon, 09 Nov 2026 14:49:21 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> - [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.

v6 has superseded this posting; this patch is v6 6/6, "amt: create
relay tunnels only from authenticated Updates":
https://lore.kernel.org/netdev/20261107004935.2627329-7-omar@xxxxxxxxxxxxx/

Both Medium findings are real, and v6 does not address them. The v7 I am
about to post does.

> Should the changelog describe a few more behaviour changes?

Yes. v7's changelog states all three:

- Key seeding: b9022b53adad seeded the secret after amt_socket_create(),
  when there was no receive path yet, and cbc21dc1cfe9 added amt_rcv()
  and the per-tunnel copy of the key without moving it. Since then a
  Request arriving during amt_dev_open() could get a tunnel whose MAC
  used an all-zero key (first open) or the previous key, for the
  tunnel's whole life. The existing Fixes: cbc21dc1cfe9 covers it.

- Expiry: a MAC is now accepted for 180 to 240 seconds after the Query
  that carried it (60 second rotation, four generations kept). RFC 7450
  5.2.3.5.4 has the gateway start a query timer it SHOULD compute from
  QQIC (125 seconds here), and 5.2.3.5.5 has it send a new Request when
  that fires; the in-tree gateway re-Requests every two seconds while
  its Requests are answered. A gateway that never re-Requests worked
  indefinitely before and now has its Updates dropped as Invalid MAC.
  So is the first Update after a new join from a gateway that, as
  5.2.3.5.4 also allows, runs a longer timer while it has no
  subscriptions: it is dropped until that gateway's next Request.

- Replay: any (nonce, MAC) pair minted for the address and port within
  that window is accepted, not only the latest Request's. The MAC never
  covered the encapsulated report, so this changes only how long a pair
  seen by an on-path observer stays usable; off-path senders still
  cannot produce one.

> Could the cap be derived from max_tunnels instead, for example
> min(16, max(1, max_tunnels / 8)), which needs no uAPI?

Yes, v7 does exactly that, at both the Request and the Update sites:

	static u32 amt_max_tunnels_per_src(const struct amt_dev *amt)
	{
		return clamp_t(u32, amt->max_tunnels / 8, 1,
			       AMT_MAX_TUNNELS_PER_SRC);
	}

That is 16 at the default 128 slots and 2 at 16. Below 16 slots it is
1, so there only one endpoint per address gets a tunnel. A configurable
limit stays net-next material.

> Is "teardown" accurate in the subject?

Fixed in v6: the subject is above, and the amt.h comment churn is gone.
The netif_running() recheck under amt->lock in
amt_tunnel_get_or_create() was still undocumented in v6; v7's changelog
explains it: dev_close() clears the running state before amt_dev_stop()
drains the list under amt->lock, so an Update still being received
cannot link a tunnel after the drain.

> Do these two comments hold on the renewal path?

Fixed in v6. The comments now describe what the code enforces ("no
tunnel state is created or changed", "No per-source state is read or
created"), which holds on the renewal path too.

> Does this need amt->lock?

No, but v7 keeps it. The full check is advisory, since
amt_tunnel_get_or_create() repeats it under the lock, and the lock is
held only for an O(1) snapshot, so a lockless read here would save a
lock round trip per Request. That is an optimization rather than a fix,
so I'm leaving it for net-next. v6 already reworded the comment so it
no longer claims more than that ("a Request holds amt->lock only for
this O(1) snapshot"), and v7 keeps it.