[PATCH net v3 0/1] atm: mpoa: Fix UAF on qos_head list in procfs

[PATCH net v3 0/1] atm: mpoa: Fix UAF on qos_head list in procfs

[Minseong Kim]

Changes since v2:
  - Replace atm_mpoa_search_qos() with atm_mpoa_get_qos() to avoid
    returning an unprotected pointer.
  - Add atm_mpoa_delete_qos_by_ip() so search+unlink+free is atomic
    under qos_mutex, preventing double-free/UAF.
  - Update all callers accordingly.

Minseong Kim (1):
  atm: mpoa: Fix UAF on qos_head list in procfs

 net/atm/mpc.c         | 184 ++++++++++++++++++++++++++++++++----------
 net/atm/mpc.h         |   3 +-
 net/atm/mpoa_caches.c |  17 ++--
 net/atm/mpoa_proc.c   |   2 +-
 4 files changed, 149 insertions(+), 57 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH net v3 1/1] atm: mpoa: Fix UAF on qos_head list in procfs

[Minseong Kim]

The global QoS list 'qos_head' in net/atm/mpc.c is accessed from the
/proc/net/atm/mpc procfs interface without proper synchronization.
The read-side seq_file show path (mpc_show() -> atm_mpoa_disp_qos())
walks qos_head without any lock, while the write-side path
(proc_mpc_write() -> parse_qos() -> atm_mpoa_delete_qos()) can unlink
and kfree() entries immediately. Concurrent read/write therefore leads
to a use-after-free.

Fix this by serializing all qos_head list operations with a mutex.
A mutex is used because seq_printf() may sleep.

To avoid returning an unprotected pointer (and the resulting lifetime
race), replace atm_mpoa_search_qos() with atm_mpoa_get_qos(), which
copies the QoS under lock. Also add atm_mpoa_delete_qos_by_ip() so
search+unlink+free is atomic under the same lock, preventing concurrent
double-free/UAF scenarios.

KASAN report:

