refactor(console): disable getc() by default

The ability to read a character from the console constitutes an attack
vector into TF-A, as it gives attackers a means to inject arbitrary
data into TF-A. It is dangerous to keep that feature enabled if not
strictly necessary, especially in production firmware builds.

Thus, we need a way to disable this feature. Moreover, when it is
disabled, all related code should be eliminated from the firmware
binaries, such that no remnant/dead getc() code remains in memory,
which could otherwise be used as a gadget as part of a bigger security
attack.

This patch disables getc() feature by default. For legitimate getc()
use cases [1], it can be explicitly enabled by building TF-A with
ENABLE_CONSOLE_GETC=1.

The following changes are introduced when getc() is disabled:

- The multi-console framework no longer provides the console_getc()
  function.

- If the console driver selected by the platform attempts to register
  a getc() callback into the multi-console framework then TF-A will
  now fail to build.

  If registered through the assembly function finish_console_register():
  - On AArch64, you'll get:
      Error: undefined symbol CONSOLE_T_GETC used as an immediate value.
  - On AArch32, you'll get:
      Error: internal_relocation (type: OFFSET_IMM) not fixed up

  If registered through the C function console_register(), this requires
  populating a struct console with a getc field, which will trigger:
    error: 'console_t' {aka 'struct console'} has no member named 'getc'

- All console drivers which previously registered a getc() callback
  have been modified to do so only when ENABLE_CONSOLE_GETC=1.

[1] Example of such use cases would be:
    - Firmware recovery: retrieving a golden BL2 image over the console in
      order to repair a broken firmware on a bricked board.
    - Factory CLI tool: Drive some soak tests through the console.

Discussed on TF-A mailing list here:
https://lists.trustedfirmware.org/archives/list/tf-a@lists.trustedfirmware.org/thread/YS7F6RCNTWBTEOBLAXIRTXWIOYINVRW7/

Change-Id: Icb412304cd23dbdd7662df7cf8992267b7975cc5
Signed-off-by: Sandrine Bailleux <sandrine.bailleux@arm.com>
Acked-by: Baruch Siach <baruch@tkos.co.il>

Makefile

@@ -1223,6 +1223,7 @@ $(eval $(call assert_booleans,\
 	CONDITIONAL_CMO \
 	RAS_FFH_SUPPORT \
 	PSA_CRYPTO	\
+	ENABLE_CONSOLE_GETC \
 )))
 
 # Numeric_Flags
@@ -1414,6 +1415,7 @@ $(eval $(call add_defines,\
 	SVE_VECTOR_LEN \
 	ENABLE_SPMD_LP \
 	PSA_CRYPTO	\
+	ENABLE_CONSOLE_GETC \
 )))
 
 ifeq (${SANITIZE_UB},trap)

docs/getting_started/build-options.rst

@@ -1191,6 +1191,12 @@ Common build options
   per the `PSA Crypto API specification`_. This feature is only supported if
   using MbedTLS 3.x version. By default it is disabled (``0``).
 
+- ``ENABLE_CONSOLE_GETC``: Boolean option to enable `getc()` feature in console
+  driver(s). By default it is disabled (``0``) because it constitutes an attack
+  vector into TF-A by potentially allowing an attacker to inject arbitrary data.
+  This option should only be enabled on a need basis if there is a use case for
+  reading characters from the console.
+
 GICv3 driver options
 --------------------
 

docs/process/security-hardening.rst

@@ -135,6 +135,16 @@ Several build options can be used to check for security issues. Refer to the
   it is recommended to develop against ``W=2`` (which will eventually become the
   default).
 
+Additional guidelines are provided below for some security-related build
+options:
+
+- The ``ENABLE_CONSOLE_GETC`` build flag should be set to 0 to disable the
+  `getc()` feature, which allows the firmware to read characters from the
+  console. Keeping this feature enabled is considered dangerous from a security
+  point of view because it potentially allows an attacker to inject arbitrary
+  data into the firmware. It should only be enabled on a need basis if there is
+  a use case for it, for example in a testing or factory environment.
+
 .. rubric:: References
 
 -  `Arm ARM`_

drivers/amlogic/console/aarch64/meson_console.S

@@ -69,7 +69,7 @@ func console_meson_register
 
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register meson putc=1, getc=1, flush=1
+	finish_console_register meson putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

drivers/arm/dcc/dcc_console.c

@@ -53,6 +53,7 @@ static inline uint32_t __dcc_getstatus(void)
 	return read_mdccsr_el0();
 }
 
+#if ENABLE_CONSOLE_GETC
 static inline char __dcc_getchar(void)
 {
 	char c;
@@ -61,6 +62,7 @@ static inline char __dcc_getchar(void)
 
 	return c;
 }
+#endif
 
 static inline void __dcc_putchar(char c)
 {
@@ -102,6 +104,7 @@ static int32_t dcc_console_putc(int32_t ch, struct console *console)
 	return ch;
 }
 
+#if ENABLE_CONSOLE_GETC
 static int32_t dcc_console_getc(struct console *console)
 {
 	unsigned int status;
@@ -113,6 +116,7 @@ static int32_t dcc_console_getc(struct console *console)
 
 	return __dcc_getchar();
 }
