Message-ID: <CAKrymDR1X3XTX_1ZW3XXXnuYH+kzsnv7Av5uivzR1sto+5BFQg@mail.gmail.com>
Date: Wed, 3 Dec 2025 17:57:11 +0900
From: Minseong Kim <ii4gsp@...il.com>
To: netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, stable@...r.kernel.org, 
	Eric Dumazet <edumazet@...gle.com>
Subject: [PATCH net] atm: mpoa: Fix UAF on qos_head list in procfs

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.

This risk is already called out in-tree:
  /* this is buggered - we need locking for qos_head */

Fix this by adding a mutex to protect all qos_head list operations.
A mutex is used (instead of a spinlock) because atm_mpoa_disp_qos()
invokes seq_printf(), which may sleep.

The fix:
  - Adds qos_mutex protecting qos_head
  - Introduces __atm_mpoa_search_qos() requiring the mutex
  - Serializes add/search/delete/show/cleanup on qos_head
  - Re-checks qos_head under lock in add path to avoid duplicates under
    concurrent additions
  - Uses a single-exit pattern in delete for clarity

Note: atm_mpoa_search_qos() still returns an unprotected pointer; callers
must ensure the entry is not freed while using it, or hold qos_mutex.

Reported-by: Minseong Kim <ii4gsp@...il.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@...r.kernel.org
Signed-off-by: Minseong Kim <ii4gsp@...il.com>
---
 net/atm/mpc.c | 60 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index 324e3ab96bb393..12da0269275c54 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -123,6 +123,7 @@ static struct llc_snap_hdr llc_snap_mpoa_data_tagged = {

 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);

@@ -175,23 +176,45 @@ static struct mpoa_client
*find_mpc_by_lec(struct net_device *dev)
 /*
  * Functions for managing QoS list
  */

+/*
+ * Search for a QoS entry. Caller must hold qos_mutex.
+ * Returns pointer to entry if found, NULL otherwise.
+ */
+static struct atm_mpoa_qos *__atm_mpoa_search_qos(__be32 dst_ip)
+{
+ struct atm_mpoa_qos *qos = qos_head;
+
+ while (qos) {
+ if (qos->ipaddr == dst_ip)
+ return qos;
+ qos = qos->next;
+ }
+ return NULL;
+}
+
+/*
+ * Search for a QoS entry.
+ * WARNING: The returned pointer is not protected. The caller must ensure
+ * that the entry is not freed while using it, or hold qos_mutex during use.
+ */
 struct atm_mpoa_qos *atm_mpoa_search_qos(__be32 dst_ip)
 {
  struct atm_mpoa_qos *qos;

- qos = qos_head;
- while (qos) {
- if (qos->ipaddr == dst_ip)
- break;
- qos = qos->next;
- }
+ mutex_lock(&qos_mutex);
+ qos = __atm_mpoa_search_qos(dst_ip);
+ mutex_unlock(&qos_mutex);

  return qos;
 }

 /*
  * 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;

- entry = atm_mpoa_search_qos(dst_ip);
- if (entry != NULL) {
+ /* 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);

- entry = kmalloc(sizeof(struct atm_mpoa_qos), GFP_KERNEL);
- if (entry == NULL) {
+ /* Allocate outside lock */
+ new = kmalloc(sizeof(*new), GFP_KERNEL);
+ if (!new) {
  pr_info("mpoa: out of memory\n");
- return entry;
+ return NULL;
  }

- entry->ipaddr = dst_ip;
- entry->qos = *qos;
+ new->ipaddr = dst_ip;
+ new->qos = *qos;

+ /* Re-check under lock to avoid duplicates */
  mutex_lock(&qos_mutex);
- entry->next = qos_head;
- qos_head = entry;
+ 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 entry;
+ return new;
 }

 /*
  * Returns 0 for failure, 1 for success
  */
 int atm_mpoa_delete_qos(struct atm_mpoa_qos *entry)
 {
  struct atm_mpoa_qos *curr;
+ int ret = 0;

  if (entry == NULL)
  return 0;

+ mutex_lock(&qos_mutex);
+
  if (entry == qos_head) {
  qos_head = qos_head->next;
- kfree(entry);
- return 1;
+ ret = 1;
+ goto out_free;
  }

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

- return 0;
+out:
+ mutex_unlock(&qos_mutex);
+ return ret;
+
+out_free:
+ mutex_unlock(&qos_mutex);
+ kfree(entry);
+ 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;

  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",
@@ -250,6 +273,7 @@ void atm_mpoa_disp_qos(struct seq_file *m)
     qos->qos.rxtp.max_cdv,
     qos->qos.rxtp.max_sdu);
  qos = qos->next;
  }
+ mutex_unlock(&qos_mutex);
 }

 static struct net_device *find_lec_by_itfnum(int itf)
@@ -1524,8 +1548,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);
--

Powered by blists - more mailing lists