From 9554b26f9fd608cc613fc3ad869db33ef0edfe5c Mon Sep 17 00:00:00 2001 From: Patrick Rudolph Date: Tue, 5 Jun 2018 15:12:56 +0200 Subject: [PATCH] vboot: Fix linking error with USE_OPTION_TABLE enabled Fix a linking problem with VBOOT and USE_OPTION_TABLE enabled. Make use of cbfs_locate_file_in_region() and always search the cmos_layout.bin in the 'COREBOOT' region. With this change applied there's no need to include the vboot_locator in SMM any more, we can't break NVRAM with different CMOS layouts, and we keep VBOOT and non VBOOT behaviour the same. Only include cmos_layout.bin and cmos.default in RO region. Add notes explaining the decisions. Tested on Intel Sandybridge, builds and boots with vboot enabled. Change-Id: I10ae94d7936581bbb5ea49384122062bd4934ea5 Signed-off-by: Patrick Rudolph Reviewed-on: https://review.coreboot.org/26863 Tested-by: build bot (Jenkins) Reviewed-by: Aaron Durbin --- src/drivers/pc80/rtc/mc146818rtc.c | 72 ++++++++++++++++++++++++------ src/lib/cbfs.c | 5 +++ src/security/vboot/Makefile.inc | 2 + 3 files changed, 66 insertions(+), 13 deletions(-) diff --git a/src/drivers/pc80/rtc/mc146818rtc.c b/src/drivers/pc80/rtc/mc146818rtc.c index 928b403239..34db4c3424 100644 --- a/src/drivers/pc80/rtc/mc146818rtc.c +++ b/src/drivers/pc80/rtc/mc146818rtc.c @@ -240,9 +240,34 @@ static enum cb_err get_cmos_value(unsigned long bit, unsigned long length, return CB_SUCCESS; } +static enum cb_err locate_cmos_layout(struct region_device *rdev) +{ + uint32_t cbfs_type = CBFS_COMPONENT_CMOS_LAYOUT; + struct cbfsf fh; + + /* + * In case VBOOT is enabled and this function is called from SMM, + * we have multiple CMOS layout files and to locate them we'd need to + * include VBOOT into SMM... + * + * Support only one CMOS layout in the 'COREBOOT' region for now. + */ + if (cbfs_locate_file_in_region(&fh, "COREBOOT", "cmos_layout.bin", + &cbfs_type)) { + printk(BIOS_ERR, "RTC: cmos_layout.bin could not be found. " + "Options are disabled\n"); + return CB_CMOS_LAYOUT_NOT_FOUND; + } + + cbfs_file_data(rdev, &fh); + + return CB_SUCCESS; +} + enum cb_err get_option(void *dest, const char *name) { struct cmos_option_table *ct; + struct region_device rdev; struct cmos_entries *ce; size_t namelen; int found = 0; @@ -255,16 +280,20 @@ enum cb_err get_option(void *dest, const char *name) /* Figure out how long name is */ namelen = strnlen(name, CMOS_MAX_NAME_LENGTH); - /* find the requested entry record */ - ct = cbfs_boot_map_with_leak("cmos_layout.bin", - CBFS_COMPONENT_CMOS_LAYOUT, NULL); + if (locate_cmos_layout(&rdev) != CB_SUCCESS) { + UNLOCK_NVRAM_CBFS_SPINLOCK(); + return CB_CMOS_LAYOUT_NOT_FOUND; + } + ct = rdev_mmap_full(&rdev); if (!ct) { - printk(BIOS_ERR, "RTC: cmos_layout.bin could not be found. " - "Options are disabled\n"); + printk(BIOS_ERR, "RTC: cmos_layout.bin could not be mapped. " + "Options are disabled\n"); UNLOCK_NVRAM_CBFS_SPINLOCK(); return CB_CMOS_LAYOUT_NOT_FOUND; } + + /* find the requested entry record */ ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length); for (; ce->tag == LB_TAG_OPTION; ce = (struct cmos_entries *)((unsigned char *)ce + ce->size)) { @@ -275,18 +304,22 @@ enum cb_err get_option(void *dest, const char *name) } if (!found) { printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name); + rdev_munmap(&rdev, ct); UNLOCK_NVRAM_CBFS_SPINLOCK(); return CB_CMOS_OPTION_NOT_FOUND; } if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC)) { + rdev_munmap(&rdev, ct); UNLOCK_NVRAM_CBFS_SPINLOCK(); return CB_CMOS_CHECKSUM_INVALID; } if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) { + rdev_munmap(&rdev, ct); UNLOCK_NVRAM_CBFS_SPINLOCK(); return CB_CMOS_ACCESS_ERROR; } + rdev_munmap(&rdev, ct); UNLOCK_NVRAM_CBFS_SPINLOCK(); return CB_SUCCESS; } @@ -347,6 +380,7 @@ unsigned int read_option_lowlevel(unsigned int start, unsigned int size, enum cb_err set_option(const char *name, void *value) { struct cmos_option_table *ct; + struct region_device rdev; struct cmos_entries *ce; unsigned long length; size_t namelen; @@ -358,14 +392,20 @@ enum cb_err set_option(const char *name, void *value) /* Figure out how long name is */ namelen = strnlen(name, CMOS_MAX_NAME_LENGTH); - /* find the requested entry record */ - ct = cbfs_boot_map_with_leak("cmos_layout.bin", - CBFS_COMPONENT_CMOS_LAYOUT, NULL); - if (!ct) { - printk(BIOS_ERR, "cmos_layout.bin could not be found. " - "Options are disabled\n"); + if (locate_cmos_layout(&rdev) != CB_SUCCESS) { + UNLOCK_NVRAM_CBFS_SPINLOCK(); return CB_CMOS_LAYOUT_NOT_FOUND; } + ct = rdev_mmap_full(&rdev); + if (!ct) { + printk(BIOS_ERR, "RTC: cmos_layout.bin could not be mapped. " + "Options are disabled\n"); + + UNLOCK_NVRAM_CBFS_SPINLOCK(); + return CB_CMOS_LAYOUT_NOT_FOUND; + } + + /* find the requested entry record */ ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length); for (; ce->tag == LB_TAG_OPTION; ce = (struct cmos_entries *)((unsigned char *)ce + ce->size)) { @@ -376,6 +416,7 @@ enum cb_err set_option(const char *name, void *value) } if (!found) { printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name); + rdev_munmap(&rdev, ct); return CB_CMOS_OPTION_NOT_FOUND; } @@ -384,13 +425,18 @@ enum cb_err set_option(const char *name, void *value) length = MAX(strlen((const char *)value) * 8, ce->length - 8); /* make sure the string is null terminated */ if (set_cmos_value(ce->bit + ce->length - 8, 8, &(u8[]){0}) - != CB_SUCCESS) + != CB_SUCCESS) { + rdev_munmap(&rdev, ct); return CB_CMOS_ACCESS_ERROR; + } } - if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) + if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) { + rdev_munmap(&rdev, ct); return CB_CMOS_ACCESS_ERROR; + } + rdev_munmap(&rdev, ct); return CB_SUCCESS; } diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 9938e6abbe..ca1fc8477e 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -321,6 +321,11 @@ extern const struct cbfs_locator vboot_locator; static const struct cbfs_locator *locators[] = { #if IS_ENABLED(CONFIG_VBOOT) + /* + * NOTE: Does not link in SMM, as the vboot_locator isn't compiled. + * ATM there's no need for VBOOT functionality in SMM and it's not + * a problem. + */ &vboot_locator, #endif &cbfs_master_header_locator, diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 6f18a35de5..75c2a9e44c 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -154,6 +154,8 @@ regions-for-file = $(subst $(spc),$(comma),$(sort \ font.bin \ vbgfx.bin \ rmu.bin \ + cmos_layout.bin \ + cmos.default \ $(call strip_quotes,$(CONFIG_RO_REGION_ONLY)) \ ,$(1)),COREBOOT,COREBOOT FW_MAIN_A FW_MAIN_B)))