[   15.911538] BUG: KASAN: slab-use-after-free in atm_mpoa_disp_qos+0x202/0x380
[   15.911988] Read of size 8 at addr ffff888007517380 by task mpoa_uaf_poc/89
[   15.912123]
[   15.912717] CPU: 0 UID: 0 PID: 89 Comm: mpoa_uaf_poc Not tainted 6.17.8 #1 PREEMPT(voluntary)
[   15.912950] Hardware name: QEMU Ubuntu 25.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   15.913110] Call Trace:
[   15.913167]  <TASK>
[   15.913247]  dump_stack_lvl+0x4e/0x70
[   15.913343]  ? atm_mpoa_disp_qos+0x202/0x380
[   15.913364]  print_report+0x174/0x4f6
[   15.913386]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[   15.913412]  ? atm_mpoa_disp_qos+0x202/0x380
[   15.913430]  kasan_report+0xce/0x100
[   15.913453]  ? atm_mpoa_disp_qos+0x202/0x380
[   15.913475]  atm_mpoa_disp_qos+0x202/0x380
[   15.913496]  mpc_show+0x575/0x700
[   15.913515]  ? kasan_save_track+0x14/0x30
[   15.913532]  ? __pfx_mpc_show+0x10/0x10
[   15.913550]  ? __pfx_mutex_lock+0x10/0x10
[   15.913565]  ? seq_read_iter+0x697/0x1110
[   15.913584]  seq_read_iter+0x2bc/0x1110
[   15.913607]  seq_read+0x267/0x3d0
[   15.913623]  ? __pfx_seq_read+0x10/0x10
[   15.913650]  proc_reg_read+0x1ab/0x270
[   15.913669]  vfs_read+0x175/0xa10
[   15.913687]  ? do_sys_openat2+0x103/0x170
[   15.913701]  ? kmem_cache_free+0xc4/0x360
[   15.913718]  ? getname_flags.part.0+0xf3/0x470
[   15.913734]  ? __pfx_vfs_read+0x10/0x10
[   15.913751]  ? mutex_lock+0x81/0xe0
[   15.913766]  ? __pfx_mutex_lock+0x10/0x10
[   15.913782]  ? __rseq_handle_notify_resume+0x4c4/0xac0
[   15.913802]  ? fdget_pos+0x24d/0x4b0
[   15.913829]  ksys_read+0xf7/0x1c0
[   15.913850]  ? __pfx_ksys_read+0x10/0x10
[   15.913877]  do_syscall_64+0xa4/0x290
[   15.913917]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   15.913981] RIP: 0033:0x45c982
[   15.914171] Code: 08 0f 85 71 ea ff ff 49 89 fb 48 89 f0 48 89 d7 48 89 ce 4c 89 c2 4d 89 ca 4c 8b 44 24 08 4c 8b 4c 24 10 4c 89 5c 24 08 0f 05 <c3> 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 55 48 89 e5
[   15.914239] RSP: 002b:00007f4b2b2cb0d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
[   15.914315] RAX: ffffffffffffffda RBX: 00007f4b2b2cc6c0 RCX: 000000000045c982
[   15.914352] RDX: 0000000000000fff RSI: 00007f4b2b2cb220 RDI: 0000000000000014
[   15.914382] RBP: 00007f4b2b2cb100 R08: 0000000000000000 R09: 0000000000000000
[   15.914413] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000a
[   15.914445] R13: ffffffffffffffd0 R14: 0000000000000000 R15: 00007ffd386450c0
[   15.914483]  </TASK>
[   15.914533]
[   15.916812] Allocated by task 78:
[   15.916975]  kasan_save_stack+0x30/0x50
[   15.917047]  kasan_save_track+0x14/0x30
[   15.917105]  __kasan_kmalloc+0x8f/0xa0
[   15.917162]  atm_mpoa_add_qos+0x1bb/0x3c0
[   15.917220]  parse_qos.cold+0x73/0x7d
[   15.917276]  proc_mpc_write+0xf4/0x150
[   15.917331]  proc_reg_write+0x1ab/0x270
[   15.917379]  vfs_write+0x1ce/0xd30
[   15.917431]  ksys_write+0xf7/0x1c0
[   15.917476]  do_syscall_64+0xa4/0x290
[   15.917523]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   15.917586]
[   15.917628] Freed by task 78:
[   15.917665]  kasan_save_stack+0x30/0x50
[   15.917714]  kasan_save_track+0x14/0x30
[   15.917762]  kasan_save_free_info+0x3b/0x70
[   15.917811]  __kasan_slab_free+0x3e/0x50
[   15.918058]  kfree+0x121/0x340
[   15.918135]  atm_mpoa_delete_qos+0xad/0xd0
[   15.918196]  parse_qos+0x1e5/0x1f0
[   15.918339]  proc_mpc_write+0xf4/0x150
[   15.918438]  proc_reg_write+0x1ab/0x270
[   15.918494]  vfs_write+0x1ce/0xd30
[   15.918538]  ksys_write+0xf7/0x1c0
[   15.918589]  do_syscall_64+0xa4/0x290
[   15.918634]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   15.918694]
[   15.918751] The buggy address belongs to the object at ffff888007517380
[   15.918751]  which belongs to the cache kmalloc-96 of size 96
[   15.918911] The buggy address is located 0 bytes inside of
[   15.918911]  freed 96-byte region [ffff888007517380, ffff8880075173e0)
[   15.919027]
[   15.919101] The buggy address belongs to the physical page:
[   15.919416] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffff888007517300 pfn:0x7517
[   15.919679] anon flags: 0x100000000000000(node=0|zone=1)
[   15.920064] page_type: f5(slab)
[   15.920361] raw: 0100000000000000 ffff888006c41280 0000000000000000 dead000000000001
[   15.920460] raw: ffff888007517300 0000000080200007 00000000f5000000 0000000000000000
[   15.920577] page dumped because: kasan: bad access detected
[   15.920634]
[   15.920663] Memory state around the buggy address:
[   15.920860]  ffff888007517280: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
[   15.920944]  ffff888007517300: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
[   15.921017] >ffff888007517380: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
[   15.921088]                    ^
[   15.921163]  ffff888007517400: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
[   15.921222]  ffff888007517480: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc

Reported-by: Minseong Kim <ii4gsp@gmail.com>
Closes: https://lore.kernel.org/netdev/CAKrymDR1X3XTX_1ZW3XXXnuYH+kzsnv7Av5uivzR1sto+5BFQg@mail.gmail.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Minseong Kim <ii4gsp@gmail.com>
---
 net/atm/mpc.c         | 184 ++++++++++++++++++++++++++++++++----------
 net/atm/mpc.h         |   3 +-
 net/atm/mpoa_caches.c |  17 ++--
 net/atm/mpoa_proc.c   |   2 +-
 4 files changed, 149 insertions(+), 57 deletions(-)

diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index f6b447bba329..650081dd82d1 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -9,6 +9,7 @@
 #include <linux/bitops.h>
 #include <linux/capability.h>
 #include <linux/seq_file.h>
+#include <linux/mutex.h>
 
 /* We are an ethernet device */
 #include <linux/if_ether.h>
@@ -122,6 +123,7 @@ static struct notifier_block mpoa_notifier = {
 
 struct mpoa_client *mpcs = NULL; /* FIXME */
 static struct atm_mpoa_qos *qos_head = NULL;
+static DEFINE_MUTEX(qos_mutex); /* Protect qos_head list */
 static DEFINE_TIMER(mpc_timer, mpc_cache_check);
 
 
@@ -172,67 +174,63 @@ static struct mpoa_client *find_mpc_by_lec(struct net_device *dev)
  */
 
 /*
- * Overwrites the old entry or makes a new one.
+ * Search for a QoS entry. Caller must hold qos_mutex.
+ * Returns pointer to entry if found, NULL otherwise.
  */
-struct atm_mpoa_qos *atm_mpoa_add_qos(__be32 dst_ip, struct atm_qos *qos)
+static struct atm_mpoa_qos *__atm_mpoa_search_qos(__be32 dst_ip)
 {
-	struct atm_mpoa_qos *entry;
-
-	entry = atm_mpoa_search_qos(dst_ip);
-	if (entry != NULL) {
-		entry->qos = *qos;
-		return entry;
-	}
+	struct atm_mpoa_qos *qos = qos_head;
 
-	entry = kmalloc(sizeof(struct atm_mpoa_qos), GFP_KERNEL);
-	if (entry == NULL) {
-		pr_info("mpoa: out of memory\n");
-		return entry;
+	while (qos) {
+		if (qos->ipaddr == dst_ip)
+			return qos;
+		qos = qos->next;
 	}
-
-	entry->ipaddr = dst_ip;
-	entry->qos = *qos;
-
-	entry->next = qos_head;
-	qos_head = entry;
-
-	return entry;
+	return NULL;
 }
 
-struct atm_mpoa_qos *atm_mpoa_search_qos(__be32 dst_ip)
+/*
+ * Get a QoS entry by copying its value.
+ * Caller gets a COPY of qos under lock.
+ * Returns true if found and copied, false if not found.
+ * This avoids lifetime issues with the returned pointer.
+ */
+bool atm_mpoa_get_qos(__be32 dst_ip, struct atm_qos *out)
 {
-	struct atm_mpoa_qos *qos;
+	struct atm_mpoa_qos *q;
 
-	qos = qos_head;
-	while (qos) {
-		if (qos->ipaddr == dst_ip)
-			break;
-		qos = qos->next;
-	}
+	if (!out)
+		return false;
 
-	return qos;
+	mutex_lock(&qos_mutex);
+	q = __atm_mpoa_search_qos(dst_ip);
+	if (q)
+		*out = q->qos;
+	mutex_unlock(&qos_mutex);
+
+	return q;
 }
 
 /*
- * Returns 0 for failure
+ * Delete a QoS entry from the list. Caller must hold qos_mutex.
+ * Returns 1 if found and deleted, 0 if not found.
  */
-int atm_mpoa_delete_qos(struct atm_mpoa_qos *entry)
+static int __atm_mpoa_delete_qos_locked(struct atm_mpoa_qos *entry)
 {
 	struct atm_mpoa_qos *curr;
 
-	if (entry == NULL)
+	if (!entry)
 		return 0;
+
 	if (entry == qos_head) {
-		qos_head = qos_head->next;
-		kfree(entry);
+		qos_head = entry->next;
 		return 1;
 	}
 
 	curr = qos_head;
-	while (curr != NULL) {
+	while (curr) {
 		if (curr->next == entry) {
 			curr->next = entry->next;
-			kfree(entry);
 			return 1;
 		}
 		curr = curr->next;
@@ -241,15 +239,107 @@ int atm_mpoa_delete_qos(struct atm_mpoa_qos *entry)
 	return 0;
 }
 
+/*
+ * Delete a QoS entry by IP address.
+ * This is atomic: search+unlink+free happens entirely under lock,
+ * avoiding the double-free issue with atm_mpoa_delete_qos(atm_mpoa_search_qos(ipaddr)).
+ */
+int atm_mpoa_delete_qos_by_ip(__be32 dst_ip)
+{
+	struct atm_mpoa_qos *entry;
+	int ret = 0;
+
+	mutex_lock(&qos_mutex);
+	entry = __atm_mpoa_search_qos(dst_ip);
+	if (entry) {
+		ret = __atm_mpoa_delete_qos_locked(entry);
+		if (ret)
+			kfree(entry);
+	}
+	mutex_unlock(&qos_mutex);
+
+	return ret;
+}
+
+/*
+ * Overwrites the old entry or makes a new one.
+ */
+struct atm_mpoa_qos *atm_mpoa_add_qos(__be32 dst_ip, struct atm_qos *qos)
+{
+	struct atm_mpoa_qos *entry;
+	struct atm_mpoa_qos *new;
+
+	/* Fast path: update existing entry */
+	mutex_lock(&qos_mutex);
+	entry = __atm_mpoa_search_qos(dst_ip);
+	if (entry) {
+		entry->qos = *qos;
+		mutex_unlock(&qos_mutex);
+		return entry;
+	}
+	mutex_unlock(&qos_mutex);
+
+	/* Allocate outside lock */
+	new = kmalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return NULL;
+
+	new->ipaddr = dst_ip;
+	new->qos = *qos;
+
+	/* Re-check under lock to avoid duplicates */
+	mutex_lock(&qos_mutex);
+	entry = __atm_mpoa_search_qos(dst_ip);
+	if (entry) {
+		entry->qos = *qos;
+		mutex_unlock(&qos_mutex);
+		kfree(new);
+		return entry;
+	}
+
+	new->next = qos_head;
+	qos_head = new;
+	mutex_unlock(&qos_mutex);
+
+	return new;
+}
+
+/*
+ * Delete a QoS entry by pointer.
+ * WARNING: This function is unsafe if called with an entry pointer
+ * obtained outside of qos_mutex protection. The entry may have been
+ * freed by another thread between the time the pointer was obtained
+ * and when this function is called.
+ * Use atm_mpoa_delete_qos_by_ip() instead for safe deletion by IP address.
+ * Returns 0 for failure, 1 for success
+ */
+int atm_mpoa_delete_qos(struct atm_mpoa_qos *entry)
+{
+	int ret;
+
+	if (!entry)
+		return 0;
+
+	mutex_lock(&qos_mutex);
+	ret = __atm_mpoa_delete_qos_locked(entry);
+	if (ret)
+		kfree(entry);
+	mutex_unlock(&qos_mutex);
+
+	return ret;
+}
+
 /* this is buggered - we need locking for qos_head */
 void atm_mpoa_disp_qos(struct seq_file *m)
 {
 	struct atm_mpoa_qos *qos;
 
-	qos = qos_head;
 	seq_printf(m, "QoS entries for shortcuts:\n");
-	seq_printf(m, "IP address\n  TX:max_pcr pcr     min_pcr max_cdv max_sdu\n  RX:max_pcr pcr     min_pcr max_cdv max_sdu\n");
+	seq_printf(m, "IP address\n  TX:max_pcr pcr     min_pcr max_cdv max_sdu\n"
+		   "  RX:max_pcr pcr     min_pcr max_cdv max_sdu\n");
 
+	mutex_lock(&qos_mutex);
+	qos = qos_head;
 	while (qos != NULL) {
 		seq_printf(m, "%pI4\n     %-7d %-7d %-7d %-7d %-7d\n     %-7d %-7d %-7d %-7d %-7d\n",
 			   &qos->ipaddr,
@@ -265,6 +355,7 @@ void atm_mpoa_disp_qos(struct seq_file *m)
 			   qos->qos.rxtp.max_sdu);
 		qos = qos->next;
 	}
+	mutex_unlock(&qos_mutex);
 }
 
 static struct net_device *find_lec_by_itfnum(int itf)
@@ -1118,13 +1209,16 @@ static void check_qos_and_open_shortcut(struct k_message *msg,
 					in_cache_entry *entry)
 {
 	__be32 dst_ip = msg->content.in_info.in_dst_ip;
-	struct atm_mpoa_qos *qos = atm_mpoa_search_qos(dst_ip);
+	struct atm_qos tmp_qos;
+	bool has_qos;
 	eg_cache_entry *eg_entry = client->eg_ops->get_by_src_ip(dst_ip, client);
 
+	has_qos = atm_mpoa_get_qos(dst_ip, &tmp_qos);
+
 	if (eg_entry && eg_entry->shortcut) {
 		if (eg_entry->shortcut->qos.txtp.traffic_class &
 		    msg->qos.txtp.traffic_class &
-		    (qos ? qos->qos.txtp.traffic_class : ATM_UBR | ATM_CBR)) {
+		    (has_qos ? tmp_qos.txtp.traffic_class : ATM_UBR | ATM_CBR)) {
 			if (eg_entry->shortcut->qos.txtp.traffic_class == ATM_UBR)
 				entry->shortcut = eg_entry->shortcut;
 			else if (eg_entry->shortcut->qos.txtp.max_pcr > 0)
@@ -1142,9 +1236,9 @@ static void check_qos_and_open_shortcut(struct k_message *msg,
 
 	/* No luck in the egress cache we must open an ingress SVC */
 	msg->type = OPEN_INGRESS_SVC;
-	if (qos &&
-	    (qos->qos.txtp.traffic_class == msg->qos.txtp.traffic_class)) {
-		msg->qos = qos->qos;
+	if (has_qos &&
+	    tmp_qos.txtp.traffic_class == msg->qos.txtp.traffic_class) {
+		msg->qos = tmp_qos;
 		pr_info("(%s) trying to get a CBR shortcut\n",
 			client->dev->name);
 	} else
@@ -1521,8 +1615,10 @@ static void __exit atm_mpoa_cleanup(void)
 		mpc = tmp;
 	}
 
+	mutex_lock(&qos_mutex);
 	qos = qos_head;
 	qos_head = NULL;
+	mutex_unlock(&qos_mutex);
 	while (qos != NULL) {
 		nextqos = qos->next;
 		dprintk("freeing qos entry %p\n", qos);
diff --git a/net/atm/mpc.h b/net/atm/mpc.h
index 454abd07651a..0536946989ad 100644
--- a/net/atm/mpc.h
+++ b/net/atm/mpc.h
@@ -47,8 +47,9 @@ struct atm_mpoa_qos {
 
 /* MPOA QoS operations */
 struct atm_mpoa_qos *atm_mpoa_add_qos(__be32 dst_ip, struct atm_qos *qos);
-struct atm_mpoa_qos *atm_mpoa_search_qos(__be32 dst_ip);
+bool atm_mpoa_get_qos(__be32 dst_ip, struct atm_qos *out);
 int atm_mpoa_delete_qos(struct atm_mpoa_qos *qos);
+int atm_mpoa_delete_qos_by_ip(__be32 dst_ip);
 
 /* Display QoS entries. This is for the procfs */
 struct seq_file;
diff --git a/net/atm/mpoa_caches.c b/net/atm/mpoa_caches.c
index f7a2f0e41105..30c5b10ee562 100644
--- a/net/atm/mpoa_caches.c
+++ b/net/atm/mpoa_caches.c
@@ -132,7 +132,6 @@ static in_cache_entry *in_cache_add_entry(__be32 dst_ip,
 
 static int cache_hit(in_cache_entry *entry, struct mpoa_client *mpc)
 {
-	struct atm_mpoa_qos *qos;
 	struct k_message msg;
 
 	entry->count++;
@@ -144,9 +143,8 @@ static int cache_hit(in_cache_entry *entry, struct mpoa_client *mpc)
 			msg.type = SND_MPOA_RES_RQST;
 			msg.content.in_info = entry->ctrl_info;
 			memcpy(msg.MPS_ctrl, mpc->mps_ctrl_addr, ATM_ESA_LEN);
-			qos = atm_mpoa_search_qos(entry->ctrl_info.in_dst_ip);
-			if (qos != NULL)
-				msg.qos = qos->qos;
+			if (!atm_mpoa_get_qos(entry->ctrl_info.in_dst_ip, &msg.qos))
+				memset(&msg.qos, 0, sizeof(msg.qos));
 			msg_to_mpoad(&msg, mpc);
 			entry->reply_wait = ktime_get_seconds();
 			entry->entry_state = INGRESS_RESOLVING;
@@ -167,9 +165,8 @@ static int cache_hit(in_cache_entry *entry, struct mpoa_client *mpc)
 		msg.type = SND_MPOA_RES_RQST;
 		memcpy(msg.MPS_ctrl, mpc->mps_ctrl_addr, ATM_ESA_LEN);
 		msg.content.in_info = entry->ctrl_info;
-		qos = atm_mpoa_search_qos(entry->ctrl_info.in_dst_ip);
-		if (qos != NULL)
-			msg.qos = qos->qos;
+		if (!atm_mpoa_get_qos(entry->ctrl_info.in_dst_ip, &msg.qos))
+			memset(&msg.qos, 0, sizeof(msg.qos));
 		msg_to_mpoad(&msg, mpc);
 		entry->reply_wait = ktime_get_seconds();
 	}
@@ -249,7 +246,6 @@ static void clear_count_and_expired(struct mpoa_client *client)
 static void check_resolving_entries(struct mpoa_client *client)
 {
 
-	struct atm_mpoa_qos *qos;
 	in_cache_entry *entry;
 	time64_t now;
 	struct k_message msg;
@@ -283,9 +279,8 @@ static void check_resolving_entries(struct mpoa_client *client)
 				msg.type = SND_MPOA_RES_RTRY;
 				memcpy(msg.MPS_ctrl, client->mps_ctrl_addr, ATM_ESA_LEN);
 				msg.content.in_info = entry->ctrl_info;
-				qos = atm_mpoa_search_qos(entry->ctrl_info.in_dst_ip);
-				if (qos != NULL)
-					msg.qos = qos->qos;
+				if (!atm_mpoa_get_qos(entry->ctrl_info.in_dst_ip, &msg.qos))
+					memset(&msg.qos, 0, sizeof(msg.qos));
 				msg_to_mpoad(&msg, client);
 				entry->reply_wait = ktime_get_seconds();
 			}
diff --git a/net/atm/mpoa_proc.c b/net/atm/mpoa_proc.c
index aaf64b953915..600075c6022e 100644
--- a/net/atm/mpoa_proc.c
+++ b/net/atm/mpoa_proc.c
@@ -254,7 +254,7 @@ static int parse_qos(const char *buff)
 	if (sscanf(buff, "del %hhu.%hhu.%hhu.%hhu",
 			ip, ip+1, ip+2, ip+3) == 4) {
 		ipaddr = *(__be32 *)ip;
-		return atm_mpoa_delete_qos(atm_mpoa_search_qos(ipaddr));
+		return atm_mpoa_delete_qos_by_ip(ipaddr);
 	}
 
 	if (sscanf(buff, "add %hhu.%hhu.%hhu.%hhu tx=%d,%d rx=tx",
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net v3 1/1] atm: mpoa: Fix UAF on qos_head list in procfs

[Jakub Kicinski]

On Thu,  4 Dec 2025 15:24:21 +0900 Minseong Kim wrote:
> Reported-by: Minseong Kim <ii4gsp@gmail.com>
> Closes: https://lore.kernel.org/netdev/CAKrymDR1X3XTX_1ZW3XXXnuYH+kzsnv7Av5uivzR1sto+5BFQg@mail.gmail.com/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Minseong Kim <ii4gsp@gmail.com>

The tags are wrong. Reported-by is only used when it's not the same as
author. And you're pointing Closes at previous version? I don't get it.

> -int atm_mpoa_delete_qos(struct atm_mpoa_qos *entry)
> +static int __atm_mpoa_delete_qos_locked(struct atm_mpoa_qos *entry)
>  {
>  	struct atm_mpoa_qos *curr;
>  
> -	if (entry == NULL)
> +	if (!entry)
>  		return 0;
> +

please avoid unrelated code cleanups in fixes

>  	if (entry == qos_head) {
> -		qos_head = qos_head->next;
> -		kfree(entry);
> +		qos_head = entry->next;
>  		return 1;
>  	}
>  
>  	curr = qos_head;
> -	while (curr != NULL) {
> +	while (curr) {
>  		if (curr->next == entry) {
>  			curr->next = entry->next;
> -			kfree(entry);
>  			return 1;
>  		}
>  		curr = curr->next;

> + * Overwrites the old entry or makes a new one.
> + */
> +struct atm_mpoa_qos *atm_mpoa_add_qos(__be32 dst_ip, struct atm_qos *qos)
> +{
> +	struct atm_mpoa_qos *entry;
> +	struct atm_mpoa_qos *new;
> +
> +	/* Fast path: update existing entry */
> +	mutex_lock(&qos_mutex);
> +	entry = __atm_mpoa_search_qos(dst_ip);
> +	if (entry) {
> +		entry->qos = *qos;
> +		mutex_unlock(&qos_mutex);
> +		return entry;
> +	}
> +	mutex_unlock(&qos_mutex);
> +
> +	/* Allocate outside lock */

Why allocate outside the lock? It makes the code more complicated,
keep it simple unless you can prove real life benefits.

> +	new = kmalloc(sizeof(*new), GFP_KERNEL);
> +	if (!new)
> +		return NULL;

[PATCH net v4] atm: mpoa: Fix UAF on qos_head list in procfs

[Minseong Kim]

/proc/net/atm/mpc read-side iterates qos_head without synchronization,
while write-side can delete and free entries concurrently, leading to
use-after-free.

Protect qos_head with a mutex and ensure procfs search+delete operations
are serialized under the same lock.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Minseong Kim <ii4gsp@gmail.com>
---
 net/atm/mpc.c       |  9 ++++++++-
 net/atm/mpc.h       |  4 ++++
 net/atm/mpoa_proc.c | 11 ++++++++++-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index f6b447bba329..c198381281fa 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -9,6 +9,7 @@
 #include <linux/bitops.h>
 #include <linux/capability.h>
 #include <linux/seq_file.h>
+#include <linux/mutex.h>
 
 /* We are an ethernet device */
 #include <linux/if_ether.h>
@@ -122,6 +123,7 @@ static struct notifier_block mpoa_notifier = {
 
 struct mpoa_client *mpcs = NULL; /* FIXME */
 static struct atm_mpoa_qos *qos_head = NULL;
+DEFINE_MUTEX(qos_mutex); /* Protect qos_head list */
 static DEFINE_TIMER(mpc_timer, mpc_cache_check);
 
 
@@ -246,10 +248,11 @@ void atm_mpoa_disp_qos(struct seq_file *m)
 {
 	struct atm_mpoa_qos *qos;
 
-	qos = qos_head;
 	seq_printf(m, "QoS entries for shortcuts:\n");
 	seq_printf(m, "IP address\n  TX:max_pcr pcr     min_pcr max_cdv max_sdu\n  RX:max_pcr pcr     min_pcr max_cdv max_sdu\n");
 
+	mutex_lock(&qos_mutex);
+	qos = qos_head;
 	while (qos != NULL) {
 		seq_printf(m, "%pI4\n     %-7d %-7d %-7d %-7d %-7d\n     %-7d %-7d %-7d %-7d %-7d\n",
 			   &qos->ipaddr,
@@ -265,6 +268,7 @@ void atm_mpoa_disp_qos(struct seq_file *m)
 			   qos->qos.rxtp.max_sdu);
 		qos = qos->next;
 	}
+	mutex_unlock(&qos_mutex);
 }
 
 static struct net_device *find_lec_by_itfnum(int itf)
@@ -1521,8 +1525,11 @@ static void __exit atm_mpoa_cleanup(void)
 		mpc = tmp;
 	}
 
+	mutex_lock(&qos_mutex);
 	qos = qos_head;
 	qos_head = NULL;
+	mutex_unlock(&qos_mutex);
+
 	while (qos != NULL) {
 		nextqos = qos->next;
 		dprintk("freeing qos entry %p\n", qos);
diff --git a/net/atm/mpc.h b/net/atm/mpc.h
index 454abd07651a..35719b5c5e88 100644
--- a/net/atm/mpc.h
+++ b/net/atm/mpc.h
@@ -7,6 +7,7 @@
 #include <linux/atmmpc.h>
 #include <linux/skbuff.h>
 #include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include "mpoa_caches.h"
 
 /* kernel -> mpc-daemon */
@@ -54,6 +55,9 @@ int atm_mpoa_delete_qos(struct atm_mpoa_qos *qos);
 struct seq_file;
 void atm_mpoa_disp_qos(struct seq_file *m);
 
+/* Protect qos_head list */
+extern struct mutex qos_mutex;
+
 #ifdef CONFIG_PROC_FS
 int mpc_proc_init(void);
 void mpc_proc_clean(void);
diff --git a/net/atm/mpoa_proc.c b/net/atm/mpoa_proc.c
index aaf64b953915..b91676187dd1 100644
--- a/net/atm/mpoa_proc.c
+++ b/net/atm/mpoa_proc.c
@@ -253,8 +253,15 @@ static int parse_qos(const char *buff)
 
 	if (sscanf(buff, "del %hhu.%hhu.%hhu.%hhu",
 			ip, ip+1, ip+2, ip+3) == 4) {
+		struct atm_mpoa_qos *entry;
+		int ret;
+
 		ipaddr = *(__be32 *)ip;
-		return atm_mpoa_delete_qos(atm_mpoa_search_qos(ipaddr));
+		mutex_lock(&qos_mutex);
+		entry = atm_mpoa_search_qos(ipaddr);
+		ret = atm_mpoa_delete_qos(entry);
+		mutex_unlock(&qos_mutex);
+		return ret;
 	}
 
 	if (sscanf(buff, "add %hhu.%hhu.%hhu.%hhu tx=%d,%d rx=tx",
@@ -277,7 +284,9 @@ static int parse_qos(const char *buff)
 		qos.txtp.max_pcr, qos.txtp.max_sdu,
 		qos.rxtp.max_pcr, qos.rxtp.max_sdu);
 
+	mutex_lock(&qos_mutex);
 	atm_mpoa_add_qos(ipaddr, &qos);
+	mutex_unlock(&qos_mutex);
 	return 1;
 }
 
-- 
2.34.1

Re: [PATCH net v4] atm: mpoa: Fix UAF on qos_head list in procfs

[Simon Horman]

On Tue, Dec 16, 2025 at 09:09:10PM +0900, Minseong Kim wrote:
> /proc/net/atm/mpc read-side iterates qos_head without synchronization,
> while write-side can delete and free entries concurrently, leading to
> use-after-free.
> 
> Protect qos_head with a mutex and ensure procfs search+delete operations
> are serialized under the same lock.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Minseong Kim <ii4gsp@gmail.com>

...

> @@ -1521,8 +1525,11 @@ static void __exit atm_mpoa_cleanup(void)
>  		mpc = tmp;
>  	}
>  
> +	mutex_lock(&qos_mutex);
>  	qos = qos_head;
>  	qos_head = NULL;
> +	mutex_unlock(&qos_mutex);

I don't think this is necessary.

mpc_proc_clean() is called earlier in atm_mpoa_cleanup().  So I don't think
any accesses to the procfs callbacks can be occurring at this point. So
there is no need to guard against that.


Conversely the following call chain accesses, qos_head, and uses an entry
if found there. But there doesn't seem to be protection for concurrent
access (or removal) from procfs.

MPOA_res_reply_rcvd()->check_qos_and_open_shortcut->atm_mpoa_search_qos()

In this case I'm concerned that extending the current locking approach may
result in poor behaviour if procfs holds qos_mutex for an extended period.


And I think that there is also a concurrency issue with
access to qos_head in the following call chain.

mpc_show()->atm_mpoa_disp_qos()

...

-- 
pw-bot: changes-requested

Re: [PATCH net v3 0/1] atm: mpoa: Fix UAF on qos_head list in procfs

[Simon Horman]

On Thu, Dec 04, 2025 at 03:24:20PM +0900, Minseong Kim wrote:
> Changes since v2:
>   - Replace atm_mpoa_search_qos() with atm_mpoa_get_qos() to avoid
>     returning an unprotected pointer.
>   - Add atm_mpoa_delete_qos_by_ip() so search+unlink+free is atomic
>     under qos_mutex, preventing double-free/UAF.
>   - Update all callers accordingly.
> 
> Minseong Kim (1):
>   atm: mpoa: Fix UAF on qos_head list in procfs

Please slow down a bit and allow at least 24h between posting
updated patches. This is to allow time for review to occur.
And reduce load on shared CI infrastructure.

Please see: https://docs.kernel.org/process/maintainer-netdev.html

Re: [PATCH net v3 0/1] atm: mpoa: Fix UAF on qos_head list in procfs

[Minseong Kim]

Hi Simon,

Thanks for the reminder, and sorry for the quick repost.
I understand the 24h minimum interval rule and will follow it for
future revisions.

Best regards,
Minseong Kim

2025년 12월 4일 (목) PM 10:28, Simon Horman <horms@kernel.org>님이 작성:
>
> On Thu, Dec 04, 2025 at 03:24:20PM +0900, Minseong Kim wrote:
> > Changes since v2:
> >   - Replace atm_mpoa_search_qos() with atm_mpoa_get_qos() to avoid
> >     returning an unprotected pointer.
> >   - Add atm_mpoa_delete_qos_by_ip() so search+unlink+free is atomic
> >     under qos_mutex, preventing double-free/UAF.
> >   - Update all callers accordingly.
> >
> > Minseong Kim (1):
> >   atm: mpoa: Fix UAF on qos_head list in procfs
>
> Please slow down a bit and allow at least 24h between posting
> updated patches. This is to allow time for review to occur.
> And reduce load on shared CI infrastructure.
>
> Please see: https://docs.kernel.org/process/maintainer-netdev.html