While reviewing a preview patch for https://bugs.chromium.org/p/project-zero/issues/detail?id=2540 , I noticed some issues - most of them minor, but the following two seem like they probably have bigger security impact:


** F.5 **
After _PmrZombieCleanup() has picked an item from the sZombieList, it
drops the hPMRZombieListLock temporarily to avoid a deadlock. When the
hPMRZombieListLock has been retaken, _PmrZombieCleanup() tries to make
sure that the PMR is still a zombie, but it does that by checking
PMR_IsZombie(); so if another thread concurrently revives the zombie
with PMRDequeueZombieAndRef(), maps it on the GPU, unmaps it, and
zombifies it again, _PmrZombieCleanup() could wrongly assume that the
PMR stayed a zombie while the lock was dropped, and destroy it
immediately.
In other words, the problem is that after retaking the lock,
_PmrZombieCleanup() doesn't check whether the zombie is still on the
same list as before. Instead of rechecking PMR_IsZombie(), you might
want to recheck that
`dllist_get_next_node(&psCleanupItem->sZombieList) == psNode`.
It might also be nice to have a comment in _PmrZombieCleanup() describing this.


** F.9 **
The refactor of the bools in `struct _PMR_` into the flags bitfield
uiInternalFlags looks racy to me (though I think really the old code
was theoretically wrong already, the refactor is probably just making
that worse): Bits in this field are updated with BITMASK_SET() /
BITMASK_UNSET(), which are non-atomic; and the callers of these
helpers don't seem to be using any other locking to avoid concurrent
writes to this field. In theory, this kind of concurrency is
forbidden; in practice, the consequence is that updates can get lost
when two threads try to update different parts of the bitfield, like
so:

thread A: reads old value of uiInternalFlags (0) thread B: reads old
value of uiInternalFlags (0) thread A: writes observed value ORed with
PMR_FLAG_INTERNAL_NO_LAYOUT_CHANGE
thread B: writes observed value ORed with PMR_FLAG_INTERNAL_DEFER_FREE

causing the final value of uiInternalFlags to be
PMR_FLAG_INTERNAL_DEFER_FREE, with thread A's write having been lost.

There are two ways around this: Either ensure all places that access
this flags field hold the same lock, or use atomic RMW operations
(such as set_bit() on Linux, though note that that's a relaxed atomic
operation and I haven't checked whether any of your uses of these
flags would require stronger memory ordering).


Marked as fixed.
The fix is public at https://chromium.googlesource.com/chromiumos/third_party/kernel/+/b61103dc9ad5aa2ae3229dfa64066ed9414abe2e%5E%21/ .

F.5 has been addressed by changing the recheck-after-relock in _PmrZombieCleanup() to use `psNode == dllist_get_next_node(&psCleanupItem->sZombieList)` instead of `PMR_IsZombie(psPMR)`.

F.9 has been addressed with the introduction of _IntFlagSet() / _IntFlagClr() / _IntFlagIsSet() for atomic bitops on the ->uiInternalFlags. (It looks like PMRPinPMR/PMRUnpinPMR/PMR_IsUnpinned weren't changed, but at least in ChromeOS, PMRPinPMR/PMRUnpinPMR seem to be dead code.)


Related CVE Number: CVE-2024-40670

Credit: Jann Horn