Merge "refactor(console): disable getc() by default" into integration

25 files changed, 75 insertions, 15 deletions

diff --git a/Makefile b/Makefile
index 464544f6c..94dfc3e16 100644
--- a/Makefile
+++ b/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)
diff --git a/docs/getting_started/build-options.rst b/docs/getting_started/build-options.rst
index 34d83f255..c045a6aa2 100644
--- a/docs/getting_started/build-options.rst
+++ b/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
 --------------------
 
diff --git a/docs/process/security-hardening.rst b/docs/process/security-hardening.rst
index f9618db08..eace467d4 100644
--- a/docs/process/security-hardening.rst
+++ b/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`_
diff --git a/drivers/amlogic/console/aarch64/meson_console.S b/drivers/amlogic/console/aarch64/meson_console.S
index 6d0a2d62e..d955d83c5 100644
--- a/drivers/amlogic/console/aarch64/meson_console.S
+++ b/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
diff --git a/drivers/arm/dcc/dcc_console.c b/drivers/arm/dcc/dcc_console.c
index 8d9f7936d..d8f9462ae 100644
--- a/drivers/arm/dcc/dcc_console.c
+++ b/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,
 	},
 };
diff --git a/drivers/arm/pl011/aarch32/pl011_console.S b/drivers/arm/pl011/aarch32/pl011_console.S
index 9caeb0c69..b7d1747f5 100644
--- a/drivers/arm/pl011/aarch32/pl011_console.S
+++ b/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}
diff --git a/drivers/arm/pl011/aarch64/pl011_console.S b/drivers/arm/pl011/aarch64/pl011_console.S
index 861d2ed22..8cb0122be 100644
--- a/drivers/arm/pl011/aarch64/pl011_console.S
+++ b/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
diff --git a/drivers/cadence/uart/aarch64/cdns_console.S b/drivers/cadence/uart/aarch64/cdns_console.S
index 1bdaa4872..d2dd0a8e2 100644
--- a/drivers/cadence/uart/aarch64/cdns_console.S
+++ b/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
diff --git a/drivers/console/aarch32/skeleton_console.S b/drivers/console/aarch32/skeleton_console.S
index a9e13ec44..05a598508 100644
--- a/drivers/console/aarch32/skeleton_console.S
+++ b/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:
diff --git a/drivers/console/aarch64/skeleton_console.S b/drivers/console/aarch64/skeleton_console.S
index 7ea2eec9f..3310d2892 100644
--- a/drivers/console/aarch64/skeleton_console.S
+++ b/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:
diff --git a/drivers/console/multi_console.c b/drivers/console/multi_console.c
index 93c38d849..e962fff37 100644
--- a/drivers/console/multi_console.c
+++ b/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)
 {
diff --git a/drivers/marvell/uart/a3700_console.S b/drivers/marvell/uart/a3700_console.S
index c7eb165e6..a1eacbc43 100644
--- a/drivers/marvell/uart/a3700_console.S
+++ b/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
diff --git a/drivers/nxp/console/16550_console.S b/drivers/nxp/console/16550_console.S
index 044d3d074..b5617a3e8 100644
--- a/drivers/nxp/console/16550_console.S
+++ b/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
diff --git a/drivers/ti/uart/aarch32/16550_console.S b/drivers/ti/uart/aarch32/16550_console.S
index 0429f8702..898a68d8c 100644
--- a/drivers/ti/uart/aarch32/16550_console.S
+++ b/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}
diff --git a/drivers/ti/uart/aarch64/16550_console.S b/drivers/ti/uart/aarch64/16550_console.S
index cb2151253..2b1b5a9d7 100644
--- a/drivers/ti/uart/aarch64/16550_console.S
+++ b/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
diff --git a/include/arch/aarch32/console_macros.S b/include/arch/aarch32/console_macros.S
index 996cb327f..726b28186 100644
--- a/include/arch/aarch32/console_macros.S
+++ b/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
diff --git a/include/arch/aarch64/console_macros.S b/include/arch/aarch64/console_macros.S
index 3285d855a..8adb9cdb1 100644
--- a/include/arch/aarch64/console_macros.S
+++ b/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
diff --git a/include/drivers/console.h b/include/drivers/console.h
index f499571f7..fa4eb9462 100644
--- a/include/drivers/console.h
+++ b/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);
 
diff --git a/include/drivers/console_assertions.h b/include/drivers/console_assertions.h
index 00caa3141..9f0657326 100644
--- a/include/drivers/console_assertions.h
+++ b/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),
diff --git a/make_helpers/defaults.mk b/make_helpers/defaults.mk
index 321ae9fd1..f31365a1f 100644
--- a/make_helpers/defaults.mk
+++ b/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
diff --git a/plat/imx/common/aarch32/imx_uart_console.S b/plat/imx/common/aarch32/imx_uart_console.S
index 1a1229aab..2a35b5edf 100644
--- a/plat/imx/common/aarch32/imx_uart_console.S
+++ b/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}
diff --git a/plat/imx/common/imx_uart_console.S b/plat/imx/common/imx_uart_console.S
index 4d17288a1..560db15b5 100644
--- a/plat/imx/common/imx_uart_console.S
+++ b/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
diff --git a/plat/imx/common/lpuart_console.S b/plat/imx/common/lpuart_console.S
index ff01e3551..7acf77384 100644
--- a/plat/imx/common/lpuart_console.S
+++ b/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
diff --git a/plat/nvidia/tegra/drivers/spe/shared_console.S b/plat/nvidia/tegra/drivers/spe/shared_console.S
index d1b18dd44..5ad4eb8ab 100644
--- a/plat/nvidia/tegra/drivers/spe/shared_console.S
+++ b/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
diff --git a/plat/socionext/uniphier/uniphier_console_setup.c b/plat/socionext/uniphier/uniphier_console_setup.c
index 9fda26e93..9268f5d5a 100644
--- a/plat/socionext/uniphier/uniphier_console_setup.c
+++ b/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,
 };