Created attachment 232581 [details] A patch that should fix brought up issue. If you call the `SNDCTL_MIXERINFO` ioctl with a `oss_mixerinfo` structure which has a uninitialized `dev` field, a kernel panic is triggered. I know that in this case the `dev` field is expected to be set, but I still think that the kernel should not panic if a malicious/bad program forgets to set it.
Created attachment 232582 [details] Example program that exposes the issue
Can you please paste the backtrace from the kernel panic here? I do not see how an uninitialized dev field would trigger a panic (and indeed the sample program just returns from the ioctl with EINVAL here).
#0 __curthread () at /usr/src/sys/amd64/include/pcpu_aux.h:55 #1 doadump (textdump=textdump@entry=1) at /usr/src/sys/kern/kern_shutdown.c:406 #2 0xffffffff80c17dbc in kern_reboot (howto=260) at /usr/src/sys/kern/kern_shutdown.c:527 #3 0xffffffff80c182ce in vpanic (fmt=0xffffffff81215450 "%s", ap=<optimized out>) at /usr/src/sys/kern/kern_shutdown.c:965 #4 0xffffffff80c18023 in panic (fmt=<unavailable>) at /usr/src/sys/kern/kern_shutdown.c:889 #5 0xffffffff810fee55 in trap_fatal (frame=0xfffffe00ce68b9c0, eva=68) at /usr/src/sys/amd64/amd64/trap.c:946 #6 0xffffffff810fef0b in trap_pfault (frame=0xfffffe00ce68b9c0, usermode=false, signo=<optimized out>, ucode=<optimized out>) at /usr #7 <signal handler called> #8 mixer_oss_mixerinfo (i_dev=0xfffff80004865c00, mi=0xfffff80010e01800, mi@entry=0x0) at /usr/src/sys/dev/sound/pcm/mixer.c:1465 #9 0xffffffff809a3941 in mixer_ioctl_cmd (i_dev=0xfffff80001a33f00, i_dev@entry=0xfffff80004865c00, cmd=<optimized out>, cmd@entry=32 from@entry=1) at /usr/src/sys/dev/sound/pcm/mixer.c:1301 #10 0xffffffff809a48db in mixer_ioctl (i_dev=0xfffff80004865c00, cmd=<optimized out>, arg=0xffffffff8126e1cb "/usr/src/sys/dev/sound/p #11 0xffffffff80aa631c in devfs_ioctl (ap=0xfffffe00ce68bba8) at /usr/src/sys/fs/devfs/devfs_vnops.c:935 #12 0xffffffff80d1f871 in vn_ioctl (fp=0xfffff801600ec9b0, com=<optimized out>, data=0xfffff80010e01800, active_cred=0xfffff801e1934a0 #13 0xffffffff80aa69ce in devfs_ioctl_f (fp=0xfffff80001a33f00, com=4, data=0xffffffff8126e1cb, cred=0x4, td=0xfffffe00d0f14e40) at /u #14 0xffffffff80c8f842 in fo_ioctl (fp=<optimized out>, com=3295696906, data=0x10000, active_cred=0x4, td=0xfffffe00d0f14e40) at /usr/ #15 kern_ioctl (td=<optimized out>, td@entry=0xfffffe00d0f14e40, fd=<optimized out>, com=com@entry=3295696906, data=0x10000 <error: Ca #16 0xffffffff80c8f596 in sys_ioctl (td=0xfffffe00d0f14e40, uap=0xfffffe00d0f15230) at /usr/src/sys/kern/sys_generic.c:711 #17 0xffffffff810ff80e in syscallenter (td=<optimized out>) at /usr/src/sys/amd64/amd64/../../kern/subr_syscall.c:189 #18 amd64_syscall (td=0xfffffe00d0f14e40, traced=0) at /usr/src/sys/amd64/amd64/trap.c:1191 #19 <signal handler called> #20 0x00000008215c4baa in ?? () Backtrace stopped: Cannot access memory at address 0x8208dd548
Perhaps try setting the `dev` field to some irrational value like `-5`? This will ensure that the value is invalid and maybe trigger the panic more reliably.
We only compare against mi->dev so it shouldn't matter if it's an invalid value: if (d->mixer_dev != NULL && d->mixer_dev->si_drv1 != NULL && ((mi->dev == -1 && d->mixer_dev == i_dev) || mi->dev == nmix)) { Indeed setting it to an explicit invalid value is fine, ktrace shows: 36100 a.out CALL ioctl(0x3,SNDCTL_MIXERINFO,0x820ea1188) 36100 a.out RET ioctl -1 errno 22 Invalid argument 36100 a.out CALL exit(0) What version (git hash if on main) are you using?
I'm on commit 1b3bef43e3cb7fb0ab49b813839915514c1134cc. kgdb shows that the panic happens on this line: 1463 if (PCM_DETACHING(d) || !PCM_REGISTERED(d)) So for some reason when I specify a invalid `dev` value, `d` is still equal to `NULL` after this statement: 1462 d = devclass_get_softc(pcm_devclass, i);
OK, I think I know what's going on. So It looks like in my case `devclass_get_maxunit` is returning a bigger value than the actual number of devices. That's why on the 4th iteration of the `for` loop `devclass_get_softc` returns `NULL` (I do not have a mixer device with the unit number 4) `mixer -a` returns this: pcm0:mixer: <Realtek ALC892 (Rear Analog 7.1/2.0)> on hdaa0 (play/rec) (default) ... pcm1:mixer: <Realtek ALC892 (Front Analog)> on hdaa0 (play/rec) ... pcm2:mixer: <Realtek ALC892 (Onboard Digital)> on hdaa0 (play) ... pcm3:mixer: <Intel Kaby Lake (HDMI/DP 8ch)> on hdaa1 (play) ... The highest unit number here is 3, but when I checked in kgdb `devclass_get_maxunit(pcm_devclass)` returns the value `5`.
> The highest unit number here is 3, but when I checked in kgdb `devclass_get_maxunit(pcm_devclass)` returns the value `5`. To clarify things - `devclass_get_maxunit(pcm_devclass)` should return 4 since I have only 4 mixer devices, but it returns 5. Now the problem is that I have no idea how to debug why `devclass_get_maxunit(pcm_devclass)` is returning the wrong value.
(In reply to Aleksander Slomka from comment #8) Indeed; trying successive mi.dev values here is fine: #include <fcntl.h> #include <stdio.h> #include <stdlib.h> #include <sys/soundcard.h> #include <unistd.h> int main() { int fd; fd = open("/dev/mixer", O_RDWR); oss_mixerinfo mi; for (mi.dev = 0; mi.dev < 100; mi.dev++) ioctl(fd, SNDCTL_MIXERINFO, &mi); return (0); }
Created attachment 232613 [details] possible patch
If this is readily reproducible for you could you please give the attached patch a try?
(In reply to Ed Maste from comment #11) Thank you for the help! The patch indeed solves the issue for me. Now the `ioctl` properly returns `EINVAL`: 31415 test CALL ioctl(0x3,SNDCTL_MIXERINFO,0x8210902e0) 31415 test RET ioctl -1 errno 22 Invalid argument It makes sense that this works, since PCM_REGISTERED is checking if it's argument is equal to NULL, just like my patch did, but I don't understand why the `if` statement didn't work when the condition was in reverse order.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=da03ac41c9bca270b491fcf4bf219c4108688a05 commit da03ac41c9bca270b491fcf4bf219c4108688a05 Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-03-21 16:15:22 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-03-24 00:10:00 +0000 sound: test PCM_REGISTERED before PCM_DETACHING PCM_REGISTERED(d) tests that d is not NULL, so perform that check first as we may have cases where devclass_get_softc has a null entry. PR: 262671 Reviewed by: hselasky MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34648 sys/dev/sound/pcm/mixer.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
> why the `if` statement didn't work when the condition was in reverse order The || operator performs short-circuit evaluation - if the first condition is true the second is not evaluated. I am still curious why there's an extra NULL entry in your case. If you're willing I may have a patch in the future to show some diagnostic information for this case.
(In reply to Ed Maste from comment #14) > The || operator performs short-circuit evaluation Thanks for the explanation, this makes much more sense now :^) > If you're willing I may have a patch in the future to show some diagnostic information for this case. Sure, that would be great.
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=74f11a40f8e4e1b3ae254edf499d467153242ce9 commit 74f11a40f8e4e1b3ae254edf499d467153242ce9 Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-03-21 16:15:22 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-03-30 20:43:57 +0000 sound: test PCM_REGISTERED before PCM_DETACHING PCM_REGISTERED(d) tests that d is not NULL, so perform that check first as we may have cases where devclass_get_softc has a null entry. PR: 262671 Reviewed by: hselasky MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34648 (cherry picked from commit da03ac41c9bca270b491fcf4bf219c4108688a05) sys/dev/sound/pcm/mixer.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=aeb4214375b7197ccc7c6ad9b2bc3185545eaf8e commit aeb4214375b7197ccc7c6ad9b2bc3185545eaf8e Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-03-21 16:15:22 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-03-31 00:31:44 +0000 sound: test PCM_REGISTERED before PCM_DETACHING PCM_REGISTERED(d) tests that d is not NULL, so perform that check first as we may have cases where devclass_get_softc has a null entry. PR: 262671 Reviewed by: hselasky MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34648 (cherry picked from commit da03ac41c9bca270b491fcf4bf219c4108688a05) sys/dev/sound/pcm/mixer.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
^Triage: committed and MFCed.