* [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-06 12:50 ` sashiko-bot
2026-11-09 14:49 ` netdev-bot+sashiko
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, 2 replies; 12+ 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] 12+ messages in thread* Re: [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 1/5] amt: fix use-after-free when a Membership Update re-arms a freed tunnel Omar Ramadan
@ 2026-11-06 12:50 ` sashiko-bot
2026-11-09 14:49 ` netdev-bot+sashiko
1 sibling, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-11-06 12:50 UTC (permalink / raw)
To: Omar Ramadan; +Cc: linux-rt-devel, linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] amt: workqueue deadlock in amt_tunnel_expire() and
__amt_group_gc_work()
- [Medium] amt: jiffies underflow in amt_tunnel_expire() when re-arming
gc_wq
--
commit 96e3ef58f0da0bec1417666e7bc125f374740408
Author: Omar Ramadan <omar@blockcast.net>
amt: fix use-after-free when a Membership Update re-arms a freed tunnel
This commit fixes a use-after-free where amt_update_handler() could re-arm
gc_wq on a tunnel concurrently being freed by amt_tunnel_expire(), by
tracking tunnel liveness via last_seen and deferring re-arming to
amt_tunnel_expire(). It also updates tunnel, group, and source teardown to
mark objects dead under tunnel->lock and defer freeing via RCU and dedicated
garbage-collection work items.
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 81d26ef82cf5c..d8914d811f928 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
[ ... ]
> @@ -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);
[ ... ]
> @@ -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);
[Severity: Medium]
Can jiffies advance past deadline between the time_before(jiffies, deadline)
check and the subtraction in mod_delayed_work() in amt_tunnel_expire()?
Since jiffies is volatile and spin_lock_bh(&amt->lock) only disables softirqs
while leaving timer interrupts enabled, reading jiffies twice without
snapshotting it into a local variable allows jiffies to tick past deadline
between the two reads. If that happens, deadline - jiffies underflows to a
value near ULONG_MAX, scheduling tunnel->gc_wq far in the future so the
tunnel is never expired until device teardown.
Would snapshotting unsigned long now = jiffies once before computing and
comparing against deadline avoid this underflow?
> + 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);
[Severity: High]
Can calling flush_delayed_work(&group_gc_wq) from amt_tunnel_expire() and
flush_delayed_work(&source_gc_wq) from amt_group_gc_work() ->
__amt_group_gc_work() deadlock amt_wq?
In amt_init(), amt_wq is created as a single shared unbound workqueue:
amt_wq = alloc_workqueue("amt", WQ_UNBOUND, 0);
which has a finite max_active limit (and per-node active limits) shared by
tunnel->gc_wq, group_gc_wq, source_gc_wq, gnode->group_timer, and
snode->source_timer.
Both group_gc_wq and source_gc_wq normally sit on their 30-second periodic
timers and do not hold an active workqueue slot. When flush_delayed_work()
runs from a worker already executing on amt_wq, it deletes the timer and calls
__queue_work() to queue the target work item onto amt_wq, then blocks in
flush_work() -> wait_for_completion() while still holding its own active slot
on amt_wq.
If all active slots on amt_wq are occupied by concurrent amt_tunnel_expire()
or timer work items (or if pwq->inactive_works is non-empty), the newly
queued group_gc_wq or source_gc_wq is placed on pwq->inactive_works and
cannot run until an active work item finishes, while the active workers are
blocked waiting for that inactive GC work item to complete. This will also
stall amt_dev_stop() under RTNL when it waits on cancel_delayed_work_sync()
or flush_delayed_work().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261105125010.1107546-1-omar@blockcast.net?part=1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [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 1/5] amt: fix use-after-free when a Membership Update re-arms a freed tunnel Omar Ramadan
2026-11-06 12:50 ` sashiko-bot
@ 2026-11-09 14:49 ` netdev-bot+sashiko
1 sibling, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 14:49 UTC (permalink / raw)
To: omar
Cc: ap420073, andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, horms, bigeasy, clrkwllms, rostedt, linux-rt-devel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 0 · 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).
- [High] amt_tunnel_expire() (drivers/net/amt.c:1412) runs on amt_wq.
- [High] amt_del_group() (drivers/net/amt.c:293) used to free the group
node with kfree_rcu().
- [Low] amt_tunnel_expire() (drivers/net/amt.c:1432-1440) treats
tunnel->last_seen == 0 as 'no authenticated Membership Update seen'…
- [Low] The subject and opening paragraphs frame this commit as a fix for
one race the author calls theoretical: amt_update_handler() re-arming…
--
amt: fix use-after-free when a Membership Update re-arms a freed tunnel
This patch stops amt_update_handler() from re-arming a relay tunnel's
gc_wq. The Update path now records last_seen, and amt_tunnel_expire()
uses it to decide whether to re-arm or free the tunnel. It also adds dead
flags for tunnels, groups and sources, and defers group node freeing to a
new global group_gc_wq.
> amt: fix use-after-free when a Membership Update re-arms a freed tunnel
[Severity: Low]
The subject and first paragraph describe one race, which the message
calls theoretical. The diff also fixes several existing lifetime bugs
that the message never names as bugs:
1) In the old amt_dev_stop(), list_for_each_entry_safe() calls
list_del_rcu() without amt->lock, before
cancel_delayed_work_sync(&tunnel->gc_wq). A concurrent
amt_tunnel_expire() can list_del_rcu() and kfree_rcu() the same tunnel.
amt_dev_stop() then calls amt_clear_groups() and kfree_rcu() on it
again: list corruption and a double free. Only an ifdown racing a tunnel
timeout is needed to hit it.
2) The old amt_destroy_source() reused snode->node for
hlist_add_head_rcu() onto source_gc_list, while RCU readers could still
be walking the group's source hash.
3) The old __amt_source_gc_work() freed source nodes without a
synchronous cancel of source_timer.
4) The old amt_del_group() called kfree_rcu(gnode) after only
cancel_delayed_work(), while group_timer or source_timer work could
still be using the node.
The message covers these only with:
Device stop marks tunnels dead, unlinks them with list_del_rcu(),
synchronously cancels their work, and frees them after lockless
readers have passed.
and:
Review of v3 and v4 raised the races in the group and source timer
work.
Could the commit message describe each of these failures? Alternatively,
could they be split into separate patches, each with its own Fixes tag,
to make stable backport selection and bisection easier?
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 81d26ef82cf5c..d8914d811f928 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
[ ... ]
> @@ -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);
> }
[Severity: High]
Before this patch the group node was freed one RCU grace period after
deletion. It now waits on the global group_gc_list, which is drained only
every AMT_GC_INTERVAL (30 seconds), on tunnel expiry, or on device stop.
amt_del_group() has already decremented tunnel->nr_groups by this point,
and amt_add_group() only checks:
if (tunnel->nr_groups >= amt->max_groups)
return ERR_PTR(-ENOSPC);
What limits the number of deleted group nodes waiting on group_gc_list?
Each node is kzalloc(sizeof(*gnode) + AMT_HSIZE * sizeof(struct
hlist_head), GFP_ATOMIC). That is about 2.2KB, so it lands in kmalloc-4k.
A remote gateway with a valid response MAC can alternate IGMPv2 Report
and Leave (or MLDv1 Report and Done) inside Membership Updates:
amt_update_handler()
amt_igmp_report_handler()
amt_igmpv2_report_handler() -> amt_add_group()
amt_igmpv2_leave_handler() -> amt_del_group()
Could this pin about 4KB per Report/Leave pair for up to 30 seconds? At
20k pps, for example, that is around 1.2GB, and pinned memory grows with
the packet rate.
When the GC finally runs, __amt_group_gc_work() walks the whole spliced
list with no cond_resched():
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);
}
amt_tunnel_expire() and amt_dev_stop() (under RTNL) both wait behind
this list via flush_delayed_work(&group_gc_wq).
The deferral is still there at the end of the series.
[ ... ]
> @@ -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);
[ ... ]
> + 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;
> + }
[Severity: Low]
Is 0 safe to use here as the "never refreshed" value for last_seen?
amt_update_handler() stores the raw value:
WRITE_ONCE(tunnel->last_seen, jiffies);
and jiffies can be 0. On 32-bit kernels INITIAL_JIFFIES is
(unsigned long)(unsigned int)(-300*HZ). So jiffies is 0 about 5 minutes
after boot, and again every 2^32 ticks.
Suppose a tunnel's last authenticated Update arrives in that tick and no
other Update follows. The next gc_wq run would then set deadline =
jiffies and free a live tunnel up to 3 * GMI early, tearing down its
group and source state.
Storing jiffies | 1, or using a separate flag, would avoid the collision.
The check is unchanged at the end of the series, where
amt_tunnel_get_or_create() also sets last_seen = jiffies.
> + 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);
> }
[Severity: High]
Can this flush deadlock amt_wq? amt_tunnel_expire() runs on amt_wq, and
so do group_gc_wq and source_gc_wq. That gives a chain of three
interdependent work items on one workqueue:
amt_tunnel_expire()
flush_delayed_work(&group_gc_wq)
__amt_group_gc_work()
flush_delayed_work(&source_gc_wq)
amt_init() creates the workqueue with:
amt_wq = alloc_workqueue("amt", WQ_UNBOUND, 0);
so it has no rescuer. wq_update_node_max_active() splits the default
max_active across NUMA nodes: about 1024 on a single node box, about 128
per node on an 8-node box. The workqueue code documents that an unbound
workqueue is only guaranteed to process min_active (WQ_DFL_MIN_ACTIVE, 8)
interdependent work items.
Each amt_tunnel_expire() sleeping in flush_work() keeps its active slot.
Gateways can create tunnels remotely, and gc_wq timeouts of several
minutes fall into coarse timer wheel buckets. Enough expiries could then
run together to fill the node's active limit before group_gc_wq is
activated. Would group_gc_wq (or source_gc_wq one level down) then stay
inactive, with every blocked expire waiting forever?
amt_wq is shared by all AMT devices, so this would also stall event_wq,
the group and source timers, and secret_wq for all of them.
amt_dev_stop() calls flush_delayed_work(&group_gc_wq) with RTNL held, so
a later ifdown would hang while holding RTNL.
The commit message says lifetime "does not rest on the flush". Are these
flushes needed? They are still there at the end of the series.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105125010.1107546-1-omar%40blockcast.net
^ permalink raw reply [flat|nested] 12+ 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-09 14:49 ` netdev-bot+sashiko
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, 1 reply; 12+ 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] 12+ messages in thread* Re: [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 2/5] amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned Omar Ramadan
@ 2026-11-09 14:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 14:49 UTC (permalink / raw)
To: omar
Cc: ap420073, andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, horms, bigeasy, clrkwllms, rostedt, linux-rt-devel
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message for "amt: reject a gateway Query or Data from
0.0.0.0 before a relay is learned" gets part of the root cause wrong…
--
amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned
This patch adds a !remote_ip guard to the Membership Query and Multicast
Data source checks in amt_rcv(), so a gateway with no learned relay
rejects packets from 0.0.0.0. It also rechecks the packet source against
the current remote_ip in amt_membership_query_handler(), which drops
Queries that were deferred across a relay address change.
> 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.
[Severity: Low]
The code change looks correct. Some parts of this description don't
match what drivers/net/amt.c does, though.
Is "when discovery gives up" accurate? amt_event_send_discovery() never
clears remote_ip. It only randomizes amt->nonce:
drivers/net/amt.c:amt_event_send_discovery() {
if (amt->status > AMT_STATUS_SENT_DISCOVERY)
goto out;
get_random_bytes(&amt->nonce, sizeof(__be32));
...
}
The reset happens in the Request phase, in amt_event_send_request(),
once req_cnt exceeds AMT_MAX_REQ_COUNT:
WRITE_ONCE(amt->remote_ip, 0);
amt_update_gw_status(amt, AMT_STATUS_INIT, false);
amt->req_cnt = 0;
amt->nonce = 0;
amt_dev_stop() and amt_newlink() also clear remote_ip. The message does
not mention either of them.
Are the Query spoof and the Data injection really two separate
exposures? amt_multicast_data_handler() already drops every packet
unless the gateway is in AMT_STATUS_SENT_UPDATE:
if (READ_ONCE(amt->status) != AMT_STATUS_SENT_UPDATE)
return true;
With remote_ip == 0, the gateway can only be in that state in two cases.
The first is after a spoofed Query has moved status to RECEIVED_QUERY
and amt_dev_xmit()->amt_send_membership_update() has set SENT_UPDATE.
The second is the short gap between WRITE_ONCE(amt->remote_ip, 0) and
the status reset.
So the Data hunk looks like defense in depth rather than a separate
hole. Could the message say so?
The message also doesn't mention the condition that makes the Query
spoof work. amt_membership_query_handler() also checks the nonce:
if (amtmq->nonce != amt->nonce)
return true;
amt->nonce is zeroed along with remote_ip in amt_dev_open() and in the
request give-up path above. A Query from 0.0.0.0 with nonce 0 would
therefore pass every check until the next discovery event randomizes
the nonce. Should the changelog state this?
The effect of a spoofed Query also seems larger than "could spoof a
Query". Besides injecting a forged IGMP/MLD query, the handler changes
gateway state:
WRITE_ONCE(amt->ready4, true);
amt->mac = response_mac;
amt->req_cnt = 0;
amt->qi = ihv3->qqic;
It also moves status from INIT to RECEIVED_QUERY. After that,
amt_event_send_discovery() bails out on the status check shown above.
amt_advertisement_handler() then rejects every Advertisement:
if (amt->status != AMT_STATUS_SENT_DISCOVERY ||
amt->nonce != amta->nonce)
return true;
Relay discovery stays blocked until the Request retry limit puts the
device back into INIT with nonce 0. At that point the spoof can be sent
again.
Would it be worth describing the nonce 0 condition and the blocked
discovery in the changelog, so stable backporters can judge the impact?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105125010.1107546-1-omar%40blockcast.net
^ permalink raw reply [flat|nested] 12+ 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; 12+ 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] 12+ 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-09 14:49 ` netdev-bot+sashiko
2026-11-05 12:50 ` [PATCH net v5 5/5] amt: harden relay authentication and teardown Omar Ramadan
4 siblings, 1 reply; 12+ 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] 12+ messages in thread* Re: [PATCH net v5 4/5] amt: make pre-query report drops visible
2026-11-05 12:50 ` [PATCH net v5 4/5] amt: make pre-query report drops visible Omar Ramadan
@ 2026-11-09 14:49 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 14:49 UTC (permalink / raw)
To: omar
Cc: ap420073, andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, horms, bigeasy, clrkwllms, rostedt, linux-rt-devel
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message says DEV_STATS_INC() is applied only to
amt_dev_xmit()'s counters: "The device is LLTX, so amt_dev_xmit() can…
--
amt: make pre-query report drops visible
This patch adds a debug message when an amt gateway drops an IGMP or MLD
report because the relay's Membership Query has not arrived yet. It also
stops amt_dev_xmit() from counting reports it forwarded as tx_dropped.
Finally, it switches the device's tx_dropped and tx_errors updates to
DEV_STATS_INC().
> 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.
[Severity: Low]
This isn't a bug, but does this paragraph cover the whole diff?
It describes the DEV_STATS_INC() change as limited to amt_dev_xmit().
Only two of the converted sites are in that function. The patch also
converts every other tx_errors and tx_dropped increment in
drivers/net/amt.c:
amt_send_discovery() (three tx_errors sites)
amt_send_request() (three tx_errors sites)
amt_send_igmp_gq() (tx_dropped)
amt_build_mld_gq() (tx_errors)
amt_send_mld_gq() (tx_dropped)
amt_send_advertisement() (three tx_errors sites)
These run from the gateway workqueue and the relay receive path, not
from ndo_start_xmit.
The extra conversions look correct and are needed. DEV_STATS_INC() does
atomic_long_inc() on the same storage as dev->stats.tx_errors and
dev->stats.tx_dropped. A plain ++ left on either field could overwrite
an atomic increment from amt_dev_xmit().
Could the commit message mention these conversions and give that reason?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105125010.1107546-1-omar%40blockcast.net
^ permalink raw reply [flat|nested] 12+ 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
2026-11-06 12:50 ` sashiko-bot
2026-11-09 14:49 ` netdev-bot+sashiko
4 siblings, 2 replies; 12+ 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] 12+ messages in thread* Re: [PATCH net v5 5/5] amt: harden relay authentication and teardown
2026-11-05 12:50 ` [PATCH net v5 5/5] amt: harden relay authentication and teardown Omar Ramadan
@ 2026-11-06 12:50 ` sashiko-bot
2026-11-09 14:49 ` netdev-bot+sashiko
1 sibling, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-11-06 12:50 UTC (permalink / raw)
To: Omar Ramadan; +Cc: linux-rt-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net v5 5/5] amt: harden relay authentication and teardown
2026-11-05 12:50 ` [PATCH net v5 5/5] amt: harden relay authentication and teardown Omar Ramadan
2026-11-06 12:50 ` sashiko-bot
@ 2026-11-09 14:49 ` netdev-bot+sashiko
1 sibling, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-11-09 14:49 UTC (permalink / raw)
To: omar
Cc: ap420073, andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, horms, bigeasy, clrkwllms, rostedt, linux-rt-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread