Whilst analysing my previous bug about the missing `splx` call in one of the code paths for `genkbd_commonioctl` (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206678), I decided to look at the declaration for the macro itself, in `sys/sys/systm.h`: /* Stubs for obsolete functions that used to be for interrupt management */ static __inline intrmask_t splbio(void) { return 0; } static __inline intrmask_t splcam(void) { return 0; } static __inline intrmask_t splclock(void) { return 0; } static __inline intrmask_t splhigh(void) { return 0; } static __inline intrmask_t splimp(void) { return 0; } static __inline intrmask_t splnet(void) { return 0; } static __inline intrmask_t spltty(void) { return 0; } static __inline void splx(intrmask_t ipl __unused) { return; } Since these functions have been removed, but kbd has not been updated to account for this, it means that none of the kbd code is thread safe. For example, `kbd_realloc_array` contains a lovely, possible race attack: s = spltty(); newsize = ((keyboards + ARRAY_DELTA)/ARRAY_DELTA)*ARRAY_DELTA; new_kbd = malloc(sizeof(*new_kbd)*newsize, M_DEVBUF, M_NOWAIT|M_ZERO); if (new_kbd == NULL) { splx(s); return (ENOMEM); } new_kbdsw = malloc(sizeof(*new_kbdsw)*newsize, M_DEVBUF, M_NOWAIT|M_ZERO); if (new_kbdsw == NULL) { free(new_kbd, M_DEVBUF); splx(s); return (ENOMEM); } bcopy(keyboard, new_kbd, sizeof(*keyboard)*keyboards); bcopy(kbdsw, new_kbdsw, sizeof(*kbdsw)*keyboards); if (keyboards > 1) { free(keyboard, M_DEVBUF); free(kbdsw, M_DEVBUF); } keyboard = new_kbd; kbdsw = new_kbdsw; keyboards = newsize; splx(s); This code would have been safe because of the `spltty`, and `splx` locks, but since these functions no longer do anything, we have a very brief window where the buffers have been freed, but have not been set to the new allocations. If the thread were to be preempted at this point, and another thread attempted to use the buffers, a use after free would occur.
(In reply to CTurt from comment #0) So you think we should have locks for ensuring 'new_kbd', 'keyboard' and 'kbdsw' are being used thread-safe, yes?
Correct. I cited `kbd_realloc_array` in my comment because it is the most blatant example of code which is no longer thread safe, following the removal of `spltty` and `splx`. However, it is not just `kbd_realloc_array` which needs to be sorted. The whole of this source file seems to rely on using `spltty` to prevent race attacks, which means multiple locks will probably need to be used.
(In reply to CTurt from comment #2) Okay, got it :D Thanks for your clear comments, sure it'll help us to make this file thread-safe again.
It is not just kbd, mouse handling code is vulnerable too. An example being `mouse_cut` from `/sys/dev/syscons/scmouse.c`: s = spltty(); scp->mouse_cut_start = start; scp->mouse_cut_end = end; splx(s); These calls should be replaced with locks.
(In reply to CTurt from comment #4) Adding locks/unlocks to these function seems no problem to me :) But before that I need to talk with who removed them to know their reasons and their probably alternative solutions too :D
Any movement on this?
(In reply to Shawn Webb from comment #6) Yes i'm placing lock/unlock where we should (and needed)
All 8042 stuff (intr/cdev) is executed with Giant lock taken. What extra protection inside dev/atkbdc is required?
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=a0cd78bf2c148d7ab63454471771e3f4b572522f commit a0cd78bf2c148d7ab63454471771e3f4b572522f Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-03-22 17:48:43 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-03-24 16:56:29 +0000 kbd: replace vestigial spl calls with Giant assertions The keyboard driver was initially protected via spl* interrupt priority calls but (as part of a comprehensive effort) migrated to use the Giant lock (mutex). The spl calls left behind became NOPs but they can be confusing as they have no bearing on the actual mutual exclusion that is now present. Remove them from kbd and add assertions that Giant is held. markj notes that there is conflation between the "bus topo" lock (which is Giant under the hood) and Giant. The assertions could either be addressed as a small item along with bus topology locking work or they'll be removed if kbd is decoupled from Giant. PR: 206680 Reviewed by: markj MFC after: 3 weeks Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34645 sys/dev/kbd/kbd.c | 67 ++++++++++--------------------------------------------- 1 file changed, 12 insertions(+), 55 deletions(-)
FWIW the stubs still left in main now are: splhigh (11 files) splimp (13) splnet (13) spltty (12)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3ec2816ad7a19dc2ae21ccd2d2d2c236027e4572 commit 3ec2816ad7a19dc2ae21ccd2d2d2c236027e4572 Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-03-22 17:48:43 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-04-15 16:29:02 +0000 kbd: replace vestigial spl calls with Giant assertions The keyboard driver was initially protected via spl* interrupt priority calls but (as part of a comprehensive effort) migrated to use the Giant lock (mutex). The spl calls left behind became NOPs but they can be confusing as they have no bearing on the actual mutual exclusion that is now present. Remove them from kbd and add assertions that Giant is held. markj notes that there is conflation between the "bus topo" lock (which is Giant under the hood) and Giant. The assertions could either be addressed as a small item along with bus topology locking work or they'll be removed if kbd is decoupled from Giant. PR: 206680 Reviewed by: markj MFC after: 3 weeks Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34645 (cherry picked from commit a0cd78bf2c148d7ab63454471771e3f4b572522f) sys/dev/kbd/kbd.c | 67 ++++++++++--------------------------------------------- 1 file changed, 12 insertions(+), 55 deletions(-)
^Triage: assign to committer who resolved this in 2022.