Bug 206678
| Summary: | OGIO_KEYMAP command does not restore priority level | ||
|---|---|---|---|
| Product: | Base System | Reporter: | CTurt <ecturt> |
| Component: | kern | Assignee: | Nikolai Lifanov <lifanov> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | emaste, lifanov, mmokhi, op, secteam, shawn.webb |
| Priority: | --- | Keywords: | dogfood, patch, security |
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
| URL: | https://reviews.freebsd.org/D5095 | ||
| See Also: | https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206680 | ||
The solution is to replace the `return` with `break`: https://github.com/HardenedBSD/hardenedBSD-playground/commit/9962f56bb8f942cf03a65b46e96ef26369571dec 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). (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. 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
Thanks for clarifying this bug/issue. :) Any movement on this? (In reply to Shawn Webb from comment #7) From patch point of view it's done. Just waiting to be committed. 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 Committed, thanks! (In reply to Nikolai Lifanov from comment #10) Thanks :) 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 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 |
`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.