Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VT kbd reset check #12378

Merged
merged 2 commits into from May 16, 2019

Conversation

8 participants
@rbalint
Copy link
Contributor

commented Apr 24, 2019

No description provided.

@setharnold

This comment has been minimized.

Copy link

commented Apr 25, 2019

Is it ever okay for vt_reset_keyboard() or toggle_utf8() to run without checking vt_verify_kbmode() first? Should this call be moved into these two functions rather than into all the call sites?

Thanks

@rbalint

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@setharnold I felt the code more descriptive having the checks and the actions separately rather than moving the extra checks to the actions. Also I did not add extra logging, because it is normal to have VTs in raw mode.
I can imagine that we may call vt_reset_keyboard() when the keyboard is really in the bad state, but did not find such scenario myself.

@poettering
Copy link
Member

left a comment

looks good, just some nitpicks

Show resolved Hide resolved src/basic/terminal-util.c Outdated
Show resolved Hide resolved src/basic/terminal-util.c Outdated
@poettering

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

i think i agree with @setharnold though: if those checks are pre-condition to these operations they probably should move into the callee, not the caller. Encapsulation and terseness is nice, and the caller shouldn't need to know about that

@rbalint

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@setharnold @poettering OK, made the requested changes.

@poettering
Copy link
Member

left a comment

Also, CI doesn't pass. Please always build and test before pushing. Thanks!

Show resolved Hide resolved src/basic/terminal-util.c
Show resolved Hide resolved src/vconsole/vconsole-setup.c Outdated

@rbalint rbalint force-pushed the rbalint:vt-kbd-reset-check branch from 4368327 to 9a03a4e Apr 30, 2019

@rbalint rbalint changed the title VT kbd reset check WIP: VT kbd reset check Apr 30, 2019

@rbalint rbalint force-pushed the rbalint:vt-kbd-reset-check branch from 9a03a4e to 6b7da9d Apr 30, 2019

@rbalint rbalint changed the title WIP: VT kbd reset check VT kbd reset check Apr 30, 2019

@rbalint

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@poettering Would you like to see something else to be changed?

@poettering
Copy link
Member

left a comment

looks pretty good, just some nitpcks

r = vt_verify_kbmode(fd);
if (r < 0) {
if (r == -EBUSY) {
log_debug_errno(-r, "Keyboard is not in XLATE or UNICODE mode, not resetting: %m");

This comment has been minimized.

Copy link
@poettering

poettering May 14, 2019

Member

no need to negate "r". log_debug_errno() (and its friends) generally do that internally if necessary.

Let's keep the indentation level small here, hence maybe write as:

if (r == -EBUSY) {
        …
} else if (r < 0)
        return r;
r = vt_verify_kbmode(fd);
if (r < 0) {
if (r == -EBUSY) {
log_debug_errno(-r, "Virtual console %s is not in K_XLATE or K_UNICODE: %m", name);

This comment has been minimized.

Copy link
@poettering

poettering May 14, 2019

Member

as above, no negation necessary

log_debug_errno(-r, "Virtual console %s is not in K_XLATE or K_UNICODE: %m", name);
return 0;
} else
return log_debug_errno(-r, "Failed to verify kbdmode on %s: %m", name);

This comment has been minimized.

Copy link
@poettering
return 0;
} else
return log_debug_errno(-r, "Failed to verify kbdmode on %s: %m", name);
}

This comment has been minimized.

Copy link
@poettering

poettering May 14, 2019

Member

and similar to above: small indentation levels are cool!

@poettering poettering added this to the v243 milestone May 14, 2019

@rbalint rbalint force-pushed the rbalint:vt-kbd-reset-check branch from 6b7da9d to 9a932dd May 14, 2019

log_debug_errno(r, "Virtual console %s is not in K_XLATE or K_UNICODE: %m", name);
return 0;
} else if (r < 0)
return log_debug_errno(r, "Failed to verify kbdmode on %s: %m", name);

This comment has been minimized.

Copy link
@poettering

poettering May 15, 2019

Member

all other failure codepaths in this functions log loudly, this one should hence too, i.e. bump this one line from log_debug_errno() to log_warning_errno()

@poettering
Copy link
Member

left a comment

(btw, please always add a quick comment when you push a new version. For some reason github doesn't always notify us about pushes)

@rbalint rbalint force-pushed the rbalint:vt-kbd-reset-check branch 2 times, most recently from e7e5b3e to de564ef May 15, 2019

Add check to switch VTs only between K_XLATE or K_UNICODE
Switching to K_UNICODE from other than L_XLATE can make the keyboard
unusable and possibly leak keypresses from X.

BugLink: https://launchpad.net/bugs/1803993

@rbalint rbalint force-pushed the rbalint:vt-kbd-reset-check branch from de564ef to 13a43c7 May 15, 2019

@rbalint

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@poettering I see bionic autopkgtests failing for other PR-s, too, and I ran autopkgtest successfully when I cherry-picked the change to Disco, thus I believe the failing tests are not related to this commit.

@poettering poettering merged commit 9725f1a into systemd:master May 16, 2019

10 of 14 checks passed

bionic-amd64 autopkgtest finished (failure)
Details
bionic-i386 autopkgtest finished (failure)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
CentOS CI (Arch in KVM with sanitizers) Build finished.
Details
CentOS CI (Arch in KVM) Build finished.
Details
CentOS CI (CentOS 7) Build finished.
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
bionic-s390x autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
systemd.systemd Build #20190515.27 succeeded
Details
systemd.systemd (ASan_UBSan) ASan_UBSan succeeded
Details
systemd.systemd (FuzzBuzz) FuzzBuzz succeeded
Details
@jackpot51

This comment has been minimized.

Copy link

commented May 16, 2019

🎉

@setharnold

This comment has been minimized.

Copy link

commented May 17, 2019

Thanks everyone for working on this!

Use CVE-2018-20839.

Thanks

@mbiebl

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

@rbalint seems like this has caused a regression.
Could you have a look at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929229 ?
I've pulled this particular patch into 241-4.

@rbalint

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Thanks, the regression is tracked in #12616 .

@fbuihuu

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Sorry for coming late but I can't figure out how the initial issue that @rbalint fixed can happen in practice and the commit message of the relevant commit doesn't really help...

In my understanding logind resets the keyboard to K_UNICODE only when it has to acknowledge the VT-switch signal itself for a terminal which has no controlling process.

@rbalint did you manage to reproduce the issue ? If so can you tell how exactly ?

I'm asking because I can't here.

@jsoref

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

fwiw, github never notifies for force-pushes. Even though it's well aware of them. People can complain to github support and maybe they'll eventually fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.