* [PATCH net v5 1/5] amt: fix use-after-free when a Membership Update re-arms a freed tunnel
2026-11-05 12:50 [PATCH net v5 0/5] amt: relay tunnel lifetime, validation and request-path hardening Omar Ramadan
@ 2026-11-05 12:50 ` Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 2/5] amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned Omar Ramadan
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Omar Ramadan @ 2026-11-05 12:50 UTC (permalink / raw)
To: Taehee Yoo, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Simon Horman, Sebastian Andrzej Siewior,
Clark Williams, Steven Rostedt, linux-rt-devel
amt_update_handler() refreshes a relay tunnel's garbage-collection timer
with mod_delayed_work() on a tunnel found by an RCU walk. A concurrent
amt_tunnel_expire() can unlink and free that tunnel while the Update path
still has the cached pointer, leaving gc_wq armed in freed memory.
Make amt_tunnel_expire() the only timeout-path freer. The Update path
records last_seen, and expire rechecks it under amt->lock before either
rearming or freeing the tunnel. Protect the dead state with tunnel->lock so
report handling cannot add multicast state after teardown begins. Device
stop marks tunnels dead, unlinks them with list_del_rcu(), synchronously
cancels their work, and frees them after lockless readers have passed.
Apply the same lifetime rules to group and source timer work: mark nodes
dead under tunnel->lock, keep RCU held while workers resolve their
owners, defer node freeing, and synchronously cancel each timer before
freeing its owner. Flush the deferred GC before freeing a tunnel so
nodes are reclaimed promptly; flush rather than cancel so periodic GC
keeps running. Lifetime does not rest on the flush, which can return
while a GC run that already spliced its list is in flight: it rests on
kfree_rcu() and on the owner pointers being cleared, and the dead checks
made, under tunnel->lock. Release the group timer's device reference on
every exit. A Request-only tunnel retains the original GMI timeout; only
an authenticated Membership Update refreshes last_seen and uses the
longer lifetime.
The Sashiko review of v2 of this series pointed out the gc_wq re-arm,
and reading amt_update_handler() against amt_tunnel_expire() and
amt_dev_stop() confirms it. It is theoretical: it needs a Membership
Update that has already found the tunnel under RCU to arrive while
amt_tunnel_expire() is freeing that tunnel. I have not triggered it,
so there is no trace to include. Review of v3 and v4 raised the races
in the group and source timer work.
Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
drivers/net/amt.c | 201 +++++++++++++++++++++++++++++++++++++++-------
include/net/amt.h | 16 ++++
2 files changed, 189 insertions(+), 28 deletions(-)
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 81d26ef82..d8914d811 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -38,6 +38,10 @@ static HLIST_HEAD(source_gc_list);
/* Lock for source_gc_list */
static spinlock_t source_gc_lock;
static struct delayed_work source_gc_wq;
+static HLIST_HEAD(group_gc_list);
+/* Lock for group_gc_list */
+static spinlock_t group_gc_lock;
+static struct delayed_work group_gc_wq;
static char *status_str[] = {
"AMT_STATUS_INIT",
"AMT_STATUS_SENT_DISCOVERY",
@@ -90,8 +94,9 @@ static void __amt_source_gc_work(void)
hlist_move_list(&source_gc_list, &gc_list);
spin_unlock_bh(&source_gc_lock);
- hlist_for_each_entry_safe(snode, t, &gc_list, node) {
- hlist_del_rcu(&snode->node);
+ hlist_for_each_entry_safe(snode, t, &gc_list, gc_node) {
+ cancel_delayed_work_sync(&snode->source_timer);
+ hlist_del_init(&snode->gc_node);
kfree_rcu(snode, rcu);
}
}
@@ -106,6 +111,42 @@ static void amt_source_gc_work(struct work_struct *work)
spin_unlock_bh(&source_gc_lock);
}
+static void __amt_group_gc_work(void)
+{
+ struct amt_group_node *gnode;
+ struct hlist_head gc_list;
+ struct hlist_node *t;
+
+ spin_lock_bh(&group_gc_lock);
+ hlist_move_list(&group_gc_list, &gc_list);
+ spin_unlock_bh(&group_gc_lock);
+
+ /* Group deletion queues all of its sources first. Flush that queue
+ * so they are reclaimed promptly; a source callback cannot reach a
+ * freed group because it resolves snode->gnode under RCU and the
+ * pointer is cleared before the source is queued. Flush rather than
+ * cancel: source_gc_wq re-arms only itself, so cancelling it here
+ * would stop periodic source GC for good.
+ */
+ flush_delayed_work(&source_gc_wq);
+
+ hlist_for_each_entry_safe(gnode, t, &gc_list, gc_node) {
+ cancel_delayed_work_sync(&gnode->group_timer);
+ hlist_del_init(&gnode->gc_node);
+ kfree_rcu(gnode, rcu);
+ }
+}
+
+static void amt_group_gc_work(struct work_struct *work)
+{
+ __amt_group_gc_work();
+
+ spin_lock_bh(&group_gc_lock);
+ mod_delayed_work(amt_wq, &group_gc_wq,
+ msecs_to_jiffies(AMT_GC_INTERVAL));
+ spin_unlock_bh(&group_gc_lock);
+}
+
static bool amt_addr_equal(union amt_addr *a, union amt_addr *b)
{
return !memcmp(a, b, sizeof(union amt_addr));
@@ -215,9 +256,14 @@ static struct amt_group_node *amt_lookup_group(struct amt_tunnel_list *tunnel,
static void amt_destroy_source(struct amt_source_node *snode)
{
- struct amt_group_node *gnode = snode->gnode;
+ struct amt_group_node *gnode;
struct amt_tunnel_list *tunnel;
+ if (READ_ONCE(snode->dead))
+ return;
+ WRITE_ONCE(snode->dead, true);
+ gnode = snode->gnode;
+
tunnel = gnode->tunnel_list;
if (!gnode->v6) {
@@ -238,8 +284,9 @@ static void amt_destroy_source(struct amt_source_node *snode)
hlist_del_init_rcu(&snode->node);
tunnel->nr_sources--;
gnode->nr_sources--;
+ WRITE_ONCE(snode->gnode, NULL);
spin_lock_bh(&source_gc_lock);
- hlist_add_head_rcu(&snode->node, &source_gc_list);
+ hlist_add_head(&snode->gc_node, &source_gc_list);
spin_unlock_bh(&source_gc_lock);
}
@@ -249,6 +296,9 @@ static void amt_del_group(struct amt_dev *amt, struct amt_group_node *gnode)
struct hlist_node *t;
int i;
+ if (READ_ONCE(gnode->dead))
+ return;
+ WRITE_ONCE(gnode->dead, true);
if (cancel_delayed_work(&gnode->group_timer))
dev_put(amt->dev);
hlist_del_rcu(&gnode->node);
@@ -266,10 +316,10 @@ static void amt_del_group(struct amt_dev *amt, struct amt_group_node *gnode)
hlist_for_each_entry_safe(snode, t, &gnode->sources[i], node)
amt_destroy_source(snode);
- /* tunnel->lock was acquired outside of amt_del_group()
- * But rcu_read_lock() was acquired too so It's safe.
- */
- kfree_rcu(gnode, rcu);
+ WRITE_ONCE(gnode->tunnel_list, NULL);
+ spin_lock_bh(&group_gc_lock);
+ hlist_add_head(&gnode->gc_node, &group_gc_list);
+ spin_unlock_bh(&group_gc_lock);
}
/* If a source timer expires with a router filter-mode for the group of
@@ -282,13 +332,28 @@ static void amt_source_work(struct work_struct *work)
struct amt_source_node *snode = container_of(to_delayed_work(work),
struct amt_source_node,
source_timer);
- struct amt_group_node *gnode = snode->gnode;
- struct amt_dev *amt = gnode->amt;
+ struct amt_group_node *gnode;
+ struct amt_dev *amt;
struct amt_tunnel_list *tunnel;
- tunnel = gnode->tunnel_list;
- spin_lock_bh(&tunnel->lock);
rcu_read_lock();
+ gnode = READ_ONCE(snode->gnode);
+ if (!gnode) {
+ rcu_read_unlock();
+ return;
+ }
+ amt = READ_ONCE(gnode->amt);
+ tunnel = READ_ONCE(gnode->tunnel_list);
+ if (!tunnel) {
+ rcu_read_unlock();
+ return;
+ }
+ spin_lock_bh(&tunnel->lock);
+ if (READ_ONCE(snode->dead) || READ_ONCE(gnode->dead)) {
+ spin_unlock_bh(&tunnel->lock);
+ rcu_read_unlock();
+ return;
+ }
if (gnode->filter_mode == MCAST_INCLUDE) {
amt_destroy_source(snode);
if (!gnode->nr_sources)
@@ -299,8 +364,8 @@ static void amt_source_work(struct work_struct *work)
*/
snode->status = AMT_SOURCE_STATUS_D_FWD;
}
- rcu_read_unlock();
spin_unlock_bh(&tunnel->lock);
+ rcu_read_unlock();
}
static void amt_act_src(struct amt_tunnel_list *tunnel,
@@ -368,6 +433,7 @@ static struct amt_source_node *amt_alloc_snode(struct amt_group_node *gnode,
snode->status = AMT_SOURCE_STATUS_NONE;
snode->flags = AMT_SOURCE_NEW;
INIT_HLIST_NODE(&snode->node);
+ INIT_HLIST_NODE(&snode->gc_node);
INIT_DELAYED_WORK(&snode->source_timer, amt_source_work);
return snode;
@@ -403,26 +469,33 @@ static void amt_group_work(struct work_struct *work)
struct amt_group_node *gnode = container_of(to_delayed_work(work),
struct amt_group_node,
group_timer);
- struct amt_tunnel_list *tunnel = gnode->tunnel_list;
- struct amt_dev *amt = gnode->amt;
+ struct amt_tunnel_list *tunnel;
+ struct amt_dev *amt;
struct amt_source_node *snode;
bool delete_group = true;
struct hlist_node *t;
int i, buckets;
+ rcu_read_lock();
+ tunnel = READ_ONCE(gnode->tunnel_list);
+ amt = READ_ONCE(gnode->amt);
+ if (!tunnel)
+ goto out;
buckets = amt->hash_buckets;
spin_lock_bh(&tunnel->lock);
+ if (READ_ONCE(gnode->dead)) {
+ spin_unlock_bh(&tunnel->lock);
+ goto out;
+ }
if (gnode->filter_mode == MCAST_INCLUDE) {
/* Not Used */
spin_unlock_bh(&tunnel->lock);
goto out;
}
- rcu_read_lock();
- for (i = 0; i < buckets; i++) {
- hlist_for_each_entry_safe(snode, t,
- &gnode->sources[i], node) {
+ for (i = 0; i < buckets; i++)
+ hlist_for_each_entry_safe(snode, t, &gnode->sources[i], node) {
if (!delayed_work_pending(&snode->source_timer) ||
snode->status == AMT_SOURCE_STATUS_D_FWD) {
amt_destroy_source(snode);
@@ -431,14 +504,13 @@ static void amt_group_work(struct work_struct *work)
snode->status = AMT_SOURCE_STATUS_FWD;
}
}
- }
if (delete_group)
amt_del_group(amt, gnode);
else
gnode->filter_mode = MCAST_INCLUDE;
- rcu_read_unlock();
spin_unlock_bh(&tunnel->lock);
out:
+ rcu_read_unlock();
dev_put(amt->dev);
}
@@ -479,6 +551,7 @@ static struct amt_group_node *amt_add_group(struct amt_dev *amt,
gnode->tunnel_list = tunnel;
gnode->filter_mode = MCAST_INCLUDE;
INIT_HLIST_NODE(&gnode->node);
+ INIT_HLIST_NODE(&gnode->gc_node);
INIT_DELAYED_WORK(&gnode->group_timer, amt_group_work);
for (i = 0; i < amt->hash_buckets; i++)
INIT_HLIST_HEAD(&gnode->sources[i]);
@@ -1342,14 +1415,40 @@ static void amt_tunnel_expire(struct work_struct *work)
struct amt_tunnel_list,
gc_wq);
struct amt_dev *amt = tunnel->amt;
+ unsigned long deadline;
spin_lock_bh(&amt->lock);
+ /* amt_dev_stop() has taken ownership of the teardown. */
+ if (READ_ONCE(tunnel->dead)) {
+ spin_unlock_bh(&amt->lock);
+ return;
+ }
+ /* A Membership Update may have refreshed the tunnel after this work
+ * was queued. Re-arm from last_seen rather than letting
+ * amt_update_handler() re-arm gc_wq: this handler is then the only
+ * context that frees the tunnel on the timeout path; amt_dev_stop()
+ * still owns teardown during device shutdown.
+ */
+ deadline = READ_ONCE(tunnel->last_seen);
+ if (!deadline)
+ deadline = jiffies;
+ else
+ deadline += msecs_to_jiffies(amt_gmi(amt)) * 3;
+ 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);
amt->nr_tunnels--;
amt_clear_groups(tunnel);
rcu_read_unlock();
spin_unlock_bh(&amt->lock);
+ flush_delayed_work(&group_gc_wq);
kfree_rcu(tunnel, rcu);
}
@@ -2508,9 +2607,13 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
if (tunnel->ip4 == saddr) {
if ((nonce == tunnel->nonce &&
response_mac == tunnel->mac)) {
- mod_delayed_work(amt_wq, &tunnel->gc_wq,
- msecs_to_jiffies(amt_gmi(amt))
- * 3);
+ /* 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);
goto report;
} else {
netdev_dbg(amt->dev, "Invalid MAC\n");
@@ -2536,6 +2639,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
}
spin_lock_bh(&tunnel->lock);
+ if (READ_ONCE(tunnel->dead)) {
+ spin_unlock_bh(&tunnel->lock);
+ return true;
+ }
amt_igmp_report_handler(amt, skb, tunnel);
spin_unlock_bh(&tunnel->lock);
@@ -2556,6 +2663,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
}
spin_lock_bh(&tunnel->lock);
+ if (READ_ONCE(tunnel->dead)) {
+ spin_unlock_bh(&tunnel->lock);
+ return true;
+ }
amt_mld_report_handler(amt, skb, tunnel);
spin_unlock_bh(&tunnel->lock);
@@ -3064,7 +3175,7 @@ static int amt_dev_open(struct net_device *dev)
static int amt_dev_stop(struct net_device *dev)
{
struct amt_dev *amt = netdev_priv(dev);
- struct amt_tunnel_list *tunnel, *tmp;
+ struct amt_tunnel_list *tunnel;
struct sk_buff *skb;
struct sock *sk;
int i;
@@ -3093,11 +3204,37 @@ static int amt_dev_stop(struct net_device *dev)
amt->req_cnt = 0;
WRITE_ONCE(amt->remote_ip, 0);
- list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
- list_del_rcu(&tunnel->list);
- amt->nr_tunnels--;
+ /* Mark every tunnel dead first, so a concurrent amt_tunnel_expire()
+ * sees ->dead and bails instead of freeing a tunnel in parallel; this
+ * path is then the sole freer.
+ */
+ spin_lock_bh(&amt->lock);
+ list_for_each_entry(tunnel, &amt->tunnel_list, list) {
+ spin_lock_bh(&tunnel->lock);
+ WRITE_ONCE(tunnel->dead, true);
+ spin_unlock_bh(&tunnel->lock);
+ }
+ spin_unlock_bh(&amt->lock);
+
+ /* Unlink one tunnel at a time with list_del_rcu() rather than splicing
+ * the list onto a local head: a lockless reader walking tunnel_list
+ * must always terminate at &amt->tunnel_list, never at a stack head.
+ */
+ for (;;) {
+ spin_lock_bh(&amt->lock);
+ tunnel = list_first_entry_or_null(&amt->tunnel_list,
+ struct amt_tunnel_list, list);
+ if (tunnel) {
+ list_del_rcu(&tunnel->list);
+ amt->nr_tunnels--;
+ }
+ spin_unlock_bh(&amt->lock);
+ if (!tunnel)
+ break;
+
cancel_delayed_work_sync(&tunnel->gc_wq);
amt_clear_groups(tunnel);
+ flush_delayed_work(&group_gc_wq);
kfree_rcu(tunnel, rcu);
}
@@ -3475,11 +3612,17 @@ static int __init amt_init(void)
}
spin_lock_init(&source_gc_lock);
+ spin_lock_init(&group_gc_lock);
spin_lock_bh(&source_gc_lock);
INIT_DELAYED_WORK(&source_gc_wq, amt_source_gc_work);
mod_delayed_work(amt_wq, &source_gc_wq,
msecs_to_jiffies(AMT_GC_INTERVAL));
spin_unlock_bh(&source_gc_lock);
+ spin_lock_bh(&group_gc_lock);
+ INIT_DELAYED_WORK(&group_gc_wq, amt_group_gc_work);
+ mod_delayed_work(amt_wq, &group_gc_wq,
+ msecs_to_jiffies(AMT_GC_INTERVAL));
+ spin_unlock_bh(&group_gc_lock);
return 0;
@@ -3499,6 +3642,8 @@ static void __exit amt_fini(void)
unregister_netdevice_notifier(&amt_notifier_block);
cancel_delayed_work_sync(&source_gc_wq);
__amt_source_gc_work();
+ cancel_delayed_work_sync(&group_gc_wq);
+ __amt_group_gc_work();
destroy_workqueue(amt_wq);
}
module_exit(amt_fini);
diff --git a/include/net/amt.h b/include/net/amt.h
index 2846dde0c..123725c88 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -240,6 +240,16 @@ struct amt_tunnel_list {
u32 nr_sources;
enum amt_status status;
struct delayed_work gc_wq;
+ /* Last time this tunnel was refreshed. gc_wq
+ * re-arms itself from this rather than being re-armed by the Update
+ * path, so the Update path cannot re-queue the work of a tunnel that
+ * amt_tunnel_expire() is freeing.
+ */
+ unsigned long last_seen;
+ /* Set under amt->lock and tunnel->lock by whichever freer reaches the
+ * tunnel first; the other then leaves it alone, so it is freed once.
+ */
+ bool dead;
__be16 source_port;
__be32 ip4;
__be32 nonce;
@@ -275,6 +285,7 @@ enum amt_source_status {
/* protected by gnode->lock */
struct amt_source_node {
struct hlist_node node;
+ struct hlist_node gc_node;
struct amt_group_node *gnode;
struct delayed_work source_timer;
union amt_addr source_addr;
@@ -282,6 +293,8 @@ struct amt_source_node {
#define AMT_SOURCE_OLD 0
#define AMT_SOURCE_NEW 1
u8 flags;
+ /* Protected by gnode->tunnel_list->lock. */
+ bool dead;
struct rcu_head rcu;
};
@@ -295,7 +308,10 @@ struct amt_group_node {
u32 nr_sources;
struct amt_tunnel_list *tunnel_list;
struct hlist_node node;
+ struct hlist_node gc_node;
struct delayed_work group_timer;
+ /* Protected by tunnel_list->lock. */
+ bool dead;
struct rcu_head rcu;
struct hlist_head sources[];
};
--
2.47.3
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net v5 2/5] amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned
2026-11-05 12:50 [PATCH net v5 0/5] amt: relay tunnel lifetime, validation and request-path hardening Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 1/5] amt: fix use-after-free when a Membership Update re-arms a freed tunnel Omar Ramadan
@ 2026-11-05 12:50 ` Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 3/5] amt: require an exact IPv4-sized IFLA_AMT_{LOCAL,DISCOVERY}_IP Omar Ramadan
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Omar Ramadan @ 2026-11-05 12:50 UTC (permalink / raw)
To: Taehee Yoo, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Simon Horman, Sebastian Andrzej Siewior,
Clark Williams, Steven Rostedt, linux-rt-devel
A gateway's amt_rcv() admits a Membership Query or Multicast Data message
when "saddr != remote_ip" is false. remote_ip is 0 before discovery has
learned a relay (and is reset to 0 by amt_dev_open() and when discovery
gives up), so a packet whose source is 0.0.0.0 would otherwise pass the
equality check and could spoof a Query or inject multicast data.
Reject both when no relay is learned: a 0 remote_ip must match nothing, so
guard the check with !remote_ip. A Query is queued before its event worker
processes it, so the relay address can also change in that window. Recheck
the packet source against the current remote_ip in the handler and discard
stale Queries. Discovery is unaffected because Relay Advertisements are
validated against discovery_ip, not remote_ip.
Found by code inspection while writing the IPv6 receive path for the
AMT IPv6 outer transport; not triggered at runtime.
Fixes: b9022b53adad ("amt: add control plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
drivers/net/amt.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index d8914d811..dacfd5524 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2482,6 +2482,10 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
if (amtmq->reserved || amtmq->version)
return true;
+ /* A deferred query may outlive a relay address change. */
+ if (ip_hdr(skb)->saddr != READ_ONCE(amt->remote_ip))
+ return true;
+
if (amtmq->nonce != amt->nonce)
return true;
@@ -2958,7 +2962,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
}
goto out;
case AMT_MSG_MULTICAST_DATA:
- if (saddr != remote_ip) {
+ if (!remote_ip || saddr != remote_ip) {
netdev_dbg(amt->dev, "Invalid Relay IP\n");
err = true;
goto drop;
@@ -2969,7 +2973,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
else
goto out;
case AMT_MSG_MEMBERSHIP_QUERY:
- if (saddr != remote_ip) {
+ if (!remote_ip || saddr != remote_ip) {
netdev_dbg(amt->dev, "Invalid Relay IP\n");
err = true;
goto drop;
--
2.47.3
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net v5 3/5] amt: require an exact IPv4-sized IFLA_AMT_{LOCAL,DISCOVERY}_IP
2026-11-05 12:50 [PATCH net v5 0/5] amt: relay tunnel lifetime, validation and request-path hardening Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 1/5] amt: fix use-after-free when a Membership Update re-arms a freed tunnel Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 2/5] amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned Omar Ramadan
@ 2026-11-05 12:50 ` Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 4/5] amt: make pre-query report drops visible Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 5/5] amt: harden relay authentication and teardown Omar Ramadan
4 siblings, 0 replies; 6+ messages in thread
From: Omar Ramadan @ 2026-11-05 12:50 UTC (permalink / raw)
To: Taehee Yoo, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Simon Horman, Sebastian Andrzej Siewior,
Clark Williams, Steven Rostedt, linux-rt-devel
The netlink policy for IFLA_AMT_LOCAL_IP and IFLA_AMT_DISCOVERY_IP is an
untyped entry with .len set to the size of an IPv4 address, which netlink
validation treats as a minimum length only. Any longer payload passes,
and nla_get_in_addr() then reads just its first four bytes: an iproute2
that puts a 16-byte IPv6 literal in one of these IPv4 attributes turns
"2001:db8::" into 32.1.13.184, and the device is created with an address
the user never asked for.
Use NLA_POLICY_EXACT_LEN(sizeof(__be32)) for both, so every payload that
is not exactly four bytes long is refused with a standard extack instead
of being truncated. The range check this expands to fails the attribute
even under the liberal parsing rtnetlink applies to IFLA_INFO_DATA.
amt_get_size() and amt_fill_info() already emit exactly four bytes for
these attributes. IFLA_AMT_REMOTE_IP is only ever reported, never read
from a request, so it is left unchanged.
Triggered while writing a selftest for the AMT IPv6 outer transport:
an iproute2 without the IPv6 attributes put all 16 bytes of
"discovery 2001:db8:a::2" into IFLA_AMT_DISCOVERY_IP. On a stock
kernel the command exited 0 with no error or warning, and the device
came up as an IPv4 gateway with discovery address 32.1.13.184, the
first four bytes. There is no oops or trace; the symptom is the
silently wrong address, which fooled the selftest's capability probe.
Fixes: b9022b53adad ("amt: add control plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
drivers/net/amt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index dacfd5524..68f08f5f6 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -3307,9 +3307,9 @@ static const struct nla_policy amt_policy[IFLA_AMT_MAX + 1] = {
[IFLA_AMT_RELAY_PORT] = { .type = NLA_U16 },
[IFLA_AMT_GATEWAY_PORT] = { .type = NLA_U16 },
[IFLA_AMT_LINK] = { .type = NLA_U32 },
- [IFLA_AMT_LOCAL_IP] = { .len = sizeof_field(struct iphdr, daddr) },
+ [IFLA_AMT_LOCAL_IP] = NLA_POLICY_EXACT_LEN(sizeof(__be32)),
[IFLA_AMT_REMOTE_IP] = { .len = sizeof_field(struct iphdr, daddr) },
- [IFLA_AMT_DISCOVERY_IP] = { .len = sizeof_field(struct iphdr, daddr) },
+ [IFLA_AMT_DISCOVERY_IP] = NLA_POLICY_EXACT_LEN(sizeof(__be32)),
[IFLA_AMT_MAX_TUNNELS] = { .type = NLA_U32 },
};
--
2.47.3
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net v5 4/5] amt: make pre-query report drops visible
2026-11-05 12:50 [PATCH net v5 0/5] amt: relay tunnel lifetime, validation and request-path hardening Omar Ramadan
` (2 preceding siblings ...)
2026-11-05 12:50 ` [PATCH net v5 3/5] amt: require an exact IPv4-sized IFLA_AMT_{LOCAL,DISCOVERY}_IP Omar Ramadan
@ 2026-11-05 12:50 ` Omar Ramadan
2026-11-05 12:50 ` [PATCH net v5 5/5] amt: harden relay authentication and teardown Omar Ramadan
4 siblings, 0 replies; 6+ messages in thread
From: Omar Ramadan @ 2026-11-05 12:50 UTC (permalink / raw)
To: Taehee Yoo, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Simon Horman, Sebastian Andrzej Siewior,
Clark Williams, Steven Rostedt, linux-rt-devel
A gateway cannot forward an IGMP or MLD report until it has received the
relay's Membership Query for that family. The Query supplies the
response MAC the Update echoes, so dropping an early report is required,
but doing so silently leaves operators with a dark multicast path and no
indication why the join never happened.
Emit a family-specific debug message before taking the drop path.
tx_dropped could not show these drops either: amt_dev_xmit() reaches the
same tx_dropped increment after amt_send_membership_update() has handed
the report to the tunnel, so in gateway mode every forwarded report was
also counted as a drop. Return NETDEV_TX_OK directly once the report has
been sent, so tx_dropped counts only reports that were discarded.
The device is LLTX, so amt_dev_xmit() can run on several CPUs at once;
use DEV_STATS_INC() for its tx_dropped and tx_errors updates so
concurrent increments are not lost.
Found by code inspection while debugging an IPv6-in-IPv6 forwarding
failure in a selftest for the AMT IPv6 outer transport. This drop can
leave one address family dark, so it was a suspect, but that was not
confirmed: the failure was later traced to the selftest, which sent a
single datagram without waiting for the relay's multicast routes to
converge. The drop has not been observed in production, and there is
no oops or trace. Review of v3 found the tx_dropped miscount and the
non-atomic counter updates.
Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
drivers/net/amt.c | 42 +++++++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 68f08f5f6..26a942c2a 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -701,7 +701,7 @@ static void amt_send_discovery(struct amt_dev *amt)
IPPROTO_UDP, 0,
amt->stream_dev->ifindex);
if (IS_ERR(rt)) {
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
goto out;
}
@@ -711,7 +711,7 @@ static void amt_send_discovery(struct amt_dev *amt)
skb = netdev_alloc_skb_ip_align(amt->dev, len);
if (!skb) {
ip_rt_put(rt);
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
goto out;
}
@@ -756,7 +756,7 @@ static void amt_send_discovery(struct amt_dev *amt)
ip_send_check(iph);
err = ip_local_out(amt->net, sk, skb);
if (unlikely(net_xmit_eval(err)))
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
amt_update_gw_status(amt, AMT_STATUS_SENT_DISCOVERY, true);
out:
@@ -792,7 +792,7 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
IPPROTO_UDP, 0,
amt->stream_dev->ifindex);
if (IS_ERR(rt)) {
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
goto out;
}
@@ -802,7 +802,7 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
skb = netdev_alloc_skb_ip_align(amt->dev, len);
if (!skb) {
ip_rt_put(rt);
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
goto out;
}
@@ -849,7 +849,7 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
ip_send_check(iph);
err = ip_local_out(amt->net, sk, skb);
if (unlikely(net_xmit_eval(err)))
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
out:
rcu_read_unlock();
@@ -871,7 +871,7 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
skb_pull(skb, sizeof(struct ethhdr));
if (amt_send_membership_query(amt, skb, tunnel, false)) {
- amt->dev->stats.tx_dropped++;
+ DEV_STATS_INC(amt->dev, tx_dropped);
kfree_skb(skb);
}
}
@@ -915,7 +915,7 @@ static struct sk_buff *amt_build_mld_gq(struct amt_dev *amt)
if (ipv6_dev_get_saddr(amt->net, amt->dev, &ip6h->daddr, 0,
&ip6h->saddr)) {
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
kfree_skb(skb);
return NULL;
}
@@ -959,7 +959,7 @@ static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
skb_pull(skb, sizeof(struct ethhdr));
if (amt_send_membership_query(amt, skb, tunnel, true)) {
- amt->dev->stats.tx_dropped++;
+ DEV_STATS_INC(amt->dev, tx_dropped);
kfree_skb(skb);
}
}
@@ -1319,7 +1319,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
group.ip6 = ipv6_hdr(skb)->daddr;
#endif
} else {
- dev->stats.tx_errors++;
+ DEV_STATS_INC(dev, tx_errors);
goto free;
}
@@ -1332,12 +1332,21 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
/* Gateway only passes IGMP/MLD packets */
if (!report)
goto free;
+ /* A validated report can only be forwarded after the relay's
+ * family-specific Membership Query supplies the state echoed
+ * by the Membership Update. Log this readiness failure before
+ * the shared drop path accounts it.
+ */
if ((!v6 && !READ_ONCE(amt->ready4)) ||
- (v6 && !READ_ONCE(amt->ready6)))
+ (v6 && !READ_ONCE(amt->ready6))) {
+ netdev_dbg(dev, "drop %s report: waiting for Membership Query\n",
+ v6 ? "MLD" : "IGMP");
goto free;
+ }
if (amt_send_membership_update(amt, skb, v6))
goto free;
- goto unlock;
+ /* Sent: the tunnel consumed the skb. Not a drop. */
+ return NETDEV_TX_OK;
} else if (amt->mode == AMT_MODE_RELAY) {
if (!data)
goto free;
@@ -1366,8 +1375,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_OK;
free:
dev_kfree_skb(skb);
-unlock:
- dev->stats.tx_dropped++;
+ DEV_STATS_INC(dev, tx_dropped);
return NETDEV_TX_OK;
}
@@ -2730,7 +2738,7 @@ static void amt_send_advertisement(struct amt_dev *amt, __be32 nonce,
IPPROTO_UDP, 0,
amt->stream_dev->ifindex);
if (IS_ERR(rt)) {
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
goto out;
}
@@ -2740,7 +2748,7 @@ static void amt_send_advertisement(struct amt_dev *amt, __be32 nonce,
skb = netdev_alloc_skb_ip_align(amt->dev, len);
if (!skb) {
ip_rt_put(rt);
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
goto out;
}
@@ -2786,7 +2794,7 @@ static void amt_send_advertisement(struct amt_dev *amt, __be32 nonce,
ip_send_check(iph);
err = ip_local_out(amt->net, sk, skb);
if (unlikely(net_xmit_eval(err)))
- amt->dev->stats.tx_errors++;
+ DEV_STATS_INC(amt->dev, tx_errors);
out:
rcu_read_unlock();
--
2.47.3
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net v5 5/5] amt: harden relay authentication and teardown
2026-11-05 12:50 [PATCH net v5 0/5] amt: relay tunnel lifetime, validation and request-path hardening Omar Ramadan
` (3 preceding siblings ...)
2026-11-05 12:50 ` [PATCH net v5 4/5] amt: make pre-query report drops visible Omar Ramadan
@ 2026-11-05 12:50 ` Omar Ramadan
4 siblings, 0 replies; 6+ messages in thread
From: Omar Ramadan @ 2026-11-05 12:50 UTC (permalink / raw)
To: Taehee Yoo, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Simon Horman, Sebastian Andrzej Siewior,
Clark Williams, Steven Rostedt, linux-rt-devel
An AMT Request has an unauthenticated, spoofable source address. Allocating
tunnel state from that path allowed spoofed Requests to consume tunnel
slots or overwrite the nonce and MAC of an established gateway.
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.
Every Membership Update, forged ones included, reads the secret
generations to check its MAC. Their writers hold amt->lock and update
them inside a seqcount, amt->key_seq. The Update path copies them in a
key_seq read loop, so it takes amt->lock only on PREEMPT_RT, to wait
for a writer.
Keying by address and port lets one address hold several tunnels;
before this patch it could hold at most one. AMT_MAX_TUNNELS_PER_SRC
(16) bounds that, so one address can fill at most an eighth of the
default 128-slot table while gateways behind NAT still get several
tunnels. A configurable limit needs new uAPI, so it is left for
net-next.
Found by code inspection against RFC 7450; not triggered at runtime.
Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
drivers/net/amt.c | 309 +++++++++++++++++++++++++++++++++-------------
include/net/amt.h | 23 ++--
2 files changed, 238 insertions(+), 94 deletions(-)
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 26a942c2a..312b36b54 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 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);
-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)
{
struct sk_buff *skb;
@@ -870,7 +875,8 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
return;
skb_pull(skb, sizeof(struct ethhdr));
- if (amt_send_membership_query(amt, skb, tunnel, false)) {
+ if (amt_send_membership_query(amt, skb, daddr, dport, nonce, mac,
+ false)) {
DEV_STATS_INC(amt->dev, tx_dropped);
kfree_skb(skb);
}
@@ -949,7 +955,8 @@ static struct sk_buff *amt_build_mld_gq(struct amt_dev *amt)
return skb;
}
-static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
+static void amt_send_mld_gq(struct amt_dev *amt, __be32 daddr, __be16 dport,
+ __be32 nonce, u64 mac)
{
struct sk_buff *skb;
@@ -958,13 +965,15 @@ static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
return;
skb_pull(skb, sizeof(struct ethhdr));
- if (amt_send_membership_query(amt, skb, tunnel, true)) {
+ if (amt_send_membership_query(amt, skb, daddr, dport, nonce, mac,
+ true)) {
DEV_STATS_INC(amt->dev, tx_dropped);
kfree_skb(skb);
}
}
#else
-static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
+static void amt_send_mld_gq(struct amt_dev *amt, __be32 daddr, __be16 dport,
+ __be32 nonce, u64 mac)
{
}
#endif
@@ -998,7 +1007,16 @@ 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));
+ write_seqcount_begin(&amt->key_seq);
+ /* Keep three previous generations for a 180-240s MAC lifetime.
+ * Its 180s floor exceeds the advertised QQIC (amt->qi, 125s),
+ * allowing the gateway to renew its MAC with a periodic Request.
+ */
+ amt->key[3] = amt->key[2];
+ amt->key[2] = amt->key[1];
+ amt->key[1] = amt->key[0];
+ get_random_bytes(&amt->key[0], sizeof(siphash_key_t));
+ write_seqcount_end(&amt->key_seq);
spin_unlock_bh(&amt->lock);
mod_delayed_work(amt_wq, &amt->secret_wq,
msecs_to_jiffies(AMT_SECRET_TIMEOUT));
@@ -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.
+ */
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)
{
struct amt_header_membership_query *amtmq;
@@ -1221,13 +1246,13 @@ static bool amt_send_membership_query(struct amt_dev *amt,
skb_reset_inner_headers(skb);
memset(&fl4, 0, sizeof(struct flowi4));
fl4.flowi4_oif = amt->stream_dev->ifindex;
- fl4.daddr = tunnel->ip4;
+ fl4.daddr = daddr;
fl4.saddr = amt->local_ip;
fl4.flowi4_dscp = inet_dsfield_to_dscp(AMT_TOS);
fl4.flowi4_proto = IPPROTO_UDP;
rt = ip_route_output_key(amt->net, &fl4);
if (IS_ERR(rt)) {
- netdev_dbg(amt->dev, "no route to %pI4\n", &tunnel->ip4);
+ netdev_dbg(amt->dev, "no route to %pI4\n", &daddr);
return true;
}
@@ -1237,8 +1262,8 @@ static bool amt_send_membership_query(struct amt_dev *amt,
amtmq->reserved = 0;
amtmq->l = 0;
amtmq->g = 0;
- amtmq->nonce = tunnel->nonce;
- amtmq->response_mac = tunnel->mac;
+ amtmq->nonce = nonce;
+ amtmq->response_mac = mac;
if (!v6)
skb_set_inner_protocol(skb, htons(ETH_P_IP));
@@ -1251,11 +1276,10 @@ static bool amt_send_membership_query(struct amt_dev *amt,
ip4_dst_hoplimit(&rt->dst),
0,
amt->relay_port,
- tunnel->source_port,
+ dport,
false,
false,
0);
- amt_update_relay_status(tunnel, AMT_STATUS_SENT_QUERY, true);
return false;
}
@@ -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;
+
+ 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);
+
+ /* 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;
+}
+
static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
{
struct amt_header_membership_update *amtmu;
struct amt_tunnel_list *tunnel;
struct ethhdr *eth;
struct iphdr *iph;
- int len, hdr_size;
+ siphash_key_t key[4];
+ int len, hdr_size, i;
+ unsigned int seq;
u64 response_mac;
+ bool verified = false;
__be32 saddr;
__be32 nonce;
+ __be16 sport;
saddr = ip_hdr(skb)->saddr;
@@ -2609,34 +2724,52 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
nonce = amtmu->nonce;
response_mac = amtmu->response_mac;
+ /* Snapshot the tunnel endpoint port before the encap is stripped. */
+ sport = udp_hdr(skb)->source;
- if (iptunnel_pull_header(skb, hdr_size, skb->protocol, false))
+ /* Recompute the MAC handed out in the Membership Query. Four live
+ * secrets cover 180-240s, exceeding the advertised QQIC (amt->qi,
+ * 125s), so a periodic Request can renew the MAC before it expires.
+ * Authenticate before decapsulation or allocating tunnel state.
+ */
+ do {
+ seq = read_seqcount_begin(&amt->key_seq);
+ for (i = 0; i < ARRAY_SIZE(key); i++)
+ key[i] = amt->key[i];
+ } while (read_seqcount_retry(&amt->key_seq, seq));
+
+ 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;
+ }
+ }
+
+ if (!verified) {
+ netdev_dbg(amt->dev, "Invalid MAC\n");
return true;
+ }
- skb_reset_network_header(skb);
+ tunnel = amt_tunnel_get_or_create(amt, saddr, sport);
+ if (!tunnel)
+ return true;
- list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
- if (tunnel->ip4 == saddr) {
- if ((nonce == tunnel->nonce &&
- response_mac == tunnel->mac)) {
- /* 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);
- goto report;
- } else {
- netdev_dbg(amt->dev, "Invalid MAC\n");
- 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);
- return true;
+ if (iptunnel_pull_header(skb, hdr_size, skb->protocol, false))
+ return true;
+
+ skb_reset_network_header(skb);
-report:
if (!pskb_may_pull(skb, sizeof(*iph)))
return true;
@@ -2821,15 +2954,29 @@ static bool amt_discovery_handler(struct amt_dev *amt, struct sk_buff *skb)
return false;
}
+/* Handle an AMT Relay Membership Request.
+ *
+ * The source of a Request is unauthenticated: it is a bare UDP datagram with a
+ * trivially spoofable source address. Allocating tunnel state here let an
+ * attacker fill the table from spoofed addresses, and let one spoofed packet
+ * overwrite the nonce/MAC of an established tunnel and cut that gateway off.
+ *
+ * Nothing needs to be remembered at this point. Every input to the keyed MAC
+ * is carried in the packet or is device state, so the MAC is generated here,
+ * echoed back by the gateway in its Membership Update, and recomputed and
+ * verified there - see amt_update_handler(). Tunnel state is created only once
+ * that verification succeeds.
+ */
static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
{
struct amt_header_request *amtrh;
struct amt_tunnel_list *tunnel;
- unsigned long long key;
struct udphdr *udph;
struct iphdr *iph;
+ siphash_key_t key;
+ bool full, have = false;
+ int per_src = 0;
u64 mac;
- int i;
if (!pskb_may_pull(skb, sizeof(*udph) + sizeof(*amtrh)))
return true;
@@ -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;
- list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
- if (tunnel->ip4 == iph->saddr)
- goto send;
-
- spin_lock_bh(&amt->lock);
- if (amt->nr_tunnels >= amt->max_tunnels) {
- spin_unlock_bh(&amt->lock);
- icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
- return true;
- }
-
- tunnel = kzalloc(sizeof(*tunnel) +
- (sizeof(struct hlist_head) * amt->hash_buckets),
- GFP_ATOMIC);
- if (!tunnel) {
- spin_unlock_bh(&amt->lock);
+ if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
return true;
- }
-
- tunnel->source_port = udph->source;
- tunnel->ip4 = iph->saddr;
-
- memcpy(&key, &tunnel->key, sizeof(unsigned long long));
- tunnel->amt = amt;
- spin_lock_init(&tunnel->lock);
- for (i = 0; i < amt->hash_buckets; i++)
- INIT_HLIST_HEAD(&tunnel->groups[i]);
-
- INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
- list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
- tunnel->key = amt->key;
- __amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_REQUEST, true);
- amt->nr_tunnels++;
- mod_delayed_work(amt_wq, &tunnel->gc_wq,
- msecs_to_jiffies(amt_gmi(amt)));
+ /* 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);
-send:
- tunnel->nonce = amtrh->nonce;
- mac = siphash_3u32((__force u32)tunnel->ip4,
- (__force u32)tunnel->source_port,
- (__force u32)tunnel->nonce,
- &tunnel->key);
- tunnel->mac = mac >> 16;
+ rcu_read_lock();
+ list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
+ if (tunnel->ip4 != iph->saddr)
+ continue;
+ per_src++;
+ if (tunnel->source_port == udph->source)
+ have = true;
+ }
+ rcu_read_unlock();
- if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
+ /* Withhold the Query when a new endpoint cannot get a tunnel. */
+ if (!have && (full || per_src >= AMT_MAX_TUNNELS_PER_SRC))
return true;
+ mac = siphash_3u32((__force u32)iph->saddr,
+ (__force u32)udph->source,
+ (__force u32)amtrh->nonce,
+ &key) >> 16;
if (!amtrh->p)
- amt_send_igmp_gq(amt, tunnel);
+ amt_send_igmp_gq(amt, iph->saddr, udph->source, amtrh->nonce,
+ mac);
else
- amt_send_mld_gq(amt, tunnel);
+ amt_send_mld_gq(amt, iph->saddr, udph->source, amtrh->nonce,
+ mac);
return false;
}
@@ -3161,6 +3290,16 @@ static int amt_dev_open(struct net_device *dev)
enable_delayed_work(&amt->discovery_wq);
enable_delayed_work(&amt->req_wq);
+ /* Seed the keys before amt_socket_create() enables the encap receive
+ * path, so a Request or Update that arrives immediately is never
+ * verified against an all-zero (predictable) secret.
+ */
+ spin_lock_bh(&amt->lock);
+ write_seqcount_begin(&amt->key_seq);
+ get_random_bytes(&amt->key, sizeof(amt->key));
+ write_seqcount_end(&amt->key_seq);
+ spin_unlock_bh(&amt->lock);
+
err = amt_socket_create(amt);
if (err) {
disable_delayed_work(&amt->req_wq);
@@ -3171,7 +3310,6 @@ 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));
amt->status = AMT_STATUS_INIT;
if (amt->mode == AMT_MODE_GATEWAY) {
@@ -3384,6 +3522,7 @@ static int amt_newlink(struct net_device *dev,
amt->max_tunnels = AMT_MAX_TUNNELS;
spin_lock_init(&amt->lock);
+ seqcount_spinlock_init(&amt->key_seq, &amt->lock);
amt->max_groups = AMT_MAX_GROUP;
amt->max_sources = AMT_MAX_SOURCE;
amt->hash_buckets = AMT_HSIZE;
diff --git a/include/net/amt.h b/include/net/amt.h
index 123725c88..abe528bec 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -242,20 +242,17 @@ struct amt_tunnel_list {
struct delayed_work gc_wq;
/* Last time this tunnel was refreshed. gc_wq
* re-arms itself from this rather than being re-armed by the Update
- * path, so the Update path cannot re-queue the work of a tunnel that
- * amt_tunnel_expire() is freeing.
+ * path, so the Update path cannot re-queue work for a tunnel that
+ * amt_tunnel_expire() is freeing on the timeout path.
*/
unsigned long last_seen;
/* Set under amt->lock and tunnel->lock by whichever freer reaches the
- * 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.
*/
bool dead;
__be16 source_port;
__be32 ip4;
- __be32 nonce;
- siphash_key_t key;
- u64 mac:48,
- reserved:16;
struct rcu_head rcu;
struct hlist_head groups[];
};
@@ -341,8 +338,12 @@ struct amt_dev {
struct work_struct event_wq;
/* AMT status */
enum amt_status status;
- /* Generated key */
- siphash_key_t key;
+ /* Current key and three previous generations keep MACs valid for
+ * 180-240s, beyond the advertised QQIC (see amt_secret_work()).
+ */
+ siphash_key_t key[4];
+ /* For lockless readers of key; writers hold lock */
+ seqcount_spinlock_t key_seq;
struct sock __rcu *sk;
u32 max_groups;
u32 max_sources;
@@ -399,6 +400,10 @@ struct amt_dev {
#define AMT_SECRET_TIMEOUT 60000
#define IANA_AMT_UDP_PORT 2268
#define AMT_MAX_TUNNELS 128
+/* Fixed per-source cap stops one verified source address from monopolising
+ * the AMT_MAX_TUNNELS-slot table.
+ */
+#define AMT_MAX_TUNNELS_PER_SRC 16
#define AMT_MAX_REQS 128
#define AMT_GW_HLEN (sizeof(struct iphdr) + \
sizeof(struct udphdr) + \
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread