sound: Fix NULL dereference in dsp_clone() and mixer_clone()
ClosedPublic
Actions

Authored by christos on Apr 24 2024, 3:05 AM.

Details

Reviewers

markj
emaste

Commits

rGcc72812c49ba: sound: Fix NULL dereference in dsp_clone() and mixer_clone()
rG074d6fbebc16: sound: Fix NULL dereference in dsp_clone() and mixer_clone()

Summary

If we only have a single soundcard attached and we detach it right
before entering [dsp|mixer]_clone(), there is a chance pcm_unregister()
will have returned already, meaning it will have set snd_unit to -1, and
thus devclass_get_softc() will return NULL here.

While here, 1) move the calls to dsp_destroy_dev() and mixer_uninit()
below the point where we unset SD_F_REGISTERED, and 2) follow what
mixer_clone() does and make sure we don't use a NULL d->dsp_dev in
dsp_clone().

Reported by: KASAN
Sponsored by: The FreeBSD Foundation
MFC after: 1 day

Test Plan

I could easily reproduce this by running:

for i in $(seq 1 15); do cat /dev/dsp >/dev/null & cat /dev/random >/dev/dsp & done

and unplugging as quickly as possible. There is a short time window where this would cause a panic:

root@freebsd:~ # ./test
ugen0.4: <Focusrite Scarlett Solo USB> at usbus0 (disconnected)
uaudio0: at uhub0, port 1, addr 7 (disconnected)
cat: /dev/dsp: Device not configured
panic: ASan: Invalid access, 4-byte read at 0xfffffe005cb9645c, UMAUseAfterFree(fd)
cpuid = 1
time = 1713930201
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe0046d4fad0
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe0046d4fc30
vpanic() at vpanic+0x210/frame 0xfffffe0046d4fdd0
panic() at panic+0xb5/frame 0xfffffe0046d4fea0
kasan_code_name() at kasan_code_name/frame 0xfffffe0046d4ff70
dev_ref() at dev_ref+0x46/frame 0xfffffe0046d4ff90
devfs_lookup() at devfs_lookup+0x76c/frame 0xfffffe0046d502b0
vfs_lookup() at vfs_lookup+0x9bb/frame 0xfffffe0046d50530
namei() at namei+0x659/frame 0xfffffe0046d506b0
vn_open_cred() at vn_open_cred+0x33b/frame 0xfffffe0046d50a30
openatfp() at openatfp+0x52b/frame 0xfffffe0046d50c90
sys_openat() at sys_openat+0x81/frame 0xfffffe0046d50cd0
filemon_wrapper_openat() at filemon_wrapper_openat+0x19/frame 0xfffffe0046d50d10
amd64_syscall() at amd64_syscall+0x39e/frame 0xfffffe0046d50f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe0046d50f30
--- syscall (499, FreeBSD ELF64, openat), rip = 0x3de8f7c0a6da, rsp = 0x3de8f4da2308, rbp = 0x3de8f4da23f0 ---
KDB: enter: panic
[ thread pid 1061 tid 100134 ]
Stopped at      kdb_enter+0x34: movq    $0,0x1f09d01(%rip)
db>

A similar panic will occur with:

for i in $(seq 1 40); do cat /dev/mixer & done

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57415
Build 54303: arc lint + arc unit

Event Timeline

christos created this revision.Apr 24 2024, 3:05 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 24 2024, 3:05 AM

christos requested review of this revision.Apr 24 2024, 3:05 AM

Harbormaster completed remote builds in B57321: Diff 137594.Apr 24 2024, 3:05 AM

christos edited the test plan for this revision. (Show Details)Apr 24 2024, 3:10 AM

Is it a use-after-free? Looks like a plain NULL pointer dereference. (I guess KASAN will flag those as KASAN violations, but it looks like a non-KASAN kernel would hit this too.)

How does d->dsp_dev get set to NULL? dsp_destroy_dev() doesn't do that.

In D44924#1024323, @markj wrote:

Is it a use-after-free? Looks like a plain NULL pointer dereference. (I guess KASAN will flag those as KASAN violations, but it looks like a non-KASAN kernel would hit this too.)

How does d->dsp_dev get set to NULL? dsp_destroy_dev() doesn't do that.

I think what is happening is that because I detach the device immediately and I am creating 30 channels, during that time window the device might get destroyed before all channels are created, so at some point we try to dev_ref() a destroyed d->dsp_dev. This doesn't occur if I wait until all channels are created, which makes me think this theory is correct.

christos retitled this revision from sound: Fix use-after-free in dsp_clone() to sound: Fix NULL dereference in dsp_clone() and mixer_clone().Apr 25 2024, 5:47 PM

christos edited the summary of this revision. (Show Details)

christos edited the test plan for this revision. (Show Details)

Actually fix bug.

Harbormaster completed remote builds in B57370: Diff 137682.Apr 25 2024, 5:50 PM

emaste added inline comments.Apr 25 2024, 6:56 PM

sys/dev/sound/pcm/dsp.c
2094–2096	I think this is one case where short-circuit `\|\|` (or `&&`) is ok, e.g. `if (d == NULL \|\| !PCM_REGISTERED(d) \|\| ...` if you're so inclined. But it's also fine if you prefer this form.

Address Ed's comment.

Harbormaster completed remote builds in B57415: Diff 137755.Apr 27 2024, 4:31 PM

markj added inline comments.Apr 27 2024, 11:12 PM

sys/dev/sound/pcm/dsp.c
2095	Sorry, I'm still missing something. How do you know that `d` is still a valid pointer? That is, how do you know that the memory that `d` points to hasn't already been freed? Suppose you call devclass_get_softc() and get a non-NULL return value. Then suppose an interrupt happens, and another thread is scheduled, and while that's happening, pcm_unregister() returns and uaudio_detach() (or whatever other driver) completes. At that point I think the softc will be freed. Now suppose execution resumes here: we'll check `PCM_REGISTERED(d)`, but `d` is a pointer to freed memory. I can't see what prevents that race from happening.