+#endif
 
 /**
  * dcc_console_flush() - Function to force a write of all buffered data
@@ -135,7 +139,9 @@ static struct dcc_console dcc_console = {
 		.flags = CONSOLE_FLAG_BOOT |
 			CONSOLE_FLAG_RUNTIME,
 		.putc = dcc_console_putc,
+#if ENABLE_CONSOLE_GETC
 		.getc = dcc_console_getc,
+#endif
 		.flush = dcc_console_flush,
 	},
 };

drivers/arm/pl011/aarch32/pl011_console.S

@@ -116,7 +116,7 @@ func console_pl011_register
 
 	mov	r0, r4
 	pop	{r4, lr}
-	finish_console_register pl011 putc=1, getc=1, flush=1
+	finish_console_register pl011 putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	pop	{r4, pc}

drivers/arm/pl011/aarch64/pl011_console.S

@@ -103,7 +103,7 @@ func console_pl011_register
 
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register pl011 putc=1, getc=1, flush=1
+	finish_console_register pl011 putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

drivers/cadence/uart/aarch64/cdns_console.S

@@ -79,7 +79,7 @@ func console_cdns_register
 
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register cdns putc=1, getc=1, flush=1
+	finish_console_register cdns putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

drivers/console/aarch32/skeleton_console.S

@@ -63,7 +63,7 @@ func console_xxx_register
 	 * If any of the argument is unspecified, then the corresponding
 	 * entry in console_t is set to 0.
 	 */
-	finish_console_register xxx putc=1, getc=1, flush=1
+	finish_console_register xxx putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 	/* Jump here if hardware init fails or parameters are invalid. */
 register_fail:

drivers/console/aarch64/skeleton_console.S

@@ -63,7 +63,7 @@ func console_xxx_register
 	 * If any of the argument is unspecified, then the corresponding
 	 * entry in console_t is set to 0.
 	 */
-	finish_console_register xxx putc=1, getc=1, flush=1
+	finish_console_register xxx putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 	/* Jump here if hardware init fails or parameters are invalid. */
 register_fail:

drivers/console/multi_console.c

@@ -108,6 +108,7 @@ int putchar(int c)
 		return EOF;
 }
 
+#if ENABLE_CONSOLE_GETC
 int console_getc(void)
 {
 	int err = ERROR_NO_VALID_CONSOLE;
@@ -127,6 +128,7 @@ int console_getc(void)
 
 	return err;
 }
