`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.
The solution is to replace the `return` with `break`: https://github.com/HardenedBSD/hardenedBSD-playground/commit/9962f56bb8f942cf03a65b46e96ef26369571dec
Related: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206680
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