Bug 206680 - kbd spl calls obscure locking
Summary: kbd spl calls obscure locking
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Ed Maste
URL:
Keywords: needs-patch, security
Depends on:
Blocks:
 
Reported: 2016-01-27 15:45 UTC by CTurt
Modified: 2024-01-07 12:08 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-01-27 15:45:10 UTC
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.
Comment 1 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-01-27 18:43:02 UTC
(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?
Comment 2 CTurt 2016-01-27 18:49:25 UTC
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.
Comment 3 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-01-27 18:52:14 UTC
(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.
Comment 4 CTurt 2016-01-28 22:54:17 UTC
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.
Comment 5 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-01-29 16:40:31 UTC
(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
Comment 6 Shawn Webb 2016-03-01 20:59:14 UTC
Any movement on this?
Comment 7 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-03-02 11:06:06 UTC
(In reply to Shawn Webb from comment #6)
Yes i'm placing lock/unlock where we should (and needed)
Comment 8 Vladimir Kondratyev freebsd_committer freebsd_triage 2018-01-21 23:37:34 UTC
All 8042 stuff (intr/cdev) is executed with Giant lock taken. What extra protection inside dev/atkbdc is required?
Comment 9 commit-hook freebsd_committer freebsd_triage 2022-03-24 16:57:11 UTC
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(-)
Comment 10 Ed Maste freebsd_committer freebsd_triage 2022-03-25 00:01:48 UTC
FWIW the stubs still left in main now are:

splhigh (11 files)
splimp (13)
splnet (13)
spltty (12)
Comment 11 commit-hook freebsd_committer freebsd_triage 2022-04-15 16:33:41 UTC
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(-)
Comment 12 Mark Linimon freebsd_committer freebsd_triage 2024-01-07 12:08:46 UTC
^Triage: assign to committer who resolved this in 2022.