Bug 206680 - kbd race attacks
Summary: kbd race attacks
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Mahdi Mokhtari
URL:
Keywords: needs-patch, security
Depends on:
Blocks:
 
Reported: 2016-01-27 15:45 UTC by CTurt
Modified: 2016-05-03 20:52 UTC (History)
5 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 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 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 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 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)