Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/74243 )
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 Reviewed-on: https://review.coreboot.org/c/coreboot/+/74243 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Elyes Haouas ehaouas@noos.fr Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/arch/x86/Makefile.inc M src/arch/x86/walkcbfs.S 2 files changed, 44 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Elyes Haouas: Looks good to me, but someone else must approve Karthik Ramasubramanian: Looks good to me, approved
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