sound: Check user-supplied size passed to SNDSTIOC_ADD_USER_DEVS*

[christos]

Summary

SNDSTIOC_ADD_USER_DEVS* expects a user-supplied sndstioc_nv_arg->nbytes,
however we currently do not check whether this size is actually valid,
which results in a panic when SNDSTIOC_ADD_USER_DEVS* is called with an
invalid size. sndstat_add_user_devs() calls
sndstat_unpack_user_nvlbuf(), which then calls malloc() with that size.

PR: 266142
Sponsored by: The FreeBSD Foundation
MFC after: 3 days

Test Plan

Test program (stolen from the PR):

#include <sys/sndstat.h>

#include <err.h>
#include <fcntl.h>
#include <string.h>

int
main(int argc, char *argv[])
{
        char buf[128];
        int fd;

        if ((fd = open("/dev/sndstat", O_RDWR)) < 0)
                err(1, "open()");
        memset(buf, 0xff, sizeof(buf));
        if (ioctl(fd, SNDSTIOC_ADD_USER_DEVS, buf) < 0)
                err(1, "ioctl(SNDSTIOC_ADD_USER_DEVS)");

        return (0);
}

Without the patch we get the following panics:

1. With memset(0xff):

panic: Assertion size > 0 failed at /mnt/src/sys/kern/subr_vmem.c:1330
cpuid = 0
time = 1715986423
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe0046dd5110
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe0046dd5270
vpanic() at vpanic+0x210/frame 0xfffffe0046dd5410
panic() at panic+0xb5/frame 0xfffffe0046dd54d0
vmem_alloc() at vmem_alloc+0x137/frame 0xfffffe0046dd5510
kmem_malloc_domainset() at kmem_malloc_domainset+0x19f/frame 0xfffffe0046dd5650
malloc_large() at malloc_large+0x35/frame 0xfffffe0046dd5690
sndstat_ioctl() at sndstat_ioctl+0x233/frame 0xfffffe0046dd5790
devfs_ioctl() at devfs_ioctl+0x1f6/frame 0xfffffe0046dd5870
vn_ioctl() at vn_ioctl+0x235/frame 0xfffffe0046dd5a80
devfs_ioctl_f() at devfs_ioctl_f+0x6c/frame 0xfffffe0046dd5ad0
kern_ioctl() at kern_ioctl+0x3a1/frame 0xfffffe0046dd5bb0
sys_ioctl() at sys_ioctl+0x247/frame 0xfffffe0046dd5d10
amd64_syscall() at amd64_syscall+0x39e/frame 0xfffffe0046dd5f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe0046dd5f30
--- syscall (54, FreeBSD ELF64, ioctl), rip = 0x8211d68fa, rsp = 0x820b52bd8, rbp = 0x820b52c40 ---
KDB: enter: panic
[ thread pid 908 tid 100143 ]
Stopped at      kdb_enter+0x34: movq    $0,0x1f09d01(%rip)

2. Without the memset():

panic: pmap_san_enter_alloc_4k: no memory to grow shadow map
cpuid = 0
time = 1715986510
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe0046db8930
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe0046db8a90
vpanic() at vpanic+0x210/frame 0xfffffe0046db8c30
panic() at panic+0xb5/frame 0xfffffe0046db8cf0
pmap_san_enter_alloc_4k() at pmap_san_enter_alloc_4k+0x2f/frame 0xfffffe0046db8d00
pmap_san_enter() at pmap_san_enter+0x228/frame 0xfffffe0046db8d40
kasan_shadow_map() at kasan_shadow_map+0x98/frame 0xfffffe0046db8d60
pmap_growkernel() at pmap_growkernel+0x9d/frame 0xfffffe0046db8db0
vm_map_insert1() at vm_map_insert1+0x460/frame 0xfffffe0046db8ef0
vm_map_find() at vm_map_find+0x877/frame 0xfffffe0046db9050
kva_import() at kva_import+0xb5/frame 0xfffffe0046db9130
vmem_try_fetch() at vmem_try_fetch+0x1d0/frame 0xfffffe0046db9220
vmem_xalloc() at vmem_xalloc+0x6f8/frame 0xfffffe0046db92d0
kva_import_domain() at kva_import_domain+0x35/frame 0xfffffe0046db9310
vmem_try_fetch() at vmem_try_fetch+0x1d0/frame 0xfffffe0046db9400
vmem_xalloc() at vmem_xalloc+0x6f8/frame 0xfffffe0046db94b0
vmem_alloc() at vmem_alloc+0x97/frame 0xfffffe0046db9510
kmem_malloc_domainset() at kmem_malloc_domainset+0x19f/frame 0xfffffe0046db9650
malloc_large() at malloc_large+0x35/frame 0xfffffe0046db9690
sndstat_ioctl() at sndstat_ioctl+0x233/frame 0xfffffe0046db9790
devfs_ioctl() at devfs_ioctl+0x1f6/frame 0xfffffe0046db9870
vn_ioctl() at vn_ioctl+0x235/frame 0xfffffe0046db9a80
devfs_ioctl_f() at devfs_ioctl_f+0x6c/frame 0xfffffe0046db9ad0
kern_ioctl() at kern_ioctl+0x3a1/frame 0xfffffe0046db9bb0
sys_ioctl() at sys_ioctl+0x247/frame 0xfffffe0046db9d10
amd64_syscall() at amd64_syscall+0x39e/frame 0xfffffe0046db9f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe0046db9f30
--- syscall (54, FreeBSD ELF64, ioctl), rip = 0x8218ef8fa, rsp = 0x820490998, rbp = 0x820490a40 ---
KDB: enter: panic
[ thread pid 909 tid 100146 ]
Stopped at      kdb_enter+0x34: movq    $0,0x1f09d01(%rip)

Applying the patch makes both panics go away.

[christos]

sys/dev/sound/pcm/sndstat.c

971

Maybe we could return ERANGE instead?

[brooks]

You need to decide the type of data based on the command passed before you examine it so you'll need a case statement before the tests. I'd also tend not to assign the arg/arg32 until you've decided the type. Additionally, data will be NULL for at least SNDSTIOC_REFRESH_DEVS and SNDSTIOC_FLUSH_USER_DEVS,

[christos]

In D45236#1031986, @brooks wrote:

You need to decide the type of data based on the command passed before you examine it so you'll need a case statement before the tests. I'd also tend not to assign the arg/arg32 until you've decided the type. Additionally, data will be NULL for at least SNDSTIOC_REFRESH_DEVS and SNDSTIOC_FLUSH_USER_DEVS,

Good catch, thank you. Will update the diff shortly. :)

[christos]

Address brooks' comment.

[brooks]

More comments.

sys/dev/sound/pcm/sndstat.c

971

Does it actually make sense to check the GET case? If the programmer wants to allocate too much space that's their problem.

974

sizeof(size_t) * 8 - 1 seem both relatively small, oddly aligned, and magic. Usually we just care that we're not making a giant kernel allocation so can let things be big as long as they aren't too big.

Also, a size_t (or uint32_t below) can't be negative.

[christos]

sys/dev/sound/pcm/sndstat.c

974

What do you think a good max value would be?

As for the < 0 case, the test program actually makes nbytes be -1 for some reason:

sndstat_add_user_devs(): nbytes=-1

[christos]

Address brooks' comments.

[brooks]

SNDST_UNVLBUF_MAX is probably excessively large, but is fine as a safety measure.

[markj]

sys/sys/sndstat.h

80

IMO it'd make more sense to allow a power-of-2-sized buffer.