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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] amt: tunnel allocated before inner IGMP/MLD check in
  amt_update_handler()
- [Medium] amt: race with amt_tunnel_expire() drops valid Update in
  amt_update_handler()
- [Low] amt: redundant amt_update_relay_status() call in
  amt_update_handler()
--

commit f8d7b3a5dca8e65b55fc968b3708aa21bb6f8353
Author: Omar Ramadan <omar@blockcast.net>

amt: harden relay authentication and teardown

This commit makes AMT Request handling stateless by computing the Query MAC
from the endpoint address, port, nonce, and secret generations without
allocating tunnel state. Tunnel allocation is deferred to Membership Update
processing after MAC verification, keyed by source IP and source port with
global and per-source limits.

> 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

[ ... ]

> @@ -2586,16 +2610,107 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
>  	return false;
>  }
>  
> +/* The tunnel of the gateway endpoint @saddr:@sport, or NULL. */
> +static struct amt_tunnel_list *amt_tunnel_lookup(struct amt_dev *amt,
> +						 __be32 saddr, __be16 sport)
> +{
> +	struct amt_tunnel_list *tunnel;
> +
> +	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
> +		if (tunnel->ip4 == saddr && tunnel->source_port == sport)
> +			return tunnel;
> +	return NULL;
> +}
> +
> +/* 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.
> + */
> +static struct amt_tunnel_list *amt_tunnel_get_or_create(struct amt_dev *amt,
> +							__be32 saddr,
> +							__be16 sport)
> +{
> +	struct amt_tunnel_list *tunnel;
> +	int per_src = 0;
> +	int i;
> +
> +	tunnel = amt_tunnel_lookup(amt, saddr, sport);
> +	if (tunnel)
> +		return tunnel;

[Severity: Medium]
What happens in drivers/net/amt.c when amt_update_handler() calls
amt_tunnel_get_or_create() concurrently with amt_tunnel_expire() expiring the
same tunnel?

In amt_tunnel_expire(), the work handler acquires amt->lock, checks
tunnel->last_seen against jiffies, marks the tunnel dead under tunnel->lock,
and removes it from amt->tunnel_list:

drivers/net/amt.c:amt_tunnel_expire() {
	spin_lock_bh(&amt->lock);
	...
	deadline = READ_ONCE(tunnel->last_seen);
	...
	if (time_before(jiffies, deadline)) {
		mod_delayed_work(amt_wq, &tunnel->gc_wq, deadline - jiffies);
		spin_unlock_bh(&amt->lock);
		return;
	}
	spin_lock_bh(&tunnel->lock);
	WRITE_ONCE(tunnel->dead, true);
	spin_unlock_bh(&tunnel->lock);
	rcu_read_lock();
	list_del_rcu(&tunnel->list);
	...
}

Because amt_tunnel_lookup() walks amt->tunnel_list under RCU without holding
amt->lock or checking tunnel->dead, it can find and return a tunnel after
amt_tunnel_expire() has already passed the deadline check (and before
list_del_rcu() completes).

When that happens, amt_tunnel_get_or_create() returns the dying tunnel instead
of taking the amt->lock slow path to wait for removal and allocate a
replacement. amt_update_handler() then updates tunnel->last_seen too late to
stop expiry, sees READ_ONCE(tunnel->dead) under tunnel->lock, and drops the
valid Membership Update:

drivers/net/amt.c:amt_update_handler() {
	...
		spin_lock_bh(&tunnel->lock);
		if (READ_ONCE(tunnel->dead)) {
			spin_unlock_bh(&tunnel->lock);
			return true;
		}
}

Should amt_tunnel_lookup() or the caller handle a concurrently expiring tunnel
so the valid Membership Update is not dropped?

> +
> +	spin_lock_bh(&amt->lock);
> +
> +	/* Re-check under the lock in case a concurrent Update won the race, and
> +	 * count how many tunnels this source already holds on the same walk.
> +	 */
> +	list_for_each_entry(tunnel, &amt->tunnel_list, list) {
> +		if (tunnel->ip4 != saddr)
> +			continue;
> +		if (tunnel->source_port == sport) {
> +			spin_unlock_bh(&amt->lock);
> +			return tunnel;
> +		}
> +		per_src++;
> +	}
> +
> +	if (amt->nr_tunnels >= amt->max_tunnels ||
> +	    per_src >= AMT_MAX_TUNNELS_PER_SRC) {
> +		spin_unlock_bh(&amt->lock);
> +		return NULL;
> +	}
> +
> +	if (!netif_running(amt->dev)) {
> +		spin_unlock_bh(&amt->lock);
> +		return NULL;
> +	}
> +
> +	tunnel = kzalloc(sizeof(*tunnel) +
> +			 (sizeof(struct hlist_head) * amt->hash_buckets),
> +			 GFP_ATOMIC);
> +	if (!tunnel) {
> +		spin_unlock_bh(&amt->lock);
> +		return NULL;
> +	}
> +
> +	tunnel->source_port = sport;
> +	tunnel->ip4 = saddr;
> +	tunnel->amt = amt;
> +	spin_lock_init(&tunnel->lock);
> +	for (i = 0; i < amt->hash_buckets; i++)
> +		INIT_HLIST_HEAD(&tunnel->groups[i]);
> +
> +	tunnel->last_seen = jiffies;
> +	INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
> +	__amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_UPDATE, false);

