Bug 206680
| Summary: | kbd spl calls obscure locking | ||
|---|---|---|---|
| Product: | Base System | Reporter: | CTurt <ecturt> |
| Component: | kern | Assignee: | Ed Maste <emaste> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | emaste, op, shawn.webb, wulf |
| Priority: | --- | Keywords: | needs-patch, security |
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
| See Also: | https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206678 | ||
|
Description
CTurt
2016-01-27 15:45:10 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? 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. (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. 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.
(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 Any movement on this? (In reply to Shawn Webb from comment #6) Yes i'm placing lock/unlock where we should (and needed) All 8042 stuff (intr/cdev) is executed with Giant lock taken. What extra protection inside dev/atkbdc is required? 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(-) FWIW the stubs still left in main now are: splhigh (11 files) splimp (13) splnet (13) spltty (12) 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(-) ^Triage: assign to committer who resolved this in 2022. |
