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 jwerner@chromium.org 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