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

Enhancement and fixes of "Secure" feature #16958

Merged
merged 21 commits into from
May 14, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions quantum/process_keycode/process_secure.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

bool preprocess_secure(uint16_t keycode, keyrecord_t *record) {
if (secure_is_unlocking()) {
if (!record->event.pressed) {
// !pressed will trigger on any already held keys (such as layer keys),
// and cause the request secure check to prematurely fail.
if (record->event.pressed) {
secure_keypress_event(record->event.key.row, record->event.key.col);
}

Expand Down Expand Up @@ -36,4 +38,4 @@ bool process_secure(uint16_t keycode, keyrecord_t *record) {
}
#endif
return true;
}
}
7 changes: 7 additions & 0 deletions quantum/quantum.c
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,13 @@ bool process_record_quantum(keyrecord_t *record) {

#if defined(SECURE_ENABLE)
if (!preprocess_secure(keycode, record)) {
// If keys are being held when this is triggered, they may not be released properly
// this can result in stuck keys, mods and layers. To prevent that, manually
// clear these, when it is triggered.
if (!record->event.pressed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens on every keypress, rather than the transition from inscure -> unlocking.

I somewhat prefer the addition of a "request unlock" keycode, or just letting the caller handle the case if they manually call secure_request_unlock.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do that.

clear_keyboard();
layer_clear();
}
return false;
}
#endif
Expand Down
16 changes: 16 additions & 0 deletions quantum/secure.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,21 @@ secure_status_t secure_get_status(void) {

void secure_lock(void) {
secure_status = SECURE_LOCKED;
secure_hook_quantum(secure_status);
}

void secure_unlock(void) {
secure_status = SECURE_UNLOCKED;
idle_time = timer_read32();
secure_hook_quantum(secure_status);
}

void secure_request_unlock(void) {
if (secure_status == SECURE_LOCKED) {
secure_status = SECURE_PENDING;
unlock_time = timer_read32();
}
secure_hook_quantum(secure_status);
}

void secure_activity_event(void) {
Expand Down Expand Up @@ -85,3 +88,16 @@ void secure_task(void) {
}
#endif
}

#if defined(SECURE_ENABLE)
drashna marked this conversation as resolved.
Show resolved Hide resolved
__attribute__((weak)) bool secure_hook_user(secure_status_t secure_status) {
return true;
}
__attribute__((weak)) bool secure_hook_kb(secure_status_t secure_status) {
return secure_hook_user(secure_status);
}

__attribute__((weak)) void secure_hook_quantum(secure_status_t secure_status) {
secure_hook_kb(secure_status);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still feels somewhat wrong, as it would contain the XAP activity hooks? Or if the plan is to add a non weak version in quantum.h, then we should add a comment here to ensure it doesnt get modified.

Though at this point it might be better if its reworked as part of XAP.

Copy link
Member Author

@drashna drashna May 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense/be best to secure_hook_quantum handle the clear, if transitioning to "pending", and have that in quantum.c?

Eg, throw this in quantum.c (and only this)

void secure_hook_quantum(secure_status_t secure_status) {
    if (secure_status == SECURE_PENDING) {
            clear_keyboard();
            layer_clear();
    }        

    secure_hook_kb(secure_status);
}

That would handle the keyboard clearing only when an unlock request is sent, and xap could be added here, as well.

Edit: yeah, I think that's probably the best way. Will add.

#endif
12 changes: 12 additions & 0 deletions quantum/secure.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,15 @@ void secure_keypress_event(uint8_t row, uint8_t col);
/** \brief Handle various secure subsystem background tasks
*/
void secure_task(void);

/** \brief quantum hook called when changing secure status device
*/
void secure_hook_quantum(secure_status_t secure_status);
drashna marked this conversation as resolved.
Show resolved Hide resolved

/** \brief user hook called when changing secure status device
*/
bool secure_hook_user(secure_status_t secure_status);

/** \brief keyboard hook called when changing secure status device
*/
bool secure_hook_kb(secure_status_t secure_status);
32 changes: 32 additions & 0 deletions tests/secure/config.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* Copyright 2021 Stefan Kerkmann
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#pragma once

#include "test_common.h"

// clang-format off
#define SECURE_UNLOCK_SEQUENCE \
{ \
{0, 1}, \
{0, 2}, \
{0, 3}, \
{0, 4} \
}
// clang-format on

#define SECURE_UNLOCK_TIMEOUT 20
#define SECURE_IDLE_TIMEOUT 50
20 changes: 20 additions & 0 deletions tests/secure/test.mk
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Copyright 2021 Stefan Kerkmann
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 2 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

# --------------------------------------------------------------------------------
# Keep this file, even if it is empty, as a marker that this folder contains tests
# --------------------------------------------------------------------------------

SECURE_ENABLE = yes
Loading