Re: [PATCH net v2 3/3] amt: do not create tunnel state for unauthenticated Requests

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · Medium: 4 · Low: 2

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] Response MACs now expire after 60–120 s.
- [Medium] When the table is full, the relay now refuses only after
  sending a Query.
- [Medium] Every Membership Update rejected for a bad MAC is counted
  twice in rx_dropped.
- [Medium] At device open, packets can be checked against MAC keys that
  are still zero, which lets an attacker create tunnels for spoofed…
- [Low] Parts of the commit message are wrong for the tree this patch
  applies to, and one behaviour change is not mentioned.
- [Low] The selftest's explanation is now out of date.

Pre-existing issues:
- [High] Use-after-free: an Update can re-arm gc_wq on a tunnel that
  amt_tunnel_expire() is unlinking and freeing.
- [High] This problem wasn't introduced by this patch, but amt_dev_stop()
  can unlink and free a tunnel at the same time as that tunnel's…

--

amt: do not create tunnel state for unauthenticated Requests

With this commit, amt_request_handler() no longer allocates tunnel state
for unauthenticated Relay Membership Requests. On Request the relay
computes the response MAC without storing anything. On Update it
recomputes the MAC, accepting either the current or the previous secret.
A tunnel is created only after that check passes.

> Note for reviewers: the MAC covers the UDP source port, so a NAT
> rebinding between Request and Update now changes the recomputed MAC where
> the stored value would have survived it.

[Severity: Low]
Is this accurate for the tree this applies to? In the parent, the Update
lookup already required a matching port:

-		if (tunnel->ip4 == saddr &&
-		    tunnel->source_port == sport) {

The stored MAC was also already computed over tunnel->source_port. So an
Update from a rebound port already matched no tunnel and was dropped.
That behaviour came from "amt: key relay tunnel state on the (address,
port) endpoint, not the address", not from this patch.

> Once full, every further
> Request also emits an ICMP_DEST_UNREACH to the spoofed source, turning
> the relay into an ICMP reflector.

The ICMP reflector is listed as one of the problems being fixed. However,
amt_request_handler() still sends an unsolicited Membership Query to
every claimed source. Only the type of the reply has changed.

The commit message also doesn't say that "relay full" is now reported on
the Update path rather than the Request path. A refused gateway now
always gets a Query, becomes ready, and then gets ICMP host unreachable
for each Update.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 492d450fdb55c..bc8e552a9bad4 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -925,7 +934,8 @@ static void amt_secret_work(struct work_struct *work)
>  					   secret_wq);
>  
>  	spin_lock_bh(&amt->lock);
> -	get_random_bytes(&amt->key, sizeof(siphash_key_t));
> +	amt->key[1] = amt->key[0];
> +	get_random_bytes(&amt->key[0], sizeof(siphash_key_t));
>  	spin_unlock_bh(&amt->lock);
>  	mod_delayed_work(amt_wq, &amt->secret_wq,
>  			 msecs_to_jiffies(AMT_SECRET_TIMEOUT));

[Severity: Medium]
Does this give every response MAC a fixed lifetime that is shorter than
the Query Interval the relay advertises?

amt_secret_work() runs every AMT_SECRET_TIMEOUT (60000 ms) and only one
previous key is kept. A MAC issued at time t therefore stops verifying in
amt_update_handler() at the second rotation after t, which is 60 to 120
seconds later.

Before this patch, each tunnel copied amt->key when it was created, and
that copy never rotated. A gateway's (nonce, MAC) stayed valid for the
whole life of the tunnel.

The relay advertises QQIC = amt->qi (AMT_INIT_QUERY_INTERVAL, 125) in
amt_build_igmp_gq() and amt_build_mld_gq(). Gateways reuse the cached
nonce and MAC from the last Query for every Update, including unsolicited
joins and leaves:

drivers/net/amt.c:amt_send_membership_update() {
    ...
	amtmu->nonce		= amt->nonce;
	amtmu->response_mac	= amt->mac;
    ...
}

Take a third-party gateway that paces its Requests by the advertised
125 second interval. Its joins and leaves would be dropped as "Invalid
MAC" during the last 5 to 65 seconds before each refresh. A lost join
means no multicast until the next cycle. A lost leave means the relay
keeps forwarding until gc.

