Attention is currently required from: Furquan Shaikh, Aaron Durbin. Hello Furquan Shaikh, Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/49337
to review the following change.
Change subject: prog_loaders: Remove prog_locate() ......................................................................
prog_loaders: Remove prog_locate()
This patch rewrites the last few users of prog_locate() to access CBFS APIs directly and removes the call. This eliminates the double-meaning of prog_rdev() (referring to both the boot medium where the program is stored before loading, and the memory area where it is loaded after) and makes sure that programs are always located and loaded in a single operation. This makes CBFS verification easier to implement and secure because it avoids leaking a raw rdev of unverified data outside the CBFS core code.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I7a5525f66e1d5f3a632e8f6f0ed9e116e3cebfcf --- M src/drivers/intel/fsp1_1/car.c M src/drivers/intel/fsp1_1/fsp_relocate.c M src/drivers/intel/fsp1_1/include/fsp/util.h M src/drivers/intel/fsp1_1/ramstage.c M src/drivers/intel/fsp1_1/temp_ram_exit.c M src/include/program_loading.h M src/lib/prog_loaders.c 7 files changed, 20 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/49337/1
diff --git a/src/drivers/intel/fsp1_1/car.c b/src/drivers/intel/fsp1_1/car.c index 9b47e71..5d41a01 100644 --- a/src/drivers/intel/fsp1_1/car.c +++ b/src/drivers/intel/fsp1_1/car.c @@ -2,13 +2,13 @@
#include <arch/romstage.h> #include <arch/symbols.h> +#include <cbfs.h> #include <cbmem.h> #include <console/console.h> #include <commonlib/helpers.h> #include <cpu/x86/mtrr.h> #include <fsp/car.h> #include <fsp/util.h> -#include <program_loading.h>
void fill_postcar_frame(struct postcar_frame *pcf) { @@ -27,14 +27,14 @@ { /* Need to locate the current FSP_INFO_HEADER. The cache-as-ram * is still enabled. We can directly access work buffer here. */ - struct prog fsp = PROG_INIT(PROG_REFCODE, "fsp.bin"); + void *fsp = cbfs_map("fsp.bin", NULL);
- if (prog_locate(&fsp)) + if (!fsp) die_with_post_code(POST_INVALID_CBFS, "Unable to locate fsp.bin");
/* This leaks a mapping which this code assumes is benign as * the flash is memory mapped CPU's address space. */ - FSP_INFO_HEADER *fih = find_fsp((uintptr_t)rdev_mmap_full(prog_rdev(&fsp))); + FSP_INFO_HEADER *fih = find_fsp((uintptr_t)fsp);
if (!fih) die("Invalid FSP header\n"); diff --git a/src/drivers/intel/fsp1_1/fsp_relocate.c b/src/drivers/intel/fsp1_1/fsp_relocate.c index d078f59..7aaba82 100644 --- a/src/drivers/intel/fsp1_1/fsp_relocate.c +++ b/src/drivers/intel/fsp1_1/fsp_relocate.c @@ -1,29 +1,24 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <console/console.h> +#include <cbfs.h> #include <cbmem.h> #include <commonlib/fsp.h> #include <fsp/util.h>
-int fsp_relocate(struct prog *fsp_relocd, const struct region_device *fsp_src) +int fsp_relocate(struct prog *fsp_relocd) { - void *new_loc; void *fih; ssize_t fih_offset; - size_t size = region_device_sz(fsp_src); + size_t size;
- new_loc = cbmem_add(CBMEM_ID_REFCODE, size); - + void *new_loc = cbfs_cbmem_alloc(prog_name(fsp_relocd), + CBMEM_ID_REFCODE, &size); if (new_loc == NULL) { printk(BIOS_ERR, "ERROR: Unable to load FSP into memory.\n"); return -1; }
- if (rdev_readat(fsp_src, new_loc, 0, size) != size) { - printk(BIOS_ERR, "ERROR: Can't read FSP's region device.\n"); - return -1; - } - fih_offset = fsp1_1_relocate((uintptr_t)new_loc, new_loc, size);
if (fih_offset <= 0) { diff --git a/src/drivers/intel/fsp1_1/include/fsp/util.h b/src/drivers/intel/fsp1_1/include/fsp/util.h index cab867a..bc34e92 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/util.h +++ b/src/drivers/intel/fsp1_1/include/fsp/util.h @@ -41,10 +41,10 @@
/* * Relocate FSP entire binary into ram. Returns < 0 on error, 0 on success. - * The FSP source is pointed to by region_device and the relocation information + * The CBFS file name of the FSP source and the relocation information * is encoded in a struct prog with its entry point set to the FSP info header. */ -int fsp_relocate(struct prog *fsp_relocd, const struct region_device *fsp_src); +int fsp_relocate(struct prog *fsp_relocd);
/* Additional HOB types not included in the FSP: * #define EFI_HOB_TYPE_HANDOFF 0x0001 diff --git a/src/drivers/intel/fsp1_1/ramstage.c b/src/drivers/intel/fsp1_1/ramstage.c index eb226db..7ad6148 100644 --- a/src/drivers/intel/fsp1_1/ramstage.c +++ b/src/drivers/intel/fsp1_1/ramstage.c @@ -158,21 +158,6 @@ stage_cache_add(STAGE_REFCODE, fsp); }
-static int fsp_find_and_relocate(struct prog *fsp) -{ - if (prog_locate(fsp)) { - printk(BIOS_ERR, "ERROR: Couldn't find %s\n", prog_name(fsp)); - return -1; - } - - if (fsp_relocate(fsp, prog_rdev(fsp))) { - printk(BIOS_ERR, "ERROR: FSP relocation failed.\n"); - return -1; - } - - return 0; -} - void fsp_load(void) { static int load_done; @@ -186,7 +171,7 @@ printk(BIOS_DEBUG, "FSP: Loading binary from cache\n"); stage_cache_load_stage(STAGE_REFCODE, &fsp); } else { - fsp_find_and_relocate(&fsp); + fsp_relocate(&fsp); fsp_cache_save(&fsp); }
diff --git a/src/drivers/intel/fsp1_1/temp_ram_exit.c b/src/drivers/intel/fsp1_1/temp_ram_exit.c index 08324eb..a4e9227 100644 --- a/src/drivers/intel/fsp1_1/temp_ram_exit.c +++ b/src/drivers/intel/fsp1_1/temp_ram_exit.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <cbfs.h> #include <console/console.h> #include <fsp/util.h>
@@ -8,9 +9,9 @@ FSP_INFO_HEADER *fih; uint32_t status; FSP_TEMP_RAM_EXIT temp_ram_exit; - struct prog fsp = PROG_INIT(PROG_REFCODE, "fsp.bin"); + void *fsp = cbfs_map("fsp.bin", NULL);
- if (prog_locate(&fsp)) { + if (!fsp) { die("Unable to locate fsp.bin\n"); } else { /* This leaks a mapping which this code assumes is benign as @@ -20,7 +21,7 @@ as it casts error values to FSP_INFO_HEADER pointers. Checking for return values can only be done sanely once that is fixed. */ - fih = find_fsp((uintptr_t)rdev_mmap_full(prog_rdev(&fsp))); + fih = find_fsp((uintptr_t)fsp); }
temp_ram_exit = (FSP_TEMP_RAM_EXIT)(fih->TempRamExitEntryOffset + diff --git a/src/include/program_loading.h b/src/include/program_loading.h index c9ec9c2..f98a382 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -40,8 +40,7 @@
/* Representation of a program. */ struct prog { - /* The region_device is the source of program content to load. After - * loading program it represents the memory region of the stages and + /* The region_device represents the memory region of the stages and * payload. For architectures that use a bounce buffer * then it would represent the bounce buffer. */ enum prog_type type; @@ -127,13 +126,10 @@ prog->arg = arg; }
-/* Locate the identified program to run. Return 0 on success. < 0 on error. */ -int prog_locate(struct prog *prog); /* The prog_locate_hook() is called prior to CBFS traversal. The hook can be - * used to implement policy that allows or prohibits further progress through - * prog_locate(). The type and name field within struct prog are the only valid - * fields. A 0 return value allows further progress while a non-zero return - * value prohibits further progress */ + * used to implement policy that allows or prohibits further program loading. + * The type and name field within struct prog are the only valid fields. A 0 + * return value allows loading while a non-zero return value prohibits it. */ int prog_locate_hook(struct prog *prog);
/* Run the program described by prog. */ @@ -146,17 +142,6 @@ * code it needs to that as well. */ void platform_prog_run(struct prog *prog);
-struct prog_loader_ops { - const char *name; - /* Determine if the loader is the active one. If so returns 1 else 0 - * or < 0 on error. */ - int (*is_loader_active)(struct prog *prog); - /* Returns < 0 on error or 0 on success. This function locates - * the rdev representing the file data associated with the passed in - * prog. */ - int (*locate)(struct prog *prog); -}; - /************************ * ROMSTAGE LOADING * ************************/ diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 5b584fe..8a3102c 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -21,23 +21,6 @@ const struct mem_region_device addrspace_32bit = MEM_REGION_DEV_RO_INIT(0, ~0UL);
-int prog_locate(struct prog *prog) -{ - struct cbfsf file; - - if (prog_locate_hook(prog)) - return -1; - - if (cbfs_boot_locate(&file, prog_name(prog), NULL)) - return -1; - - cbfsf_file_type(&file, &prog->cbfs_type); - - cbfs_file_data(prog_rdev(prog), &file); - - return 0; -} - void run_romstage(void) { struct prog romstage =
-
Julius Werner (Code Review)