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/+/49335
to review the following change.
Change subject: cbfs: Remove prog_locate() for stages and rmodules ......................................................................
cbfs: Remove prog_locate() for stages and rmodules
This patch removes the prog_locate() step for stages and rmodules. Instead, the stage and rmodule loading functions will now perform the locate step directly together with the actual loading. The long-term goal of this is to eliminate prog_locate() (and the rdev member in struct prog that it fills) completely in order to make CBFS verification code safer and its security guarantees easier to follow. prog_locate() is the main remaining use case where a raw rdev of CBFS file data "leaks" out of cbfs.c into other code, and that other code needs to manually make sure that the contents of the rdev get verified during loading. By eliminating this step and moving all code that directly deals with file data into cbfs.c, we can concentrate the code that needs to worry about file data hashing (and needs access to cbfs_private.h APIs) into one file, making it easier to keep track of and reason about.
This patch is the first step of this move, later patches will do the same for SELFs and other program types.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia600e55f77c2549a00e2606f09befc1f92594a3a --- M src/arch/arm64/bl31.c M src/arch/x86/bootblock_normal.c M src/arch/x86/postcar_loader.c M src/include/program_loading.h M src/lib/cbfs.c M src/lib/prog_loaders.c M src/lib/rmodule.c M src/security/vboot/vboot_loader.c M src/soc/amd/common/block/pi/refcode_loader.c M src/soc/intel/baytrail/refcode.c M src/soc/intel/broadwell/refcode.c M src/soc/mediatek/mt8183/memory.c M src/soc/mediatek/mt8192/memory.c M src/soc/qualcomm/common/qclib.c 14 files changed, 46 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/49335/1
diff --git a/src/arch/arm64/bl31.c b/src/arch/arm64/bl31.c index caa1136..a387027 100644 --- a/src/arch/arm64/bl31.c +++ b/src/arch/arm64/bl31.c @@ -87,9 +87,6 @@ struct prog bl32 = PROG_INIT(PROG_BL32, CONFIG_CBFS_PREFIX"/secure_os");
- if (prog_locate(&bl32)) - die("BL32 not found"); - if (cbfs_prog_stage_load(&bl32)) die("BL32 load failed");
diff --git a/src/arch/x86/bootblock_normal.c b/src/arch/x86/bootblock_normal.c index e2dfcf4..9341ac3 100644 --- a/src/arch/x86/bootblock_normal.c +++ b/src/arch/x86/bootblock_normal.c @@ -12,7 +12,7 @@ return ++stagelist; }
-int legacy_romstage_selector(struct prog *romstage) +int legacy_romstage_select_and_load(struct prog *romstage) { static const char *default_filenames = "normal/romstage\0fallback/romstage"; const char *boot_candidate; @@ -24,10 +24,10 @@
if (do_normal_boot()) { romstage->name = boot_candidate; - if (!prog_locate(romstage)) + if (!cbfs_prog_stage_load(romstage)) return 0; }
romstage->name = get_fallback(boot_candidate); - return prog_locate(romstage); + return cbfs_prog_stage_load(romstage); } diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 241ba8c..fc4d426 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -136,9 +136,6 @@
vboot_run_logic();
- if (prog_locate(prog)) - die_with_post_code(POST_INVALID_ROM, - "Failed to locate after CAR program.\n"); if (rmodule_stage_load(&rsl)) die_with_post_code(POST_INVALID_ROM, "Failed to load after CAR program.\n"); diff --git a/src/include/program_loading.h b/src/include/program_loading.h index fae70ce..7ceecc4 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -165,7 +165,7 @@ void run_romstage(void);
/* Runtime selector for CBFS_PREFIX of romstage. */ -int legacy_romstage_selector(struct prog *romstage); +int legacy_romstage_select_and_load(struct prog *romstage);
/************************ * RAMSTAGE LOADING * diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 54896d1..476146e 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -377,19 +377,29 @@ return cbmem_add((uintptr_t)arg, size); }
-int cbfs_prog_stage_load(struct prog *pstage) +cb_err_t cbfs_prog_stage_load(struct prog *pstage) { + union cbfs_mdata mdata; + struct region_device rdev; struct cbfs_stage stage; uint8_t *load; void *entry; size_t fsize; size_t foffset; - const struct region_device *fh = prog_rdev(pstage); + cb_err_t err;
- if (rdev_readat(fh, &stage, 0, sizeof(stage)) != sizeof(stage)) - return -1; + prog_locate_hook(pstage);
- fsize = region_device_sz(fh); + if ((err = cbfs_boot_lookup(prog_name(pstage), false, &mdata, &rdev))) + return err; + + assert(be32toh(mdata.h.type) == CBFS_TYPE_STAGE); + pstage->cbfs_type = CBFS_TYPE_STAGE; + + if (rdev_readat(&rdev, &stage, 0, sizeof(stage)) != sizeof(stage)) + return CB_CBFS_IO; + + fsize = region_device_sz(&rdev); fsize -= sizeof(stage); foffset = 0; foffset += sizeof(stage); @@ -411,16 +421,16 @@ * that would hit this path initialize themselves. */ if ((ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) && !CONFIG(NO_XIP_EARLY_STAGES) && CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) { - void *mapping = rdev_mmap(fh, foffset, fsize); - rdev_munmap(fh, mapping); + void *mapping = rdev_mmap(&rdev, foffset, fsize); + rdev_munmap(&rdev, mapping); if (mapping == load) goto out; }
- fsize = cbfs_stage_load_and_decompress(fh, foffset, fsize, load, - stage.memlen, stage.compression); + fsize = cbfs_stage_load_and_decompress(&rdev, foffset, fsize, load, + stage.memlen, stage.compression); if (!fsize) - return -1; + return CB_ERR;
/* Clear area not covered by file. */ memset(&load[fsize], 0, stage.memlen - fsize); @@ -431,7 +441,7 @@ prog_set_area(pstage, load, stage.memlen); prog_set_entry(pstage, entry, NULL);
- return 0; + return CB_SUCCESS; }
void cbfs_boot_device_find_mcache(struct cbfs_boot_device *cbd, uint32_t id) diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 93efc0a..964b7f5 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -46,18 +46,15 @@
vboot_run_logic();
- if (ENV_X86 && CONFIG(BOOTBLOCK_NORMAL)) { - if (legacy_romstage_selector(&romstage)) - goto fail; - } else { - if (prog_locate(&romstage)) - goto fail; - } - timestamp_add_now(TS_START_COPYROM);
- if (cbfs_prog_stage_load(&romstage)) - goto fail; + if (ENV_X86 && CONFIG(BOOTBLOCK_NORMAL)) { + if (legacy_romstage_select_and_load(&romstage)) + goto fail; + } else { + if (cbfs_prog_stage_load(&romstage)) + goto fail; + }
timestamp_add_now(TS_END_COPYROM);
@@ -82,6 +79,7 @@ /* Load the cached ramstage to runtime location. */ stage_cache_load_stage(STAGE_RAMSTAGE, ramstage);
+ ramstage->cbfs_type = CBFS_TYPE_STAGE; prog_set_arg(ramstage, cbmem_top());
if (prog_entry(ramstage) != NULL) { @@ -124,9 +122,6 @@
vboot_run_logic();
- if (prog_locate(&ramstage)) - goto fail; - timestamp_add_now(TS_START_COPYRAM);
if (ENV_X86) { diff --git a/src/lib/rmodule.c b/src/lib/rmodule.c index 7944535..6ea9db7 100644 --- a/src/lib/rmodule.c +++ b/src/lib/rmodule.c @@ -2,6 +2,7 @@ #include <assert.h> #include <cbmem.h> #include <cbfs.h> +#include <cbfs_private.h> #include <stdint.h> #include <stdlib.h> #include <string.h> @@ -244,14 +245,22 @@ int load_offset; struct cbfs_stage stage; void *rmod_loc; - struct region_device *fh; + struct region_device rdev; + union cbfs_mdata mdata;
if (rsl->prog == NULL || prog_name(rsl->prog) == NULL) return -1;
- fh = prog_rdev(rsl->prog); + if (prog_locate_hook(rsl->prog)) + return -1;
- if (rdev_readat(fh, &stage, 0, sizeof(stage)) != sizeof(stage)) + if (cbfs_boot_lookup(prog_name(rsl->prog), false, &mdata, &rdev) != CB_SUCCESS) + return -1; + + assert(be32toh(mdata.h.type) == CBFS_TYPE_STAGE); + rsl->prog->cbfs_type = CBFS_TYPE_STAGE; + + if (rdev_readat(&rdev, &stage, 0, sizeof(stage)) != sizeof(stage)) return -1;
rmodule_offset = @@ -268,7 +277,7 @@ printk(BIOS_INFO, "Decompressing stage %s @ %p (%d bytes)\n", prog_name(rsl->prog), rmod_loc, stage.memlen);
- if (!cbfs_load_and_decompress(fh, sizeof(stage), stage.len, rmod_loc, + if (!cbfs_load_and_decompress(&rdev, sizeof(stage), stage.len, rmod_loc, stage.memlen, stage.compression)) return -1;
diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 56a0664..9ca6259 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -45,19 +45,12 @@ verstage_main(); after_verstage(); } else if (verstage_should_load()) { - struct cbfsf file; struct prog verstage = PROG_INIT(PROG_VERSTAGE, CONFIG_CBFS_PREFIX "/verstage");
printk(BIOS_DEBUG, "VBOOT: Loading verstage.\n");
- /* load verstage from RO */ - if (cbfs_boot_locate(&file, prog_name(&verstage), NULL)) - die("failed to load verstage"); - - cbfs_file_data(prog_rdev(&verstage), &file); - if (cbfs_prog_stage_load(&verstage)) die("failed to load verstage");
diff --git a/src/soc/amd/common/block/pi/refcode_loader.c b/src/soc/amd/common/block/pi/refcode_loader.c index 2fdbe83..01783c5 100644 --- a/src/soc/amd/common/block/pi/refcode_loader.c +++ b/src/soc/amd/common/block/pi/refcode_loader.c @@ -53,9 +53,6 @@ // There is no way to tell if this succeeded. stage_cache_load_stage(STAGE_REFCODE, &prog); } else { - if (prog_locate(&prog)) - return -1; - if (rmodule_stage_load(&rmod_agesa) < 0) return -1;
diff --git a/src/soc/intel/baytrail/refcode.c b/src/soc/intel/baytrail/refcode.c index 297ed9c..b79a2b4 100644 --- a/src/soc/intel/baytrail/refcode.c +++ b/src/soc/intel/baytrail/refcode.c @@ -41,11 +41,6 @@ return load_refcode_from_cache(); }
- if (prog_locate(&prog)) { - printk(BIOS_DEBUG, "Couldn't locate reference code.\n"); - return NULL; - } - if (rmodule_stage_load(&refcode)) { printk(BIOS_DEBUG, "Error loading reference code.\n"); return NULL; diff --git a/src/soc/intel/broadwell/refcode.c b/src/soc/intel/broadwell/refcode.c index 882f888..c0c16ea 100644 --- a/src/soc/intel/broadwell/refcode.c +++ b/src/soc/intel/broadwell/refcode.c @@ -36,11 +36,6 @@ if (acpi_is_wakeup_s3()) return load_refcode_from_cache();
- if (prog_locate(&prog)) { - printk(BIOS_DEBUG, "Couldn't locate reference code.\n"); - return NULL; - } - if (rmodule_stage_load(&refcode)) { printk(BIOS_DEBUG, "Error loading reference code.\n"); return NULL; diff --git a/src/soc/mediatek/mt8183/memory.c b/src/soc/mediatek/mt8183/memory.c index 6b80a43..a43cdea 100644 --- a/src/soc/mediatek/mt8183/memory.c +++ b/src/soc/mediatek/mt8183/memory.c @@ -105,9 +105,6 @@ /* Load and run the provided blob for full-calibration if available */ struct prog dram = PROG_INIT(PROG_REFCODE, CONFIG_CBFS_PREFIX "/dram");
- if (prog_locate(&dram)) - return -1; - if (cbfs_prog_stage_load(&dram)) return -2;
diff --git a/src/soc/mediatek/mt8192/memory.c b/src/soc/mediatek/mt8192/memory.c index 549dede..d39ecba 100644 --- a/src/soc/mediatek/mt8192/memory.c +++ b/src/soc/mediatek/mt8192/memory.c @@ -80,11 +80,6 @@
initialize_dramc_param(dparam);
- if (prog_locate(&dram)) { - printk(BIOS_ERR, "DRAM-K: Locate program failed\n"); - return -1; - } - if (cbfs_prog_stage_load(&dram)) { printk(BIOS_ERR, "DRAM-K: CBFS load program failed\n"); return -2; diff --git a/src/soc/qualcomm/common/qclib.c b/src/soc/qualcomm/common/qclib.c index 9358890..f374f78 100644 --- a/src/soc/qualcomm/common/qclib.c +++ b/src/soc/qualcomm/common/qclib.c @@ -158,9 +158,6 @@ CONFIG_CBFS_PREFIX "/qcsdi");
/* Attempt to load QCSDI elf */ - if (prog_locate(&qcsdi)) - goto fail; - if (cbfs_prog_stage_load(&qcsdi)) goto fail;
@@ -175,9 +172,6 @@ struct prog qclib = PROG_INIT(PROG_REFCODE, CONFIG_CBFS_PREFIX "/qclib");
- if (prog_locate(&qclib)) - goto fail; - if (cbfs_prog_stage_load(&qclib)) goto fail;
-
Julius Werner (Code Review)