[S] Change in coreboot[master]: arch/x86: Disable walkcbfs_asm code when CONFIG_CBFS_VERIFICATION is set

[Julius Werner (Code Review)]

Attention is currently required from: Subrata Banik, Karthik Ramasubramanian.

Hello Subrata Banik, Karthik Ramasubramanian,

I'd like you to do a code review.
Please visit

https://review.coreboot.org/c/coreboot/+/74243

to review the following change.

Change subject: arch/x86: Disable walkcbfs_asm code when CONFIG_CBFS_VERIFICATION is set
......................................................................

arch/x86: Disable walkcbfs_asm code when CONFIG_CBFS_VERIFICATION is set

walkcbfs_asm is a simple CBFS implementation in assembly to find a file
on a system with memory-mapped SPI flash. It seems to be mostly unused
nowadays and is only still called for early microcode loading on some
old systems (e.g. FSP 1.1 and older).

Using this implementation with CONFIG_CBFS_VERIFICATION is unsafe
because it does not verify the hashes the way the normal CBFS code does.
Therefore, to avoid potential security vulnerabilities from creeping in,
this patch makes sure the code cannot be compiled in when
CBFS_VERIFICATION is active. That means it won't be supported on the old
boards using this for microcode loading.

Ideally CONFIG_CBFS_VERIFICATION should have a `depends on` to make this
dependency more obvious in menuconfig, but the configs actually using
this code are not easy to untangle (e.g. CONFIG_MICROCODE_UPDATE_PRE_RAM
is just set everywhere by default although only very few boards are
really using it, and a lot of different old Intel CPU models are linking
in src/cpu/intel/car/non-evict/cache_as_ram.S without being united under
a single Kconfig so that's not easy to change). To keep things simple,
this patch will just prevent the code from being built and result in a
linker error if a bad combination of Kconfigs is used together. Later
patches can clean up the Kconfigs to better wrap that dependency if the
affected boards are still of enough interest to be worth that effort.

Signed-off-by: Julius Werner 
Change-Id: I614a1b05881aa7c1539a7f7f296855ff708db56c
---
M src/arch/x86/Makefile.inc
M src/arch/x86/walkcbfs.S
2 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/74243/1

diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index d281037..4ac30ca 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -112,8 +112,10 @@
    $(cbfs-autogen-attributes) $(TS_OPTIONS) $(CBFSTOOL_ADD_CMD_OPTIONS)
 endif

+ifneq ($(CONFIG_CBFS_VERIFICATION),y)
 $(call src-to-obj,bootblock,$(dir)/walkcbfs.S): $(obj)/fmap_config.h
 bootblock-y += walkcbfs.S
+endif

endif # CONFIG_ARCH_BOOTBLOCK_X86_32 / CONFIG_ARCH_BOOTBLOCK_X86_64

diff --git a/src/arch/x86/walkcbfs.S b/src/arch/x86/walkcbfs.S
index 208a128..f4b8d254 100644
--- a/src/arch/x86/walkcbfs.S
+++ b/src/arch/x86/walkcbfs.S
@@ -2,6 +2,10 @@

#include <fmap_config.h>

+#if CONFIG(CBFS_VERIFICATION)
+#error "walkcbfs_asm is not safe to use with CBFS verification!"
+#endif
+
 /* we use this instead of CBFS_HEADER_ALIGN because the latter is retired. */
 #define CBFS_ALIGNMENT 64