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