Skip to content

fix: make kstorage write_kstorage find+replace atomic - #309

Open
AYwlilwYA wants to merge 1 commit into
bmax121:mainfrom
AYwlilwYA:fix-kstorage-race
Open

AYwlilwYA wants to merge 1 commit into
bmax121:mainfrom
AYwlilwYA:fix-kstorage-race

Conversation

@AYwlilwYA

Copy link
Copy Markdown

Supersedes #293 (which was closed). Same one-line-of-thought patch, but this time with the answers
I should have given when asked "kstorage 在什么情况下会导致这样的情况" — plus field evidence and
the reason I believe the spinlock concern doesn't hold.

Problem

write_kstorage() finds an existing entry outside the group spinlock and replaces it inside:

rcu_read_lock();
hlist_for_each_entry_rcu(pos, bucket, hnode) {   // lookup OUTSIDE the lock
    if (pos->did == did) { old = pos; break; }
}
spin_lock(lock);
if (old) hlist_replace_rcu(&old->hnode, &new->hnode);   // replace INSIDE the lock
else     hlist_add_head_rcu(&new->hnode, bucket);
spin_unlock(lock);
rcu_read_unlock();

Two writers racing on the same did both capture the same old. The first hlist_replace_rcu()
sets old->pprev = LIST_POISON2; the second one then executes
rcu_assign_pointer(*(struct hlist_node **)old->pprev, new) — a write through the poisoned pointer.

Under what circumstances

su_add_allow_uid() is only a write_kstorage() on key uid (sucompat.c:72-83), so writing the same
uid twice sequentially is harmless. The bug needs two writers inside write_kstorage() for the
same did at the same time
. That is reachable: load_ap_package_config() (userd.c:1133) has
no locking of any kind in its body, and it is reachable from four independent entry points:

entry point site who calls it
report_user_event("post-fs-data") user_event.c:20 userspace event report
SUPERCALL_AP_LOAD_PACKAGE_CONFIG (0x100d) supercall.c:290-291 apd, directly
supercmd reload-cfg supercmd.c:466 apd
sucompat_init() sucompat.c:410-411 boot, writes uid 2000 and 0

plus a third writer SUPERCALL_SU_GRANT_UIDcall_grant_uid() (supercall.c:301-302), also apd-driven.

The kernel also has a cross-context entry: the LSM hooks after_security_path_rename /
after_security_inode_renamerefresh_packages_list_tmp_rename() (userd.c:1620+) fire when
/system/packages.list.tmp is renamed, i.e. in the context of whatever task performed the rename
(normally system_server writing /data/system/packages.list). So "install / uninstall / update a
package" and "apd (re-)authorizes" can genuinely overlap, and both walk the same list calling
su_add_allow_uid() for the same uid — uid 2000 appears in every one of those paths.

This is not exotic on the affected device: at boot the same batch of 5 uids is added twice within
2.6 s
([4.624261][4.624269], then [7.256972][7.256985]).

Field evidence

CPU: 5 PID: 7651 Comm: apd   Tainted: G  W  O
Unable to handle kernel paging request at virtual address dead000000000122
  ESR = 0x0000000096000044
  EC = 0x25: DABT (current EL), IL = 32 bits
pc : 0xffffffe7476af624   lr : 0xffffffe7476af618
Kernel panic - not syncing: Oops: Fatal exception
  • dead000000000122 is LIST_POISON2 (on arm64 the 0xdead… prefix comes from
    CONFIG_ILLEGAL_POINTER_VALUE) — a dereference of a poisoned list pointer.
  • ESR = 0x96000044WnR = 1: a write fault, matching
    rcu_assign_pointer(*(struct hlist_node **)old->pprev, new) inside hlist_replace_rcu().
  • The PC is inside KP Region, the caller frame inside KP Hook — consistent with
    hlist_replace_rcu() being static inline and folded into write_kstorage().
  • The line immediately before the panic is
    [+] KP D su_add_allow_uid: uid: 2000, to_uid: 0, sctx: u:r:magisk:s0, rc: 0.
  • The device has recorded 11 abnormal reboots.

Disassembly extracted from the device's own boot partition confirms the lookup is outside the lock
(link base 0xd000, running 0.13.3 build):

0x195c4  blr   x0                     ; rcu_read_lock()
0x195dc  ldr   x19, [x25, x1, lsl#3]  ; walk bucket — NO spinlock held here
0x195f8  b.ne  #0x195e8
0x19614  blr   x1                     ; spin_lock(lock)   ← taken only AFTER the lookup
0x19624  stlr  x21, [x1]              ; *old->pprev = new     ← the faulting store
0x19638  mov   x0, #0x122
0x1963c  movk  x0, #0xdead, lsl #48
0x19640  str   x0, [x19, #8]          ; old->pprev = LIST_POISON2
0x1964c  blr   x1                     ; spin_unlock
0x1965c  blr   x0                     ; rcu_read_unlock()

Fix

Move the lookup inside the group spinlock so find + replace/add are atomic under the same lock.
vmalloc() / memcpy() still happen before the lock (they may sleep).

On the spinlock / contention concern

  1. What moves inside the lock is a hash-bucket walk. KSTORAGE_NBUCKETS = 256
    (kstorage.c:21-22); on the affected device the su list holds 6 entries, so buckets are almost
    always empty and the walk is 0–2 nodes.
  2. The expensive parts (vmalloc(), memcpy()) are already outside the lock.
  3. remove_kstorage() already does exactly this — same kstorage_glocks[gid], bucket walked
    inside the lock (kstorage.c:243-249). "Lookup + mutate under the group spinlock" is already the
    established pattern in this file.

Writes to this group only happen on config change / authorization — not a hot path.

On ordering

The missing property is mutual exclusion, not ordering. hlist_replace_rcu() /
hlist_add_head_rcu() publish with rcu_assign_pointer (compiled to stlr) and readers use
rcu_dereference, so the ordering is already correct and no manual barrier is needed. The defect is
that the find is not covered by the same lock as the replace.

Happy to rework the shape (per-bucket locks, etc.) if you prefer — I'd just like the correctness fix in.

write_kstorage() looked up the existing node under rcu_read_lock() but
replaced it under the group spinlock. Two writers racing on the same did
could both find the same old node; the second hlist_replace_rcu() on an
already-replaced node wrote through its poisoned ->pprev (LIST_POISON2),
causing a kernel Oops (observed in apd during APatch re-authorization
after a system_server soft restart).

Move the lookup inside the spinlock so find+replace/add are atomic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant