Bug 206678 - OGIO_KEYMAP command does not restore priority level
Summary: OGIO_KEYMAP command does not restore priority level
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Nikolai Lifanov
URL: https://reviews.freebsd.org/D5095
Keywords: dogfood, patch, security
Depends on:
Blocks:
 
Reported: 2016-01-27 15:05 UTC by CTurt
Modified: 2017-01-17 22:03 UTC (History)
6 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:05:42 UTC
`genkbd_commonioctl` function from `sys/dev/kbd/kbd.c` begins by calling `spltty()` to block hard interrupts from TTY:

int
genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
{
	keymap_t *mapp;
	okeymap_t *omapp;
	keyarg_t *keyp;
	fkeyarg_t *fkeyp;splx
	int s;
	int i, j;
	int error;

	s = spltty();
	switch (cmd) {

It should always restore the original priority level using the `splx` function before returning. For example at the end of the function:

	splx(s);
	return (0);
}

And for any commands which need to return early:

	case GIO_KEYMAP:	/* get keyboard translation table */
		error = copyout(kbd->kb_keymap, *(void **)arg,
		    sizeof(keymap_t));
		splx(s);
		return (error);

The problem is that for the `OGIO_KEYMAP` command, this does not happen:

	case OGIO_KEYMAP:	/* get keyboard translation table (compat) */
		mapp = kbd->kb_keymap;
		omapp = (okeymap_t *)arg;
		omapp->n_keys = mapp->n_keys;
		for (i = 0; i < NUM_KEYS; i++) {
			for (j = 0; j < NUM_STATES; j++)
				omapp->key[i].map[j] =
				    mapp->key[i].map[j];
			omapp->key[i].spcl = mapp->key[i].spcl;
			omapp->key[i].flgs = mapp->key[i].flgs;
		}
		return (0);

My guess is that since this is a compatibility command, it was copied into here from somewhere else, which is why the call to `splx` is missing.
Comment 1 CTurt 2016-01-27 15:29:21 UTC
The solution is to replace the `return` with `break`:

https://github.com/HardenedBSD/hardenedBSD-playground/commit/9962f56bb8f942cf03a65b46e96ef26369571dec
Comment 3 CTurt 2016-01-27 18:21:33 UTC
Just wanted to clarify that since `spltty` and `splx` actually do nothing (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206680), this isn't actually a bug, but I'd still like to see it patched as a matter of code correctness.

The real problem which needs to be addressed is the race conditions caused by the removal of `splx`, especially in `kbd_realloc_array` (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206680).
Comment 4 Mahdi Mokhtari freebsd_committer 2016-01-27 18:25:02 UTC
(In reply to CTurt from comment #3)
So you mean still patch is needed for 'break;' instead of 'return (0);'?
And can you please clarify what do you mean by 'do nothing' exactly ?
And how race condition occurs?

if it's possible each one in its 'file bug' :D
Thanks, Mokhi.
Comment 5 CTurt 2016-01-27 18:32:53 UTC
Sorry if I wasn't clear.

The patch from `return` to `break` will correct the code so that it always calls `splx` after calling `spltty`.

However, the fact that `splx` isn't called doesn't actually matter, since `splx` doesn't do anything anyway.

As I said, it would be nice to get this patched anyway so that the behaviour is consistent to prevent further confusion (alternatively, the `spltty` and `splx` calls could be removed from `genkbd_commonioctl` altogether).

For reference, the macros are defined 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; }

The race condition I refer to is this bug which I filed:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206680
Comment 6 Mahdi Mokhtari freebsd_committer 2016-01-27 18:44:06 UTC
Thanks for clarifying this bug/issue. :)
Comment 7 Shawn Webb 2016-03-01 20:58:34 UTC
Any movement on this?
Comment 8 Mahdi Mokhtari freebsd_committer 2016-03-02 11:07:54 UTC
(In reply to Shawn Webb from comment #7)
From patch point of view it's done.
Just waiting to be committed.
Comment 9 commit-hook freebsd_committer 2017-01-07 15:59:05 UTC
A commit references this bug:

Author: lifanov
Date: Sat Jan  7 15:58:57 UTC 2017
New revision: 311650
URL: https://svnweb.freebsd.org/changeset/base/311650

Log:
  Restore priority value for OGIO_KEYMAP

  PR:		206678
  Submitted by:	ecturt@gmail.com
  Reviewed by:	cem
  Approved by:	cem, matthew (mentor)
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D5095

Changes:
  head/sys/dev/kbd/kbd.c
Comment 10 Nikolai Lifanov freebsd_committer 2017-01-07 15:59:37 UTC
Committed, thanks!
Comment 11 Mahdi Mokhtari freebsd_committer 2017-01-07 16:36:02 UTC
(In reply to Nikolai Lifanov from comment #10)
Thanks :)
Comment 12 commit-hook freebsd_committer 2017-01-17 22:02:13 UTC
A commit references this bug:

Author: lifanov
Date: Tue Jan 17 22:01:33 UTC 2017
New revision: 312351
URL: https://svnweb.freebsd.org/changeset/base/312351

Log:
  MFC r311650

  Restore priority value for OGIO_KEYMAP

  PR:		206678
  Submitted by:	ecturt@gmail.com
  Reviewed by:	cem
  Approved by:	cem, matthew (mentor)
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D5095

Changes:
_U  stable/11/
  stable/11/sys/dev/kbd/kbd.c
Comment 13 commit-hook freebsd_committer 2017-01-17 22:03:17 UTC
A commit references this bug:

Author: lifanov
Date: Tue Jan 17 22:02:22 UTC 2017
New revision: 312352
URL: https://svnweb.freebsd.org/changeset/base/312352

Log:
  MFC r311650

  Restore priority value for OGIO_KEYMAP

  PR:		206678
  Submitted by:	ecturt@gmail.com
  Reviewed by:	cem
  Approved by:	cem, matthew (mentor)
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D5095

Changes:
_U  stable/10/
  stable/10/sys/dev/kbd/kbd.c