+#endif
 
 void console_flush(void)
 {

drivers/marvell/uart/a3700_console.S

@@ -140,7 +140,7 @@ func console_a3700_register
 
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register a3700, putc=1, getc=1, flush=1
+	finish_console_register a3700, putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

drivers/nxp/console/16550_console.S

@@ -167,7 +167,7 @@ func nxp_console_16550_register
 register_16550:
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register 16550 putc=1, getc=1, flush=1
+	finish_console_register 16550 putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

drivers/ti/uart/aarch32/16550_console.S

@@ -124,7 +124,7 @@ func console_16550_register
 register_16550:
 	mov	r0, r4
 	pop	{r4, lr}
-	finish_console_register 16550 putc=1, getc=1, flush=1
+	finish_console_register 16550 putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	pop	{r4, pc}

drivers/ti/uart/aarch64/16550_console.S

@@ -118,7 +118,7 @@ func console_16550_register
 register_16550:
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register 16550 putc=1, getc=1, flush=1
+	finish_console_register 16550 putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

include/arch/aarch32/console_macros.S

@@ -29,12 +29,20 @@
 	.endif
 	str	r1, [r0, #CONSOLE_T_PUTC]
 
+	/*
+	 * If ENABLE_CONSOLE_GETC support is disabled, but a getc callback is
+	 * specified nonetheless, the assembler will abort on encountering the
+	 * CONSOLE_T_GETC macro, which is undefined.
+	 */
 	.ifne \getc
 	  ldr	r1, =console_\_driver\()_getc
+	  str	r1, [r0, #CONSOLE_T_GETC]
 	.else
+#if ENABLE_CONSOLE_GETC
 	  mov	r1, #0
+	  str	r1, [r0, #CONSOLE_T_GETC]
+#endif
 	.endif
-	str	r1, [r0, #CONSOLE_T_GETC]
 
 	.ifne \flush
 	  ldr	r1, =console_\_driver\()_flush

include/arch/aarch64/console_macros.S

@@ -30,12 +30,19 @@
 	  str	xzr, [x0, #CONSOLE_T_PUTC]
 	.endif
 
+	/*
+	 * If ENABLE_CONSOLE_GETC support is disabled, but a getc callback is
+	 * specified nonetheless, the assembler will abort on encountering the
+	 * CONSOLE_T_GETC macro, which is undefined.
+	 */
 	.ifne \getc
 	  adrp	x1, console_\_driver\()_getc
 	  add	x1, x1, :lo12:console_\_driver\()_getc
 	  str	x1, [x0, #CONSOLE_T_GETC]
 	.else
+#if ENABLE_CONSOLE_GETC
 	  str	xzr, [x0, #CONSOLE_T_GETC]
+#endif
 	.endif
 
 	.ifne \flush

include/drivers/console.h

@@ -12,10 +12,16 @@
 #define CONSOLE_T_NEXT			(U(0) * REGSZ)
 #define CONSOLE_T_FLAGS			(U(1) * REGSZ)
 #define CONSOLE_T_PUTC			(U(2) * REGSZ)
+#if ENABLE_CONSOLE_GETC
 #define CONSOLE_T_GETC			(U(3) * REGSZ)
 #define CONSOLE_T_FLUSH			(U(4) * REGSZ)
 #define CONSOLE_T_BASE			(U(5) * REGSZ)
 #define CONSOLE_T_DRVDATA		(U(6) * REGSZ)
+#else
+#define CONSOLE_T_FLUSH			(U(3) * REGSZ)
+#define CONSOLE_T_BASE			(U(4) * REGSZ)
+#define CONSOLE_T_DRVDATA		(U(5) * REGSZ)
+#endif
 
 #define CONSOLE_FLAG_BOOT		(U(1) << 0)
 #define CONSOLE_FLAG_RUNTIME		(U(1) << 1)
@@ -42,7 +48,9 @@ typedef struct console {
 	 */
 	u_register_t flags;
 	int (*const putc)(int character, struct console *console);
+#if ENABLE_CONSOLE_GETC
 	int (*const getc)(struct console *console);
+#endif
 	void (*const flush)(struct console *console);
 	uintptr_t base;
 	/* Additional private driver data may follow here. */
@@ -75,8 +83,10 @@ void console_set_scope(console_t *console, unsigned int scope);
 void console_switch_state(unsigned int new_state);
 /* Output a character on all consoles registered for the current state. */
 int console_putc(int c);
+#if ENABLE_CONSOLE_GETC
 /* Read a character (blocking) from any console registered for current state. */
 int console_getc(void);
+#endif
 /* Flush all consoles registered for the current state. */
 void console_flush(void);
 

include/drivers/console_assertions.h

@@ -19,8 +19,10 @@ CASSERT(CONSOLE_T_FLAGS == __builtin_offsetof(console_t, flags),
 	assert_console_t_flags_offset_mismatch);
 CASSERT(CONSOLE_T_PUTC == __builtin_offsetof(console_t, putc),
 	assert_console_t_putc_offset_mismatch);
+#if ENABLE_CONSOLE_GETC
 CASSERT(CONSOLE_T_GETC == __builtin_offsetof(console_t, getc),
 	assert_console_t_getc_offset_mismatch);
+#endif
 CASSERT(CONSOLE_T_FLUSH == __builtin_offsetof(console_t, flush),
 	assert_console_t_flush_offset_mismatch);
 CASSERT(CONSOLE_T_DRVDATA == sizeof(console_t),

make_helpers/defaults.mk

@@ -362,3 +362,8 @@ ENABLE_SPMD_LP			:= 0
 
 # By default, disable PSA crypto (use MbedTLS legacy crypto API).
 PSA_CRYPTO			:= 0
+
+# getc() support from the console(s).
+# Disabled by default because it constitutes an attack vector into TF-A. It
+# should only be enabled if there is a use case for it.
+ENABLE_CONSOLE_GETC		:= 0

plat/imx/common/aarch32/imx_uart_console.S

@@ -28,7 +28,7 @@ func console_imx_uart_register
 
 	mov	r0, r4
 	pop	{r4, lr}
-	finish_console_register imx_uart putc=1, getc=1, flush=1
+	finish_console_register imx_uart putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	pop	{r4, pc}

plat/imx/common/imx_uart_console.S

@@ -33,7 +33,7 @@ func console_imx_uart_register
 
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register imx_uart putc=1, getc=1, flush=1
+	finish_console_register imx_uart putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

plat/imx/common/lpuart_console.S

@@ -27,7 +27,7 @@ func console_lpuart_register
 
 	mov	x0, x6
 	mov	x30, x7
-	finish_console_register lpuart putc=1, getc=1, flush=1
+	finish_console_register lpuart putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	ret	x7

plat/nvidia/tegra/drivers/spe/shared_console.S

@@ -71,7 +71,7 @@ func console_spe_register
 	cbz	x3, register_fail
 	str	x0, [x3, #CONSOLE_T_BASE]
 	mov	x0, x3
-	finish_console_register spe putc=1, getc=1, flush=1
+	finish_console_register spe putc=1, getc=ENABLE_CONSOLE_GETC, flush=1
 
 register_fail:
 	mov	w0, wzr

plat/socionext/uniphier/uniphier_console_setup.c

@@ -30,7 +30,9 @@ static console_t uniphier_console = {
 		 CONSOLE_FLAG_CRASH |
 		 CONSOLE_FLAG_TRANSLATE_CRLF,
 	.putc = uniphier_console_putc,
+#if ENABLE_CONSOLE_GETC
 	.getc = uniphier_console_getc,
+#endif
 	.flush = uniphier_console_flush,
 };