[Severity: Low]
In drivers/net/amt.c, amt_tunnel_get_or_create() initializes tunnel->status to
AMT_STATUS_RECEIVED_UPDATE here via __amt_update_relay_status() with validate
set to false.

Later in amt_update_handler(), when __netif_rx(skb) succeeds, the code calls
amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_UPDATE, true):

drivers/net/amt.c:amt_update_handler() {
	...
	if (__netif_rx(skb) == NET_RX_SUCCESS) {
		amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_UPDATE,
					true);
		dev_sw_netstats_rx_add(amt->dev, len);
	}
}

Since __amt_update_relay_status() returns early when validate is true and
tunnel->status >= status:

drivers/net/amt.c:__amt_update_relay_status() {
	if (validate && tunnel->status >= status)
		return;
	...
}

and tunnel->status is already AMT_STATUS_RECEIVED_UPDATE from creation, does
the amt_update_relay_status() call in amt_update_handler() just take and drop
tunnel->lock on every Membership Update as a no-op?

> +
> +	/* Publish only once the entry is fully initialised. */
> +	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
> +	amt->nr_tunnels++;
> +	mod_delayed_work(amt_wq, &tunnel->gc_wq,
> +			 msecs_to_jiffies(amt_gmi(amt)) * 3);
> +	spin_unlock_bh(&amt->lock);
> +
> +	return tunnel;
> +}

[ ... ]

> @@ -2609,34 +2724,52 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)

[ ... ]

> +	if (!verified) {
> +		netdev_dbg(amt->dev, "Invalid MAC\n");
> +		return true;
> +	}
> +
> +	tunnel = amt_tunnel_get_or_create(amt, saddr, sport);
> +	if (!tunnel)
> +		return true;
> +
> +	/* Refresh liveness only.  gc_wq is re-armed by amt_tunnel_expire()
> +	 * itself from last_seen, never from here, so this cannot re-queue the
> +	 * work of a tunnel that is being freed.
> +	 */
> +	WRITE_ONCE(tunnel->last_seen, jiffies);
> +
> +	if (iptunnel_pull_header(skb, hdr_size, skb->protocol, false))
> +		return true;
>  
> -	return true;
> +	skb_reset_network_header(skb);
>  
> -report:
>  	if (!pskb_may_pull(skb, sizeof(*iph)))
>  		return true;

[Severity: Medium]
In drivers/net/amt.c, amt_update_handler() calls amt_tunnel_get_or_create()
and updates tunnel->last_seen before decapsulating the packet and validating
the inner IGMP or MLD report:

drivers/net/amt.c:amt_update_handler() {
	...
	if (!pskb_may_pull(skb, sizeof(*iph)))
		return true;

	if (skb_cow_head(skb, 0))
		return true;

	iph = ip_hdr(skb);
	if (iph->version == 4) {
		if (ip_mc_check_igmp(skb)) {
			netdev_dbg(amt->dev, "Invalid IGMP\n");
			return true;
		}
	...
	} else if (iph->version == 6) {
		struct ipv6hdr *ip6h = ipv6_hdr(skb);

		if (ipv6_mc_check_mld(skb)) {
			netdev_dbg(amt->dev, "Invalid MLD\n");
			return true;
		}
	...
	} else {
		netdev_dbg(amt->dev, "Unsupported Protocol\n");
		return true;
	}
}

If a remote host completes a Request/Query exchange and sends a Membership
Update with a valid outer MAC, but the inner packet is truncated, uses an
unsupported protocol, or fails ip_mc_check_igmp() or ipv6_mc_check_mld(),
doesn't this still allocate a new tunnel entry in amt->tunnel_list (or refresh
tunnel->last_seen on an existing tunnel) before dropping the packet?

Since saddr and sport are already snapshotted before iptunnel_pull_header(),
could amt_tunnel_get_or_create() and the tunnel->last_seen update be deferred
until after ip_mc_check_igmp() or ipv6_mc_check_mld() succeeds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105125010.1107546-1-omar@blockcast.net?part=5