Matt Delco has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32717
Change subject: libpayload: classify all keyboards ......................................................................
libpayload: classify all keyboards
Gets rid of 'unknown' and classifies all keyboards. Depthcharge uses the keyboard type to help determine whether it can trust the keyboard for security-sensitive confirmations. Currently it trusts anything except usb, but now there's a need to distrust ps/2 keyboards that are associated with untrusted ECs. To help facilitate this, coreboot needs to report more details about non-usb keyboards, so this change replaces unknown with enum values that distinguish uart from ps/2. After this change there are no unknown keyboards left, so I've removed the enum.
BUG=b:129471321 BRANCH=None TEST=Local compile and flash to systems with trusted and non-trusted ECs. Confirmed that security confirmation can't be performed via keyboard on a system with an untrusted EC but can still be performed on a system with a trusted EC.
Change-Id: Iee6295dafadf7cb3da98b62f43b0e184b2b69b1e Signed-off-by: Matt Delco delco@chromium.org --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/serial/8250.c M payloads/libpayload/drivers/serial/ipq40xx.c M payloads/libpayload/drivers/serial/ipq806x.c M payloads/libpayload/drivers/serial/s5p.c M payloads/libpayload/include/libpayload.h 6 files changed, 11 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/32717/1
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 1035bf2..92a4146 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -296,7 +296,8 @@
static struct console_input_driver cons = { .havekey = keyboard_havechar, - .getchar = keyboard_getchar + .getchar = keyboard_getchar, + .input_type = CONSOLE_INPUT_TYPE_I8042, };
void keyboard_init(void) diff --git a/payloads/libpayload/drivers/serial/8250.c b/payloads/libpayload/drivers/serial/8250.c index f503bdb..9502d4b 100644 --- a/payloads/libpayload/drivers/serial/8250.c +++ b/payloads/libpayload/drivers/serial/8250.c @@ -98,7 +98,8 @@
static struct console_input_driver consin = { .havekey = &serial_havechar, - .getchar = &serial_getchar + .getchar = &serial_getchar, + .input_type = CONSOLE_INPUT_TYPE_UART, };
static struct console_output_driver consout = { diff --git a/payloads/libpayload/drivers/serial/ipq40xx.c b/payloads/libpayload/drivers/serial/ipq40xx.c index 52d71b8..7656ad7 100644 --- a/payloads/libpayload/drivers/serial/ipq40xx.c +++ b/payloads/libpayload/drivers/serial/ipq40xx.c @@ -560,6 +560,7 @@
consin.havekey = serial_havechar; consin.getchar = serial_getchar; + consin.input_type = CONSOLE_INPUT_TYPE_UART;
consout.putchar = serial_putchar;
diff --git a/payloads/libpayload/drivers/serial/ipq806x.c b/payloads/libpayload/drivers/serial/ipq806x.c index 912893d..183ada6 100644 --- a/payloads/libpayload/drivers/serial/ipq806x.c +++ b/payloads/libpayload/drivers/serial/ipq806x.c @@ -352,6 +352,7 @@
consin.havekey = serial_havechar; consin.getchar = serial_getchar; + consin.input_type = CONSOLE_INPUT_TYPE_UART;
consout.putchar = serial_putchar;
diff --git a/payloads/libpayload/drivers/serial/s5p.c b/payloads/libpayload/drivers/serial/s5p.c index 1d23352..6ca5dc4 100644 --- a/payloads/libpayload/drivers/serial/s5p.c +++ b/payloads/libpayload/drivers/serial/s5p.c @@ -84,7 +84,8 @@ static struct console_input_driver s5p_serial_input = { .havekey = &serial_havechar, - .getchar = &serial_getchar + .getchar = &serial_getchar, + .input_type = CONSOLE_INPUT_TYPE_UART, };
void serial_init(void) diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 3a84b3b..3fdb096 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -348,8 +348,10 @@ * @{ */ typedef enum { - CONSOLE_INPUT_TYPE_UNKNOWN = 0, + /* UNKNOWN was zero, but currently all are known */ CONSOLE_INPUT_TYPE_USB, + CONSOLE_INPUT_TYPE_I8042, + CONSOLE_INPUT_TYPE_UART, } console_input_type;
void console_init(void);
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 1: Code-Review+2
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 1: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32717/1/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32717/1/payloads/libpayload/include/libpaylo... PS1, Line 351: /* UNKNOWN was zero, but currently all are known */ No, please keep an UNKNOWN as zero. Otherwise, when people add new drivers and don't pay attention to this, they will automatically be USB.
Hello Julius Werner, Duncan Laurie, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32717
to look at the new patch set (#2).
Change subject: libpayload: classify all keyboards ......................................................................
libpayload: classify all keyboards
Depthcharge uses the keyboard type to help determine whether it can trust the keyboard for security-sensitive confirmations. Currently it trusts anything except usb, but now there's a need to distrust ps/2 keyboards that are associated with untrusted ECs. To help facilitate this, coreboot needs to report more details about non-usb keyboards, so this change replaces the current instances of unknown with enum values that distinguish uart from ps/2.
BUG=b:129471321 BRANCH=None TEST=Local compile and flash to systems with trusted and non-trusted ECs. Confirmed that security confirmation can't be performed via keyboard on a system with an untrusted EC but can still be performed on a system with a trusted EC.
Change-Id: Iee6295dafadf7cb3da98b62f43b0e184b2b69b1e Signed-off-by: Matt Delco delco@chromium.org --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/serial/8250.c M payloads/libpayload/drivers/serial/ipq40xx.c M payloads/libpayload/drivers/serial/ipq806x.c M payloads/libpayload/drivers/serial/s5p.c M payloads/libpayload/include/libpayload.h 6 files changed, 10 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/32717/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 2: Code-Review+2
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32717/1/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32717/1/payloads/libpayload/include/libpaylo... PS1, Line 351: /* UNKNOWN was zero, but currently all are known */
No, please keep an UNKNOWN as zero. […]
I overlooked adding a "= 1" after usb so the numeric value remained the same (and avoids issues with placing dependencies on commits). I've brought back the UNKNOWN.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 2: Code-Review+2
Hello Julius Werner, Duncan Laurie, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32717
to look at the new patch set (#3).
Change subject: libpayload: classify all keyboards ......................................................................
libpayload: classify all keyboards
Depthcharge uses the keyboard type to help determine whether it can trust the keyboard for security-sensitive confirmations. Currently it trusts anything except usb, but now there's a need to distrust ec-based ps/2 keyboards that are associated with untrusted ECs. To help facilitate this, coreboot needs to report more details about non-usb keyboards, so this change replaces the current instances of unknown with enum values that distinguish uart and gpio from ec-based keyboards.
BUG=b:129471321 BRANCH=None TEST=Local compile and flash to systems with trusted and non-trusted ECs. Confirmed that security confirmation can't be performed via keyboard on a system with an untrusted EC but can still be performed on a system with a trusted EC.
Change-Id: Iee6295dafadf7cb3da98b62f43b0e184b2b69b1e Signed-off-by: Matt Delco delco@chromium.org --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/serial/8250.c M payloads/libpayload/drivers/serial/ipq40xx.c M payloads/libpayload/drivers/serial/ipq806x.c M payloads/libpayload/drivers/serial/s5p.c M payloads/libpayload/include/libpayload.h 6 files changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/32717/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32717/3/payloads/libpayload/drivers/i8042/ke... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/#/c/32717/3/payloads/libpayload/drivers/i8042/ke... PS3, Line 300: CONSOLE_INPUT_TYPE_EC even on systems without an EC?
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32717 )
Change subject: libpayload: classify all keyboards ......................................................................
libpayload: classify all keyboards
Depthcharge uses the keyboard type to help determine whether it can trust the keyboard for security-sensitive confirmations. Currently it trusts anything except usb, but now there's a need to distrust ec-based ps/2 keyboards that are associated with untrusted ECs. To help facilitate this, coreboot needs to report more details about non-usb keyboards, so this change replaces the current instances of unknown with enum values that distinguish uart and gpio from ec-based keyboards.
BUG=b:129471321 BRANCH=None TEST=Local compile and flash to systems with trusted and non-trusted ECs. Confirmed that security confirmation can't be performed via keyboard on a system with an untrusted EC but can still be performed on a system with a trusted EC.
Change-Id: Iee6295dafadf7cb3da98b62f43b0e184b2b69b1e Signed-off-by: Matt Delco delco@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32717 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/serial/8250.c M payloads/libpayload/drivers/serial/ipq40xx.c M payloads/libpayload/drivers/serial/ipq806x.c M payloads/libpayload/drivers/serial/s5p.c M payloads/libpayload/include/libpayload.h 6 files changed, 11 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 1035bf2..42431c3 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -296,7 +296,8 @@
static struct console_input_driver cons = { .havekey = keyboard_havechar, - .getchar = keyboard_getchar + .getchar = keyboard_getchar, + .input_type = CONSOLE_INPUT_TYPE_EC, };
void keyboard_init(void) diff --git a/payloads/libpayload/drivers/serial/8250.c b/payloads/libpayload/drivers/serial/8250.c index f503bdb..9502d4b 100644 --- a/payloads/libpayload/drivers/serial/8250.c +++ b/payloads/libpayload/drivers/serial/8250.c @@ -98,7 +98,8 @@
static struct console_input_driver consin = { .havekey = &serial_havechar, - .getchar = &serial_getchar + .getchar = &serial_getchar, + .input_type = CONSOLE_INPUT_TYPE_UART, };
static struct console_output_driver consout = { diff --git a/payloads/libpayload/drivers/serial/ipq40xx.c b/payloads/libpayload/drivers/serial/ipq40xx.c index 52d71b8..7656ad7 100644 --- a/payloads/libpayload/drivers/serial/ipq40xx.c +++ b/payloads/libpayload/drivers/serial/ipq40xx.c @@ -560,6 +560,7 @@
consin.havekey = serial_havechar; consin.getchar = serial_getchar; + consin.input_type = CONSOLE_INPUT_TYPE_UART;
consout.putchar = serial_putchar;
diff --git a/payloads/libpayload/drivers/serial/ipq806x.c b/payloads/libpayload/drivers/serial/ipq806x.c index 912893d..183ada6 100644 --- a/payloads/libpayload/drivers/serial/ipq806x.c +++ b/payloads/libpayload/drivers/serial/ipq806x.c @@ -352,6 +352,7 @@
consin.havekey = serial_havechar; consin.getchar = serial_getchar; + consin.input_type = CONSOLE_INPUT_TYPE_UART;
consout.putchar = serial_putchar;
diff --git a/payloads/libpayload/drivers/serial/s5p.c b/payloads/libpayload/drivers/serial/s5p.c index 1d23352..6ca5dc4 100644 --- a/payloads/libpayload/drivers/serial/s5p.c +++ b/payloads/libpayload/drivers/serial/s5p.c @@ -84,7 +84,8 @@ static struct console_input_driver s5p_serial_input = { .havekey = &serial_havechar, - .getchar = &serial_getchar + .getchar = &serial_getchar, + .input_type = CONSOLE_INPUT_TYPE_UART, };
void serial_init(void) diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 3a84b3b..a578d41 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -350,6 +350,9 @@ typedef enum { CONSOLE_INPUT_TYPE_UNKNOWN = 0, CONSOLE_INPUT_TYPE_USB, + CONSOLE_INPUT_TYPE_EC, + CONSOLE_INPUT_TYPE_UART, + CONSOLE_INPUT_TYPE_GPIO, } console_input_type;
void console_init(void);
-
Duncan Laurie (Code Review) -
Julius Werner (Code Review)
-
Matt Delco (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)