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

[christos]

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

[markj]

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.

[christos]

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]

Actually fix bug.

[emaste]

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.

[christos]

Address Ed's comment.

[markj]

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.

[christos]

sys/dev/sound/pcm/dsp.c

2095

But even if we modify every PCM_* macro to check for d != NULL, this race can still happen. Is there a "standard" way to approach this issue?

[markj]

sys/dev/sound/pcm/dsp.c

2095

I think you need to synchronize with subr_bus.c. That is, wrap the lookup and checks with bus_topo_lock().

[christos]

sys/dev/sound/pcm/dsp.c

2095

Alright. I think it'd be better to do this in a separate patch.