The Linux gateway in this file sends a new Request about every 2 seconds
(amt_event_send_request() with req_cnt reset by each Query), so it is
not affected.

The commit message describes the two-key window only as covering "an
exchange straddling a rotation". Should it mention this lifetime limit?
Alternatively, should the rotation period or the number of kept keys be
tied to amt->qi?

> @@ -1115,9 +1125,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]
This isn't a bug, but the comment above test_query_egress() in
tools/testing/selftests/net/amt.sh still describes the Query as tied to
a tunnel:

# The relay sends its General Queries straight from the receive path, in
# the same context that found the tunnel. A query queued on the amt device
# instead could outlive the tunnel it was built for.

After this change amt_request_handler() neither looks up nor creates a
tunnel. Should the selftest comment be updated to match?

>  static bool amt_send_membership_query(struct amt_dev *amt,
>  				      struct sk_buff *skb,
> -				      struct amt_tunnel_list *tunnel,
> +				      __be32 daddr, __be16 dport,
> +				      __be32 nonce, u64 mac,
>  				      bool v6)

[ ... ]

> @@ -2496,36 +2573,58 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(key); i++) {
> +		u64 mac = siphash_3u32((__force u32)saddr,
> +				       (__force u32)sport,
> +				       (__force u32)nonce,
> +				       &key[i]) >> 16;

[ ... ]

> +		if (response_mac == mac) {
> +			verified = true;
> +			break;
>  		}
>  	}
>  
> -	return true;
> +	if (!verified) {
> +		/* Count the drop: an unauthenticated Update is not observable
> +		 * from the gateway's own side.
> +		 */
> +		netdev_dbg(amt->dev, "Invalid MAC\n");
> +		amt->dev->stats.rx_dropped++;
> +		return true;
> +	}

[Severity: Medium]
Is this packet counted twice in rx_dropped?

amt_update_handler() increments rx_dropped and returns true. amt_rcv()
then takes its drop path for the same skb:

drivers/net/amt.c:amt_rcv() {
    ...
		case AMT_MSG_MEMBERSHIP_UPDATE:
			err = amt_update_handler(amt, skb);
			if (err)
				goto drop;
    ...
drop:
	if (err) {
		amt->dev->stats.rx_dropped++;
		kfree_skb(skb);
    ...
}

The extra increment came in with "amt: key relay tunnel state on the
(address, port) endpoint, not the address". This patch moves it but
keeps it, so every spoofed or stale-MAC Update raises rx_dropped by 2.

> +
> +	tunnel = amt_tunnel_get_or_create(amt, saddr, sport);
> +	if (!tunnel) {
> +		/* Out of tunnel slots.  Unlike the Request path this reply
> +		 * only ever goes to a source that has proved it received our
> +		 * Query, so it cannot be used to reflect at a third party.
> +		 */
> +		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
> +		return true;
> +	}

[Severity: Medium]
When the table is full, does this stop the gateway from ever falling back
to rediscovery?

amt_request_handler() now answers every Request with a Query, whatever
the capacity, and the refusal only happens here. With the gateway code in
this file, the sequence becomes:

gateway: amt_membership_query_handler()
    amt->req_cnt = 0;  (ready4/ready6 set to true)
gateway sends Update
relay: amt_update_handler() -> icmp_ndo_send(ICMP_HOST_UNREACH)
gateway: amt_err_lookup()
    mod_delayed_work(amt_wq, &amt->req_wq, 0);
gateway: amt_event_send_request() sees req_cnt == 0, sends a new Request
relay answers with another Query, and req_cnt is reset again

As a result, this give-up branch in amt_event_send_request() can no
longer be reached:

	if (amt->req_cnt > AMT_MAX_REQ_COUNT) {
		...
		WRITE_ONCE(amt->remote_ip, 0);
		amt_update_gw_status(amt, AMT_STATUS_INIT, false);

Before this patch, a full relay answered the Request with ICMP and sent
no Query. req_cnt kept climbing, and the gateway went back to discovery,
where an anycast discovery address could pick another relay.

Now the gateway keeps cycling through Request, Query, Update and ICMP
with a relay that will never admit it, and it reports ready4/ready6 as
true the whole time. Is this change in behaviour intended?

> +
> +	mod_delayed_work(amt_wq, &tunnel->gc_wq,
> +			 msecs_to_jiffies(amt_gmi(amt)) * 3);

[Severity: High]
This is a pre-existing issue, but can this re-arm gc_wq on a tunnel that
amt_tunnel_expire() is in the middle of freeing?

amt_tunnel_get_or_create() returns an existing tunnel either from the
lockless walk or from the re-check after amt->lock is dropped. Here the
only protection is rcu_read_lock_bh() from amt_rcv(). Suppose the
tunnel's gc_wq has already started running:

CPU1 (amt_wq)
amt_tunnel_expire()
    spin_lock_bh(&amt->lock);
    list_del_rcu(&tunnel->list);
    amt->nr_tunnels--;
    amt_clear_groups(tunnel);
    spin_unlock_bh(&amt->lock);
    kfree_rcu(tunnel, rcu);

CPU2 (amt_rcv)
amt_update_handler()
    tunnel = amt_tunnel_get_or_create(...);  /* same tunnel */
    mod_delayed_work(amt_wq, &tunnel->gc_wq, ...);
    amt_igmp_report_handler(amt, skb, tunnel);

The work is running rather than pending, so mod_delayed_work() queues it
again and arms a timer inside an object that is waiting for kfree_rcu().
The report handler also attaches new group and source nodes after
amt_clear_groups() has already run.

When that timer fires, amt_tunnel_expire() runs on freed memory. It calls
list_del_rcu() on a poisoned entry and decrements nr_tunnels a second
time, which underflows it and bypasses max_tunnels. The orphaned group
nodes leak, with timers that point at the freed tunnel.

The lookup has now moved into a new helper. Would it be worth checking
under amt->lock that the tunnel is still on the list (or not marked dead)
before gc_wq is re-armed?

[ ... ]

> @@ -3061,7 +3128,7 @@ static int amt_dev_open(struct net_device *dev)
>  	amt->req_cnt = 0;
>  	WRITE_ONCE(amt->remote_ip, 0);
>  	amt->nonce = 0;
> -	get_random_bytes(&amt->key, sizeof(siphash_key_t));
> +	get_random_bytes(&amt->key, sizeof(amt->key));

[Severity: Medium]
Can Updates be verified against all-zero keys while the device is being
opened?

amt_socket_create() -> setup_udp_tunnel_sock() publishes sk_user_data,
installs encap_rcv = amt_rcv and enables encap before it returns. The
keys are seeded only after that, and without amt->lock:

amt_dev_open()
    err = amt_socket_create(amt);
    ...
    get_random_bytes(&amt->key, sizeof(amt->key));

On the first bring-up the netdev private data is zero-filled, so key[0]
and key[1] are both zero during that window.

With this patch, the only check before admitting a tunnel is recomputing
siphash_3u32(saddr, sport, nonce, key) >> 16. A spoofed Update carrying a
MAC computed offline under the zero key would pass amt_update_handler()
and get a tunnel, plus group joins that forward multicast to the spoofed
address. That entry stays after the keys are randomized.

amt_request_handler(), amt_update_handler() and amt_secret_work() all
access amt->key[] under amt->lock, so readers here can also see a
half-written key. Should the keys be seeded under amt->lock before
amt_socket_create() is called?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch.
While looking at tunnel lifetime, though, can amt_dev_stop() double free
a tunnel whose amt_tunnel_expire() is running at the same time?

drivers/net/amt.c:amt_dev_stop() {
    ...
	list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
		list_del_rcu(&tunnel->list);
		amt->nr_tunnels--;
		cancel_delayed_work_sync(&tunnel->gc_wq);
		amt_clear_groups(tunnel);
		kfree_rcu(tunnel, rcu);
	}
    ...
}

amt->lock is not held here, and the tunnel is unlinked before gc_wq is
cancelled.

Suppose amt_tunnel_expire() is already running for that tunnel. It takes
amt->lock and calls list_del_rcu() on an entry that dev_stop has already
unlinked, which writes through LIST_POISON2.

cancel_delayed_work_sync() then waits for expire, which calls
kfree_rcu(tunnel), and dev_stop calls kfree_rcu(tunnel) a second time.
nr_tunnels is also decremented twice.

>  
>  	amt->status = AMT_STATUS_INIT;
>  	if (amt->mode == AMT_MODE_GATEWAY) {

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