coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Change in coreboot[master]: soc/intel/alderlake: Add TDP to give correct VR configuration.
by Paul Menzel (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Tim Wawrzynczak, Curtis Chen, Patrick Rudolph. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59644 ) Change subject: soc/intel/alderlake: Add TDP to give correct VR configuration. ...................................................................... Patch Set 1: (4 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/59644/comment/999533eb_934bd76c PS1, Line 7: soc/intel/alderlake: Add TDP to give correct VR configuration. Please remove the dot/period at the end [1]. [1]: https://chris.beams.io/posts/git-commit/ https://review.coreboot.org/c/coreboot/+/59644/comment/3a9a5274_20823103 PS1, Line 11: Add TDP to recognize the correct SKU and give the correct power setting. Please add a blank line between paragraphs. File src/soc/intel/alderlake/vr_config.c: https://review.coreboot.org/c/coreboot/+/59644/comment/78235ea9_ffd1a953 PS1, Line 36: u8 tdp; Please be consistent, so `uint8_t`. https://review.coreboot.org/c/coreboot/+/59644/comment/9525417f_0c86a47d PS1, Line 83: { PCI_DEVICE_ID_INTEL_ADL_P_ID_3, 28, VR_CFG_ALL_DOMAINS_TDC_CURRENT(40, 40) }, That line is new. Maybe make it a separate commit? -- To view, visit https://review.coreboot.org/c/coreboot/+/59644 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4d31e7afc76d9a8c772781671f92ec08f9d8713f Gerrit-Change-Number: 59644 Gerrit-PatchSet: 1 Gerrit-Owner: Curtis Chen <curtis.chen(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Curtis Chen <curtis.chen(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Thu, 25 Nov 2021 22:31:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cbfs: Remove deprecated APIs
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Jakub Czapiga, Christian Walter, Yu-Ping Wu. Hello Jakub Czapiga, Christian Walter, Yu-Ping Wu, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59682 to look at the new patch set (#2). Change subject: cbfs: Remove deprecated APIs ...................................................................... cbfs: Remove deprecated APIs This patch removes all remaining pieces of the old CBFS API, now that the last straggling use cases of it have been ported to the new one. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I1cec0ca2d9d311626a087318d1d78163243bfc3c --- M src/commonlib/Makefile.inc D src/commonlib/cbfs.c D src/commonlib/include/commonlib/cbfs.h M src/include/cbfs.h M src/lib/cbfs.c M src/security/tpm/tspi/crtm.h M src/security/vboot/Kconfig 7 files changed, 2 insertions(+), 483 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/59682/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59682 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1cec0ca2d9d311626a087318d1d78163243bfc3c Gerrit-Change-Number: 59682 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: cbfs | tspi: Join hash calculation for verification and measurement
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Jakub Czapiga, Christian Walter, Arthur Heymans, Werner Zeh. Hello Jakub Czapiga, Christian Walter, Arthur Heymans, Werner Zeh, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59681 to look at the new patch set (#2). Change subject: cbfs | tspi: Join hash calculation for verification and measurement ...................................................................... cbfs | tspi: Join hash calculation for verification and measurement This patch moves the CBFS file measurement when CONFIG_TPM_MEASURED_BOOT is enabled from the lookup step into the code where a file is actually loaded or mapped from flash. This has the advantage that CBFS routines which just look up a file to inspect its metadata (e.g. cbfs_get_size()) do not cause the file to be measured twice. It also removes the existing inefficiency that files are loaded twice when measurement is enabled (once to measure and then again when they are used). When CBFS verification is enabled and uses the same hash algorithm as the TPM, we are even able to only hash the file a single time and use the result for both purposes. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I70d7066c6768195077f083c7ffdfa30d9182b2b7 --- M src/lib/cbfs.c M src/security/tpm/tspi.h M src/security/tpm/tspi/crtm.c M src/security/tpm/tspi/crtm.h M src/security/tpm/tspi/tspi.c 5 files changed, 57 insertions(+), 94 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/59681/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59681 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I70d7066c6768195077f083c7ffdfa30d9182b2b7 Gerrit-Change-Number: 59681 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: cbfs: Add unverified_area APIs
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Raul Rangel, Jakub Czapiga. Hello Raul Rangel, Jakub Czapiga, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59678 to look at the new patch set (#2). Change subject: cbfs: Add unverified_area APIs ...................................................................... cbfs: Add unverified_area APIs This patch adds a new ..._unverified_area_... group of functions to the cbfs_map/_load/_alloc() APIs. These functions can be used to access custom FMAP sections and are meant to replace the existing cbfs_locate_file_in_region(). The name is intended to highlight that accesses through this API will not be verified when CBFS_VERIFICATION is enabled and should always be treated as if they may return malicious data. (Due to laziness I'm not adding the combination of this API with the ..._type_... variant at this point, since it seems very unlikely that we'll ever have a use case for that. If we ever do, it should be easy to add later.) (Also remove the 'inline' from cbfs_file_hash_mismatch(). I'm not sure why I put it there in the first place, probably a bad copy&paste.) Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I402265900f7075aa0c2f58d812c67ea63ddf2900 --- M src/include/cbfs.h M src/lib/cbfs.c 2 files changed, 151 insertions(+), 78 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/59678/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59678 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I402265900f7075aa0c2f58d812c67ea63ddf2900 Gerrit-Change-Number: 59678 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: cbfs: Add unverified_area APIs
by build bot (Jenkins) (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Raul Rangel, Jakub Czapiga. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59678 ) Change subject: cbfs: Add unverified_area APIs ...................................................................... Patch Set 1: (1 comment) File src/lib/cbfs.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-134409): https://review.coreboot.org/c/coreboot/+/59678/comment/5e051cf4_adc8fa49 PS1, Line 531: if (tspi_measure_cbfs_hook(&file_rdev, name, be32toh(mdata.h.type))) { braces {} are not necessary for single statement blocks -- To view, visit https://review.coreboot.org/c/coreboot/+/59678 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I402265900f7075aa0c2f58d812c67ea63ddf2900 Gerrit-Change-Number: 59678 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Comment-Date: Thu, 25 Nov 2021 22:19:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cbfs: Remove deprecated APIs
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Christian Walter, Yu-Ping Wu. Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59682 ) Change subject: cbfs: Remove deprecated APIs ...................................................................... cbfs: Remove deprecated APIs This patch removes all remaining pieces of the old CBFS API, now that the last straggling use cases of it have been ported to the new one. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I1cec0ca2d9d311626a087318d1d78163243bfc3c --- M src/commonlib/Makefile.inc D src/commonlib/cbfs.c D src/commonlib/include/commonlib/cbfs.h M src/include/cbfs.h M src/lib/cbfs.c M src/security/tpm/tspi/crtm.h M src/security/vboot/Kconfig 7 files changed, 2 insertions(+), 483 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/59682/1 diff --git a/src/commonlib/Makefile.inc b/src/commonlib/Makefile.inc index 9347f88..2477e07 100644 --- a/src/commonlib/Makefile.inc +++ b/src/commonlib/Makefile.inc @@ -27,13 +27,6 @@ endif ramstage-$(CONFIG_PLATFORM_USES_FSP2_0) += fsp_relocate.c -bootblock-y += cbfs.c -verstage-y += cbfs.c -romstage-y += cbfs.c -ramstage-y += cbfs.c -smm-y += cbfs.c -postcar-y += cbfs.c - bootblock-y += bsd/cbfs_private.c verstage-y += bsd/cbfs_private.c romstage-y += bsd/cbfs_private.c diff --git a/src/commonlib/cbfs.c b/src/commonlib/cbfs.c deleted file mode 100644 index e7f800c..0000000 --- a/src/commonlib/cbfs.c +++ /dev/null @@ -1,347 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <commonlib/cbfs.h> -#include <commonlib/endian.h> -#include <commonlib/helpers.h> -#include <string.h> -#include <vb2_sha.h> - -static size_t cbfs_next_offset(const struct region_device *cbfs, - const struct cbfsf *f) -{ - size_t offset; - - if (f == NULL) - return 0; - - /* The region_device objects store absolute offsets over the whole - * region. Therefore a relative offset needs to be calculated. */ - offset = rdev_relative_offset(cbfs, &f->data); - offset += region_device_sz(&f->data); - - return ALIGN_UP(offset, CBFS_ALIGNMENT); -} - -static int cbfs_end(const struct region_device *cbfs, size_t offset) -{ - if (offset >= region_device_sz(cbfs)) - return 1; - - return 0; -} - -int cbfs_for_each_file(const struct region_device *cbfs, - const struct cbfsf *prev, struct cbfsf *fh) -{ - size_t offset; - - offset = cbfs_next_offset(cbfs, prev); - - /* Try to scan the entire cbfs region looking for file name. */ - while (1) { - struct cbfs_file file; - const size_t fsz = sizeof(file); - - DEBUG("Checking offset %zx\n", offset); - - /* End of region. */ - if (cbfs_end(cbfs, offset)) - return 1; - - /* Can't read file. Nothing else to do but bail out. */ - if (rdev_readat(cbfs, &file, offset, fsz) != fsz) - break; - - if (memcmp(file.magic, CBFS_FILE_MAGIC, sizeof(file.magic))) { - offset++; - offset = ALIGN_UP(offset, CBFS_ALIGNMENT); - continue; - } - - file.len = read_be32(&file.len); - file.offset = read_be32(&file.offset); - - DEBUG("File @ offset %zx size %x\n", offset, file.len); - - /* Keep track of both the metadata and the data for the file. */ - if (rdev_chain(&fh->metadata, cbfs, offset, file.offset)) - break; - - if (rdev_chain(&fh->data, cbfs, offset + file.offset, file.len)) - break; - - /* Success. */ - return 0; - } - - return -1; -} - -size_t cbfs_for_each_attr(void *metadata, size_t metadata_size, - size_t last_offset) -{ - struct cbfs_file_attribute *attr; - - if (!last_offset) { - struct cbfs_file *file = metadata; - size_t start_offset = read_be32(&file->attributes_offset); - if (start_offset <= sizeof(struct cbfs_file) || - start_offset + sizeof(*attr) > metadata_size) - return 0; - return start_offset; - } - - attr = metadata + last_offset; - size_t next_offset = last_offset + read_be32(&attr->len); - - if (next_offset + sizeof(*attr) > metadata_size) - return 0; - return next_offset; -} - -int cbfsf_decompression_info(struct cbfsf *fh, uint32_t *algo, size_t *size) -{ - size_t metadata_size = region_device_sz(&fh->metadata); - void *metadata = rdev_mmap_full(&fh->metadata); - size_t offs = 0; - - if (!metadata) - return -1; - - while ((offs = cbfs_for_each_attr(metadata, metadata_size, offs))) { - struct cbfs_file_attr_compression *attr = metadata + offs; - if (read_be32(&attr->tag) != CBFS_FILE_ATTR_TAG_COMPRESSION) - continue; - - *algo = read_be32(&attr->compression); - *size = read_be32(&attr->decompressed_size); - rdev_munmap(&fh->metadata, metadata); - return 0; - } - - *algo = CBFS_COMPRESS_NONE; - *size = region_device_sz(&fh->data); - rdev_munmap(&fh->metadata, metadata); - return 0; -} - -int cbfsf_file_type(struct cbfsf *fh, uint32_t *ftype) -{ - const size_t sz = sizeof(*ftype); - - if (rdev_readat(&fh->metadata, ftype, - offsetof(struct cbfs_file, type), sz) != sz) - return -1; - - *ftype = read_be32(ftype); - - return 0; -} - -int cbfs_locate(struct cbfsf *fh, const struct region_device *cbfs, - const char *name, uint32_t *type) -{ - struct cbfsf *prev; - - LOG("Locating '%s'\n", name); - - prev = NULL; - - while (1) { - int ret; - char *fname; - int name_match; - const size_t fsz = sizeof(struct cbfs_file); - - ret = cbfs_for_each_file(cbfs, prev, fh); - prev = fh; - - /* Either failed to read or hit the end of the region. */ - if (ret < 0 || ret > 0) - break; - - fname = rdev_mmap(&fh->metadata, fsz, - region_device_sz(&fh->metadata) - fsz); - - if (fname == NULL) - break; - - name_match = !strcmp(fname, name); - rdev_munmap(&fh->metadata, fname); - - if (!name_match) { - DEBUG(" Unmatched '%s' at %zx\n", fname, - rdev_relative_offset(cbfs, &fh->metadata)); - continue; - } - - if (type != NULL) { - uint32_t ftype; - - if (cbfsf_file_type(fh, &ftype)) - break; - - if (*type != 0 && *type != ftype) { - DEBUG(" Unmatched type %x at %zx\n", ftype, - rdev_relative_offset(cbfs, - &fh->metadata)); - continue; - } - // *type being 0 means we want to know ftype. - // We could just do a blind assignment but - // if type is pointing to read-only memory - // that might be bad. - if (*type == 0) - *type = ftype; - } - - LOG("Found @ offset %zx size %zx\n", - rdev_relative_offset(cbfs, &fh->metadata), - region_device_sz(&fh->data)); - - /* Success. */ - return 0; - } - - LOG("'%s' not found.\n", name); - return -1; -} - -static int cbfs_extend_hash_buffer(struct vb2_digest_context *ctx, - void *buf, size_t sz) -{ - return vb2_digest_extend(ctx, buf, sz); -} - -static int cbfs_extend_hash(struct vb2_digest_context *ctx, - const struct region_device *rdev) -{ - uint8_t buffer[1024]; - size_t sz_left; - size_t offset; - - sz_left = region_device_sz(rdev); - offset = 0; - - while (sz_left) { - int rv; - size_t block_sz = MIN(sz_left, sizeof(buffer)); - - if (rdev_readat(rdev, buffer, offset, block_sz) != block_sz) - return VB2_ERROR_UNKNOWN; - - rv = cbfs_extend_hash_buffer(ctx, buffer, block_sz); - - if (rv) - return rv; - - sz_left -= block_sz; - offset += block_sz; - } - - return VB2_SUCCESS; -} - -/* Include offsets of child regions within the parent into the hash. */ -static int cbfs_extend_hash_with_offset(struct vb2_digest_context *ctx, - const struct region_device *p, - const struct region_device *c) -{ - int32_t soffset; - int rv; - - soffset = rdev_relative_offset(p, c); - - if (soffset < 0) - return VB2_ERROR_UNKNOWN; - - /* All offsets in big endian format. */ - write_be32(&soffset, soffset); - - rv = cbfs_extend_hash_buffer(ctx, &soffset, sizeof(soffset)); - - if (rv) - return rv; - - return cbfs_extend_hash(ctx, c); -} - -/* Hash in the potential CBFS header sitting at the beginning of the CBFS - * region as well as relative offset at the end. */ -static int cbfs_extend_hash_master_header(struct vb2_digest_context *ctx, - const struct region_device *cbfs) -{ - struct region_device rdev; - int rv; - - if (rdev_chain(&rdev, cbfs, 0, sizeof(struct cbfs_header))) - return VB2_ERROR_UNKNOWN; - - rv = cbfs_extend_hash_with_offset(ctx, cbfs, &rdev); - - if (rv) - return rv; - - /* Include potential relative offset at end of region. */ - if (rdev_chain(&rdev, cbfs, region_device_sz(cbfs) - sizeof(int32_t), - sizeof(int32_t))) - return VB2_ERROR_UNKNOWN; - - return cbfs_extend_hash_with_offset(ctx, cbfs, &rdev); -} - -int cbfs_vb2_hash_contents(const struct region_device *cbfs, - enum vb2_hash_algorithm hash_alg, void *digest, - size_t digest_sz) -{ - struct vb2_digest_context ctx; - int rv; - struct cbfsf f; - struct cbfsf *prev; - struct cbfsf *fh; - - rv = vb2_digest_init(&ctx, hash_alg); - - if (rv) - return rv; - - rv = cbfs_extend_hash_master_header(&ctx, cbfs); - if (rv) - return rv; - - prev = NULL; - fh = &f; - - while (1) { - uint32_t ftype; - - rv = cbfs_for_each_file(cbfs, prev, fh); - prev = fh; - - if (rv < 0) - return VB2_ERROR_UNKNOWN; - - /* End of CBFS. */ - if (rv > 0) - break; - - rv = cbfs_extend_hash_with_offset(&ctx, cbfs, &fh->metadata); - - if (rv) - return rv; - - /* Include data contents in hash if file is non-empty. */ - if (cbfsf_file_type(fh, &ftype)) - return VB2_ERROR_UNKNOWN; - - if (ftype == CBFS_TYPE_DELETED || ftype == CBFS_TYPE_NULL) - continue; - - rv = cbfs_extend_hash_with_offset(&ctx, cbfs, &fh->data); - - if (rv) - return rv; - } - - return vb2_digest_finalize(&ctx, digest, digest_sz); -} diff --git a/src/commonlib/include/commonlib/cbfs.h b/src/commonlib/include/commonlib/cbfs.h deleted file mode 100644 index 6565c1d..0000000 --- a/src/commonlib/include/commonlib/cbfs.h +++ /dev/null @@ -1,75 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#ifndef _COMMONLIB_CBFS_H_ -#define _COMMONLIB_CBFS_H_ - -#include <commonlib/bsd/cbfs_private.h> -#include <commonlib/region.h> -#include <vb2_api.h> - -/* Object representing cbfs files. */ -struct cbfsf { - struct region_device metadata; - struct region_device data; - union cbfs_mdata mdata; -}; - -/* Locate file by name and optional type. Returns 0 on success else < 0 on - * error.*/ -int cbfs_locate(struct cbfsf *fh, const struct region_device *cbfs, - const char *name, uint32_t *type); - -static inline void cbfs_file_data(struct region_device *data, - const struct cbfsf *file) -{ - rdev_chain_full(data, &file->data); -} - -static inline void cbfs_file_metadata(struct region_device *metadata, - const struct cbfsf *file) -{ - rdev_chain_full(metadata, &file->metadata); -} - -/* - * Provide a handle to each cbfs file within a cbfs. The prev pointer represents - * the previous file (NULL on first invocation). The next object gets filled - * out with the next file. This returns < 0 on error, 0 on finding the next - * file, and > 0 at end of cbfs. - */ -int cbfs_for_each_file(const struct region_device *cbfs, - const struct cbfsf *prev, struct cbfsf *fh); - -/* - * Return the offset for each CBFS attribute in a CBFS file metadata region. - * The metadata must already be fully mapped by the caller. Will return the - * offset (relative to the start of the metadata) or 0 when there are no - * further attributes. Should be called with 0 to begin, then always with - * the previously returned value until it returns 0. - */ -size_t cbfs_for_each_attr(void *metadata, size_t metadata_size, - size_t last_offset); - -/* - * Find out the decompression algorithm and decompressed size of a non-stage - * CBFS file (by parsing its metadata attributes), and return them with - * out-parameters. Returns 0 on success and < 0 on error. - */ -int cbfsf_decompression_info(struct cbfsf *fh, uint32_t *algo, size_t *size); - -/* - * Return the CBFS file type as out-parameter. - * Returns 0 on success and < 0 on error. - */ -int cbfsf_file_type(struct cbfsf *fh, uint32_t *ftype); - -/* - * Perform the vb2 hash over the CBFS region skipping empty file contents. - * Caller is responsible for providing the hash algorithm as well as storage - * for the final digest. Return 0 on success or non-zero on error. - */ -int cbfs_vb2_hash_contents(const struct region_device *cbfs, - enum vb2_hash_algorithm hash_alg, void *digest, - size_t digest_sz); - -#endif diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 6c79625..5731d6e 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -5,9 +5,9 @@ #include <cbmem.h> #include <commonlib/bsd/cbfs_mdata.h> -#include <commonlib/cbfs.h> #include <commonlib/mem_pool.h> #include <commonlib/region.h> +#include <endian.h> #include <program_loading.h> #include <types.h> #include <vb2_sha.h> @@ -190,16 +190,6 @@ /********************************************************************************************** - * LEGACY APIs, TO BE DEPRECATED/REPLACED * - **********************************************************************************************/ - -/* Locate file by name and optional type. Return 0 on success. < 0 on error. */ -int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type); -/* Locate file in a specific region of fmap. Return 0 on success. < 0 on error*/ -int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name, - const char *name, uint32_t *type); - -/********************************************************************************************** * INTERNAL HELPERS FOR INLINES, DO NOT USE. * **********************************************************************************************/ cb_err_t _cbfs_boot_lookup(const char *name, bool force_ro, diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 89101c1..dfb7a69 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -6,7 +6,6 @@ #include <cbmem.h> #include <commonlib/bsd/cbfs_private.h> #include <commonlib/bsd/compression.h> -#include <commonlib/endian.h> #include <console/console.h> #include <fmap.h> #include <lib.h> @@ -84,25 +83,6 @@ return CB_SUCCESS; } -int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type) -{ - if (_cbfs_boot_lookup(name, false, &fh->mdata, &fh->data)) - return -1; - - size_t msize = be32toh(fh->mdata.h.offset); - if (rdev_chain_mem(&fh->metadata, &fh->mdata, msize)) - return -1; - - if (type) { - if (!*type) - *type = be32toh(fh->mdata.h.type); - else if (*type != be32toh(fh->mdata.h.type)) - return -1; - } - - return 0; -} - void cbfs_unmap(void *mapping) { /* @@ -115,27 +95,6 @@ mem_pool_free(&cbfs_cache, mapping); } -int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name, - const char *name, uint32_t *type) -{ - struct region_device rdev; - int ret = 0; - if (fmap_locate_area_as_rdev(region_name, &rdev)) { - LOG("%s region not found while looking for %s\n", region_name, name); - return -1; - } - - uint32_t dummy_type = 0; - if (!type) - type = &dummy_type; - - ret = cbfs_locate(fh, &rdev, name, type); - if (!ret) - if (tspi_measure_cbfs_hook(&rdev, name, *type)) - LOG("error measuring %s in region %s\n", name, region_name); - return ret; -} - static inline bool fsps_env(void) { /* FSP-S is assumed to be loaded in ramstage. */ diff --git a/src/security/tpm/tspi/crtm.h b/src/security/tpm/tspi/crtm.h index 8ebb661..c4d051d 100644 --- a/src/security/tpm/tspi/crtm.h +++ b/src/security/tpm/tspi/crtm.h @@ -3,7 +3,6 @@ #ifndef __SECURITY_TSPI_CRTM_H__ #define __SECURITY_TSPI_CRTM_H__ -#include <commonlib/cbfs.h> #include <program_loading.h> #include <security/tpm/tspi.h> #include <types.h> diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 7cbeea3..312bcc7 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -251,7 +251,7 @@ default n depends on VBOOT_SLOTS_RW_A help - When this option is enabled cbfs_boot_locate will look for a file in the RO + When this option is enabled, CBFS will look for a file in the RO (COREBOOT) region if it isn't available in the active RW region. config VBOOT_EARLY_EC_SYNC -- To view, visit https://review.coreboot.org/c/coreboot/+/59682 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1cec0ca2d9d311626a087318d1d78163243bfc3c Gerrit-Change-Number: 59682 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: cbfs | tspi: Join hash calculation for verification and measurement
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Christian Walter. Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59681 ) Change subject: cbfs | tspi: Join hash calculation for verification and measurement ...................................................................... cbfs | tspi: Join hash calculation for verification and measurement This patch moves the CBFS file measurement when CONFIG_TPM_MEASURED_BOOT is enabled from the lookup step into the code where a file is actually loaded or mapped from flash. This has the advantage that CBFS routines which just look up a file to inspect its metadata (e.g. cbfs_get_size()) do not cause the file to be measured twice. It also removes the existing inefficiency that files are loaded twice when measurement is enabled (once to measure and then again when they are used). When CBFS verification is enabled and uses the same hash algorithm as the TPM, we are even able to only hash the file a single time and use the result for both purposes. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I70d7066c6768195077f083c7ffdfa30d9182b2b7 --- M src/lib/cbfs.c M src/security/tpm/tspi.h M src/security/tpm/tspi/crtm.c M src/security/tpm/tspi/crtm.h M src/security/tpm/tspi/tspi.c 5 files changed, 57 insertions(+), 95 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/59681/1 diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index d6a5862..89101c1 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -81,10 +81,6 @@ if (rdev_chain(rdev, &cbd->rdev, data_offset, be32toh(mdata->h.len))) return CB_ERR; - if (tspi_measure_cbfs_hook(rdev, name, be32toh(mdata->h.type))) { - printk(BIOS_ERR, "CBFS ERROR: error when measuring '%s'\n", name); - } - return CB_SUCCESS; } @@ -193,19 +189,36 @@ static bool cbfs_file_hash_mismatch(const void *buffer, size_t size, const union cbfs_mdata *mdata, bool skip_verification) { - /* Avoid linking hash functions when verification is disabled. */ - if (!CONFIG(CBFS_VERIFICATION) || skip_verification) + /* Avoid linking hash functions when verification and measurement are disabled. */ + if (!CONFIG(CBFS_VERIFICATION) && !CONFIG(TPM_MEASURED_BOOT)) return false; - const struct vb2_hash *hash = cbfs_file_hash(mdata); - if (!hash) { - ERROR("'%s' does not have a file hash!\n", mdata->h.filename); - return true; + const struct vb2_hash *hash = NULL; + + if (CONFIG(CBFS_VERIFICATION) && !skip_verification) { + hash = cbfs_file_hash(mdata); + if (!hash) { + ERROR("'%s' does not have a file hash!\n", mdata->h.filename); + return true; + } + if (vb2_hash_verify(buffer, size, hash) != VB2_SUCCESS) { + ERROR("'%s' file hash mismatch!\n", mdata->h.filename); + return true; + } } - if (vb2_hash_verify(buffer, size, hash) != VB2_SUCCESS) { - ERROR("'%s' file hash mismatch!\n", mdata->h.filename); - return true; + if (CONFIG(TPM_MEASURED_BOOT) && !ENV_SMM) { + struct vb2_hash calculated_hash; + + /* No need to re-hash file if we already have it from verification. */ + if (!hash || hash->algo != TPM_MEASURE_ALGO) { + vb2_hash_calculate(buffer, size, TPM_MEASURE_ALGO, &calculated_hash); + hash = &calculated_hash; + } + + if (tspi_cbfs_measurement(mdata->h.filename, be32toh(mdata->h.type), hash)) + ERROR("failed to measure '%s' into TCPA log\n", mdata->h.filename); + /* We intentionally continue to boot on measurement errors. */ } return false; @@ -528,10 +541,6 @@ if (rdev_chain(&file_rdev, &area_rdev, data_offset, be32toh(mdata.h.len))) return NULL; - if (tspi_measure_cbfs_hook(&file_rdev, name, be32toh(mdata.h.type))) { - ERROR("error measuring '%s' in '%s'\n", name, area); - } - return do_alloc(&mdata, &file_rdev, allocator, arg, size_out, true); } diff --git a/src/security/tpm/tspi.h b/src/security/tpm/tspi.h index ed642c3..7157b4d 100644 --- a/src/security/tpm/tspi.h +++ b/src/security/tpm/tspi.h @@ -51,7 +51,7 @@ * @return TPM_SUCCESS on success. If not a tpm error is returned */ uint32_t tpm_extend_pcr(int pcr, enum vb2_hash_algorithm digest_algo, - uint8_t *digest, size_t digest_len, + const uint8_t *digest, size_t digest_len, const char *name); /** diff --git a/src/security/tpm/tspi/crtm.c b/src/security/tpm/tspi/crtm.c index b64bbbf..8cd0793 100644 --- a/src/security/tpm/tspi/crtm.c +++ b/src/security/tpm/tspi/crtm.c @@ -6,37 +6,6 @@ #include "crtm.h" #include <string.h> -/* - * This function sets the TCPA log namespace - * for the cbfs file (region) lookup. - */ -static int create_tcpa_metadata(const struct region_device *rdev, - const char *cbfs_name, char log_string[TCPA_PCR_HASH_NAME]) -{ - int i; - struct region_device fmap; - static const char *const fmap_cbfs_names[] = { - "COREBOOT", - "FW_MAIN_A", - "FW_MAIN_B", - "RW_LEGACY" - }; - - for (i = 0; i < ARRAY_SIZE(fmap_cbfs_names); i++) { - if (fmap_locate_area_as_rdev(fmap_cbfs_names[i], &fmap) == 0) { - if (region_is_subregion(region_device_region(&fmap), - region_device_region(rdev))) { - snprintf(log_string, TCPA_PCR_HASH_NAME, - "FMAP: %s CBFS: %s", - fmap_cbfs_names[i], cbfs_name); - return 0; - } - } - } - - return -1; -} - static int tcpa_log_initialized; static inline int tcpa_log_available(void) { @@ -64,8 +33,6 @@ */ static uint32_t tspi_init_crtm(void) { - struct prog bootblock = PROG_INIT(PROG_BOOTBLOCK, "bootblock"); - /* Initialize TCPA PRERAM log. */ if (!tcpa_log_available()) { tcpa_preram_log_clear(); @@ -87,7 +54,6 @@ } /* measure bootblock from RO */ - struct cbfsf bootblock_data; struct region_device bootblock_fmap; if (fmap_locate_area_as_rdev("BOOTBLOCK", &bootblock_fmap) == 0) { if (tpm_measure_region(&bootblock_fmap, @@ -95,16 +61,16 @@ "FMAP: BOOTBLOCK")) return VB2_ERROR_UNKNOWN; } else { - if (cbfs_boot_locate(&bootblock_data, - prog_name(&bootblock), NULL)) { - /* - * measurement is done in - * tspi_measure_cbfs_hook() - */ + /* Mapping measures the file. We know we can safely map here because + bootblock-as-a-file is only used on x86, where we don't need cache to map. */ + enum cbfs_type type = CBFS_TYPE_BOOTBLOCK; + void *mapping = cbfs_ro_type_map("bootblock", NULL, &type); + if (!mapping) { printk(BIOS_INFO, "TSPI: Couldn't measure bootblock into CRTM!\n"); return VB2_ERROR_UNKNOWN; } + cbfs_unmap(mapping); } return VB2_SUCCESS; @@ -129,8 +95,7 @@ return !strcmp(allowlist, name); } -uint32_t tspi_measure_cbfs_hook(const struct region_device *rdev, const char *name, - uint32_t cbfs_type) +uint32_t tspi_cbfs_measurement(const char *name, uint32_t type, const struct vb2_hash *hash) { uint32_t pcr_index; char tcpa_metadata[TCPA_PCR_HASH_NAME]; @@ -144,7 +109,7 @@ printk(BIOS_DEBUG, "CRTM initialized.\n"); } - switch (cbfs_type) { + switch (type) { case CBFS_TYPE_MRC_CACHE: pcr_index = TPM_RUNTIME_DATA_PCR; break; @@ -166,10 +131,10 @@ break; } - if (create_tcpa_metadata(rdev, name, tcpa_metadata) < 0) - return VB2_ERROR_UNKNOWN; + snprintf(tcpa_metadata, TCPA_PCR_HASH_NAME, "CBFS: %s", name); - return tpm_measure_region(rdev, pcr_index, tcpa_metadata); + return tpm_extend_pcr(pcr_index, hash->algo, hash->raw, vb2_digest_size(hash->algo), + tcpa_metadata); } int tspi_measure_cache_to_pcr(void) diff --git a/src/security/tpm/tspi/crtm.h b/src/security/tpm/tspi/crtm.h index 011fa26..8ebb661 100644 --- a/src/security/tpm/tspi/crtm.h +++ b/src/security/tpm/tspi/crtm.h @@ -7,6 +7,7 @@ #include <program_loading.h> #include <security/tpm/tspi.h> #include <types.h> +#include <vb2_sha.h> /* CRTM */ #define TPM_CRTM_PCR 2 @@ -16,21 +17,16 @@ */ #define TPM_RUNTIME_DATA_PCR 3 +#define TPM_MEASURE_ALGO (CONFIG(TPM1) ? VB2_HASH_SHA1 : VB2_HASH_SHA256) + /** * Measure digests cached in TCPA log entries into PCRs */ int tspi_measure_cache_to_pcr(void); -#if !ENV_SMM && CONFIG(TPM_MEASURED_BOOT) -/* - * Measures cbfs data via hook (cbfs) - * rdev covers the file data (not metadata) - * return 0 if successful, else an error +/** + * Extend a measurement hash taken for a CBFS file into the appropriate PCR. */ -uint32_t tspi_measure_cbfs_hook(const struct region_device *rdev, - const char *name, uint32_t cbfs_type); -#else -#define tspi_measure_cbfs_hook(rdev, name, cbfs_type) 0 -#endif +uint32_t tspi_cbfs_measurement(const char *name, uint32_t type, const struct vb2_hash *hash); #endif /* __SECURITY_TSPI_CRTM_H__ */ diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c index b1bea41..e2d6d1e 100644 --- a/src/security/tpm/tspi/tspi.c +++ b/src/security/tpm/tspi/tspi.c @@ -220,7 +220,7 @@ } uint32_t tpm_extend_pcr(int pcr, enum vb2_hash_algorithm digest_algo, - uint8_t *digest, size_t digest_len, const char *name) + const uint8_t *digest, size_t digest_len, const char *name) { uint32_t result; @@ -234,16 +234,22 @@ return result; } - printk(BIOS_DEBUG, "TPM: Extending digest for %s into PCR %d\n", name, pcr); + printk(BIOS_DEBUG, "TPM: Extending digest for `%s` into PCR %d\n", name, pcr); result = tlcl_extend(pcr, digest, NULL); - if (result != TPM_SUCCESS) + if (result != TPM_SUCCESS) { + printk(BIOS_ERR, "TPM: Extending hash for `%s` into PCR %d failed.\n", + name, pcr); return result; + } } if (CONFIG(TPM_MEASURED_BOOT)) tcpa_log_add_table_entry(name, pcr, digest_algo, digest, digest_len); + printk(BIOS_DEBUG, "TPM: Digest of `%s` to PCR %d %s\n", + name, pcr, tspi_tpm_is_setup() ? "measured" : "logged"); + return TPM_SUCCESS; } @@ -253,23 +259,16 @@ { uint8_t digest[TPM_PCR_MAX_LEN], digest_len; uint8_t buf[HASH_DATA_CHUNK_SIZE]; - uint32_t result, offset; + uint32_t offset; size_t len; struct vb2_digest_context ctx; - enum vb2_hash_algorithm hash_alg; if (!rdev || !rname) return TPM_E_INVALID_ARG; - if (CONFIG(TPM1)) { - hash_alg = VB2_HASH_SHA1; - } else { /* CONFIG_TPM2 */ - hash_alg = VB2_HASH_SHA256; - } - - digest_len = vb2_digest_size(hash_alg); + digest_len = vb2_digest_size(TPM_MEASURE_ALGO); assert(digest_len <= sizeof(digest)); - if (vb2_digest_init(&ctx, hash_alg)) { + if (vb2_digest_init(&ctx, TPM_MEASURE_ALGO)) { printk(BIOS_ERR, "TPM: Error initializing hash.\n"); return TPM_E_HASH_ERROR; } @@ -294,13 +293,6 @@ printk(BIOS_ERR, "TPM: Error finalizing hash.\n"); return TPM_E_HASH_ERROR; } - result = tpm_extend_pcr(pcr, hash_alg, digest, digest_len, rname); - if (result != TPM_SUCCESS) { - printk(BIOS_ERR, "TPM: Extending hash into PCR failed.\n"); - return result; - } - printk(BIOS_DEBUG, "TPM: Digest of %s to PCR %d %s\n", - rname, pcr, tspi_tpm_is_setup() ? "measured" : "logged"); - return TPM_SUCCESS; + return tpm_extend_pcr(pcr, TPM_MEASURE_ALGO, digest, digest_len, rname); } #endif /* VBOOT_LIB */ -- To view, visit https://review.coreboot.org/c/coreboot/+/59681 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I70d7066c6768195077f083c7ffdfa30d9182b2b7 Gerrit-Change-Number: 59681 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: drivers/smmstore: Remove SMMSTORE_IN_CBFS
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59680 ) Change subject: drivers/smmstore: Remove SMMSTORE_IN_CBFS ...................................................................... drivers/smmstore: Remove SMMSTORE_IN_CBFS The SMMSTORE_IN_CBFS option was just meant as a workaround for an attempt to backport SMMSTORE into older Chromebooks that never actually happened. All current and future users of coreboot should be using SMMSTORE in an FMAP region. The APIs needed for SMMSTORE_IN_CBFS clash with the CBFS rdev isolation needed for CBFS_VERIFICATION, so let's just get rid of it. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: Ia0604a4ffd20b46774631d585925311b65d5a0e9 --- M src/drivers/smmstore/Kconfig M src/drivers/smmstore/store.c 2 files changed, 7 insertions(+), 38 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/59680/1 diff --git a/src/drivers/smmstore/Kconfig b/src/drivers/smmstore/Kconfig index 13b7312..3e20e3e 100644 --- a/src/drivers/smmstore/Kconfig +++ b/src/drivers/smmstore/Kconfig @@ -19,27 +19,10 @@ By using version 2 you cannot make use of software that expects a version 1 SMMSTORE. -config SMMSTORE_IN_CBFS - bool - default n - help - Select this if you want to add an SMMSTORE region to a - cbfsfile in a cbfs FMAP region - if SMMSTORE -config SMMSTORE_REGION - string "fmap region in which SMM store file is kept" if SMMSTORE_IN_CBFS - default "RW_LEGACY" if CHROMEOS && SMMSTORE_IN_CBFS - default "COREBOOT" if SMMSTORE_IN_CBFS - default "SMMSTORE" - -config SMMSTORE_FILENAME - string "SMM store file name" if SMMSTORE_IN_CBFS - default "smm_store" config SMMSTORE_SIZE hex "size of the SMMSTORE FMAP region" - depends on !SMMSTORE_IN_CBFS default 0x40000 help Sets the size of the default SMMSTORE FMAP region. diff --git a/src/drivers/smmstore/store.c b/src/drivers/smmstore/store.c index a12cd58..24e8a88 100644 --- a/src/drivers/smmstore/store.c +++ b/src/drivers/smmstore/store.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ #include <boot_device.h> -#include <cbfs.h> #include <fmap.h> #include <commonlib/helpers.h> #include <commonlib/region.h> @@ -9,6 +8,8 @@ #include <smmstore.h> #include <types.h> +#define SMMSTORE_REGION "SMMSTORE" + /* * The region format is still not finalized, but so far it looks like this: * ( @@ -33,26 +34,11 @@ static enum cb_err lookup_store_region(struct region *region) { - if (CONFIG(SMMSTORE_IN_CBFS)) { - struct cbfsf file; - if (cbfs_locate_file_in_region(&file, - CONFIG_SMMSTORE_REGION, - CONFIG_SMMSTORE_FILENAME, NULL) < 0) { - printk(BIOS_WARNING, - "smm store: Unable to find SMM store file in region '%s'\n", - CONFIG_SMMSTORE_REGION); - return CB_ERR; - } - struct region_device rdev; - cbfs_file_data(&rdev, &file); - *region = *region_device_region(&rdev); - } else { - if (fmap_locate_area(CONFIG_SMMSTORE_REGION, region)) { - printk(BIOS_WARNING, - "smm store: Unable to find SMM store FMAP region '%s'\n", - CONFIG_SMMSTORE_REGION); - return CB_ERR; - } + if (fmap_locate_area(SMMSTORE_REGION, region)) { + printk(BIOS_WARNING, + "smm store: Unable to find SMM store FMAP region '%s'\n", + SMMSTORE_REGION); + return CB_ERR; } return CB_SUCCESS; -- To view, visit https://review.coreboot.org/c/coreboot/+/59680 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia0604a4ffd20b46774631d585925311b65d5a0e9 Gerrit-Change-Number: 59680 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: intel: cse_lite: Use cbfs_unverified_area API
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Attention is currently required from: Patrick Rudolph. Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59679 ) Change subject: intel: cse_lite: Use cbfs_unverified_area API ...................................................................... intel: cse_lite: Use cbfs_unverified_area API This patch replaces the use of the deprecated cbfs_locate_file_in_region() API with the new cbfs_unverified_area_map(). Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: If4855280d6d06cf1aa646fded916fd830b287b30 --- M src/soc/intel/common/block/cse/cse_lite.c 1 file changed, 8 insertions(+), 30 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/59679/1 diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c index 281d381..bb64444 100644 --- a/src/soc/intel/common/block/cse/cse_lite.c +++ b/src/soc/intel/common/block/cse/cse_lite.c @@ -2,7 +2,6 @@ #include <console/console.h> #include <cbfs.h> -#include <commonlib/cbfs.h> #include <commonlib/region.h> #include <fmap.h> #include <intelblocks/cse.h> @@ -476,26 +475,6 @@ return CONFIG_SOC_INTEL_CSE_RW_B_FMAP_NAME; } -static bool cse_get_source_rdev(struct region_device *rdev) -{ - const char *reg_name; - uint32_t cbfs_type = CBFS_TYPE_RAW; - struct cbfsf fh; - - reg_name = cse_get_source_rdev_fmap(); - - if (reg_name == NULL) - return false; - - if (cbfs_locate_file_in_region(&fh, reg_name, CONFIG_SOC_INTEL_CSE_RW_CBFS_NAME, - &cbfs_type) < 0) - return false; - - cbfs_file_data(rdev, &fh); - - return true; -} - /* * Compare versions of CSE CBFS RW and CSE RW partition * If ver_cmp_status = 0, no update is required @@ -686,15 +665,16 @@ enum cse_update_status status, struct region_device *target_rdev) { - struct region_device source_rdev; enum csme_failure_reason rv; uint8_t *cbfs_rw_hash; + size_t size; - if (!cse_get_source_rdev(&source_rdev)) + const char *area_name = cse_get_source_rdev_fmap(); + if (!area_name) return CSE_LITE_SKU_RW_BLOB_NOT_FOUND; - void *cse_cbfs_rw = rdev_mmap_full(&source_rdev); - + void *cse_cbfs_rw = cbfs_unverified_area_map(area_name, + CONFIG_SOC_INTEL_CSE_RW_CBFS_NAME, &size); if (!cse_cbfs_rw) { printk(BIOS_ERR, "cse_lite: CSE CBFS RW blob could not be mapped\n"); return CSE_LITE_SKU_RW_BLOB_NOT_FOUND; @@ -708,8 +688,7 @@ goto error_exit; } - if (!cse_verify_cbfs_rw_sha256(cbfs_rw_hash, cse_cbfs_rw, - region_device_sz(&source_rdev))) { + if (!cse_verify_cbfs_rw_sha256(cbfs_rw_hash, cse_cbfs_rw, size)) { rv = CSE_LITE_SKU_RW_BLOB_SHA256_MISMATCH; goto error_exit; } @@ -719,12 +698,11 @@ goto error_exit; } - rv = cse_update_rw(cse_bp_info, cse_cbfs_rw, region_device_sz(&source_rdev), - target_rdev); + rv = cse_update_rw(cse_bp_info, cse_cbfs_rw, size, target_rdev); error_exit: cbfs_unmap(cbfs_rw_hash); - rdev_munmap(&source_rdev, cse_cbfs_rw); + cbfs_unmap(cse_cbfs_rw); return rv; } -- To view, visit https://review.coreboot.org/c/coreboot/+/59679 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If4855280d6d06cf1aa646fded916fd830b287b30 Gerrit-Change-Number: 59679 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: cbfs: Add unverified_area APIs
by Julius Werner (Code Review) 25 Nov '21

25 Nov '21

Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59678 ) Change subject: cbfs: Add unverified_area APIs ...................................................................... cbfs: Add unverified_area APIs This patch adds a new ..._unverified_area_... group of functions to the cbfs_map/_load/_alloc() APIs. These functions can be used to access custom FMAP sections and are meant to replace the existing cbfs_locate_file_in_region(). The name is intended to highlight that accesses through this API will not be verified when CBFS_VERIFICATION is enabled and should always be treated as if they may return malicious data. (Due to laziness I'm not adding the combination of this API with the ..._type_... variant at this point, since it seems very unlikely that we'll ever have a use case for that. If we ever do, it should be easy to add later.) (Also remove the 'inline' from cbfs_file_hash_mismatch(). I'm not sure why I put it there in the first place, probably a bad copy&paste.) Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I402265900f7075aa0c2f58d812c67ea63ddf2900 --- M src/include/cbfs.h M src/lib/cbfs.c 2 files changed, 152 insertions(+), 78 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/59678/1 diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 43d8123..6c79625 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -55,6 +55,11 @@ * section), even when running in an RW stage from one of the RW CBFSs. Only relevant if * CONFIG(VBOOT) is set. * + * ..._unverified_area_...: Will look for the CBFS file in the named FMAP area, rather than + * any of the default (RO or RW) CBFSs. Files accessed this way are *not* verified in any + * way (even if CONFIG(CBFS_VERIFICATION) is enabled) and should always be treated as + * untrusted (potentially malicious) data. Mutually exclusive with the ..._ro_... variant. + * * ..._type_...: May pass in an extra enum cbfs_type *type parameter. If the value it points to * is CBFS_TYPE_QUERY, it will be replaced with the actual CBFS type of the found file. If * it is anything else, the type will be compared with the actually found type, and the @@ -76,11 +81,15 @@ enum cbfs_type *type); static inline size_t cbfs_ro_type_load(const char *name, void *buf, size_t size, enum cbfs_type *type); +static inline size_t cbfs_unverified_area_load(const char *area, const char *name, + void *buf, size_t size); static inline void *cbfs_map(const char *name, size_t *size_out); static inline void *cbfs_ro_map(const char *name, size_t *size_out); static inline void *cbfs_type_map(const char *name, size_t *size_out, enum cbfs_type *type); static inline void *cbfs_ro_type_map(const char *name, size_t *size_out, enum cbfs_type *type); +static inline void *cbfs_unverified_area_map(const char *area, const char *name, + size_t *size_out); static inline void *cbfs_alloc(const char *name, cbfs_allocator_t allocator, void *arg, size_t *size_out); @@ -90,6 +99,9 @@ size_t *size_out, enum cbfs_type *type); static inline void *cbfs_ro_type_alloc(const char *name, cbfs_allocator_t allocator, void *arg, size_t *size_out, enum cbfs_type *type); +static inline void *cbfs_unverified_area_alloc(const char *area, const char *name, + cbfs_allocator_t allocator, void *arg, + size_t *size_out); static inline void *cbfs_cbmem_alloc(const char *name, uint32_t cbmem_id, size_t *size_out); static inline void *cbfs_ro_cbmem_alloc(const char *name, uint32_t cbmem_id, size_t *size_out); @@ -97,6 +109,8 @@ enum cbfs_type *type); static inline void *cbfs_ro_type_cbmem_alloc(const char *name, uint32_t cbmem_id, size_t *size_out, enum cbfs_type *type); +static inline void *cbfs_unverified_area_cbmem_alloc(const char *area, const char *name, + uint32_t cbmem_id, size_t *size_out); /* * Starts the processes of preloading a file into RAM. @@ -194,6 +208,9 @@ void *_cbfs_alloc(const char *name, cbfs_allocator_t allocator, void *arg, size_t *size_out, bool force_ro, enum cbfs_type *type); +void *_cbfs_unverified_area_alloc(const char *area, const char *name, + cbfs_allocator_t allocator, void *arg, size_t *size_out); + struct _cbfs_default_allocator_arg { void *buf; size_t buf_size; @@ -229,6 +246,13 @@ return _cbfs_alloc(name, allocator, arg, size_out, true, type); } +static inline void *cbfs_unverified_area_alloc(const char *area, const char *name, + cbfs_allocator_t allocator, void *arg, + size_t *size_out) +{ + return _cbfs_unverified_area_alloc(area, name, allocator, arg, size_out); +} + static inline void *cbfs_map(const char *name, size_t *size_out) { return cbfs_type_map(name, size_out, NULL); @@ -249,6 +273,12 @@ return cbfs_ro_type_alloc(name, NULL, NULL, size_out, type); } +static inline void *cbfs_unverified_area_map(const char *area, const char *name, + size_t *size_out) +{ + return _cbfs_unverified_area_alloc(area, name, NULL, NULL, size_out); +} + static inline size_t _cbfs_load(const char *name, void *buf, size_t size, bool force_ro, enum cbfs_type *type) { @@ -281,6 +311,16 @@ return _cbfs_load(name, buf, size, true, type); } +static inline size_t cbfs_unverified_area_load(const char *area, const char *name, + void *buf, size_t size) +{ + struct _cbfs_default_allocator_arg arg = { .buf = buf, .buf_size = size }; + if (_cbfs_unverified_area_alloc(area, name, _cbfs_default_allocator, &arg, &size)) + return size; + else + return 0; +} + static inline void *cbfs_cbmem_alloc(const char *name, uint32_t cbmem_id, size_t *size_out) { return cbfs_type_cbmem_alloc(name, cbmem_id, size_out, NULL); @@ -305,6 +345,13 @@ size_out, type); } +static inline void *cbfs_unverified_area_cbmem_alloc(const char *area, const char *name, + uint32_t cbmem_id, size_t *size_out) +{ + return _cbfs_unverified_area_alloc(area, name, _cbfs_cbmem_allocator, + (void *)(uintptr_t)cbmem_id, size_out); +} + static inline size_t cbfs_get_size(const char *name) { union cbfs_mdata mdata; diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 1ffc695..d6a5862 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -190,30 +190,37 @@ return true; } -static inline bool cbfs_file_hash_mismatch(const void *buffer, size_t size, - const struct vb2_hash *file_hash) +static bool cbfs_file_hash_mismatch(const void *buffer, size_t size, + const union cbfs_mdata *mdata, bool skip_verification) { /* Avoid linking hash functions when verification is disabled. */ - if (!CONFIG(CBFS_VERIFICATION)) + if (!CONFIG(CBFS_VERIFICATION) || skip_verification) return false; - /* If there is no file hash, always count that as a mismatch. */ - if (file_hash && vb2_hash_verify(buffer, size, file_hash) == VB2_SUCCESS) - return false; + const struct vb2_hash *hash = cbfs_file_hash(mdata); + if (!hash) { + ERROR("'%s' does not have a file hash!\n", mdata->h.filename); + return true; + } - printk(BIOS_CRIT, "CBFS file hash mismatch!\n"); - return true; + if (vb2_hash_verify(buffer, size, hash) != VB2_SUCCESS) { + ERROR("'%s' file hash mismatch!\n", mdata->h.filename); + return true; + } + + return false; } static size_t cbfs_load_and_decompress(const struct region_device *rdev, void *buffer, size_t buffer_size, uint32_t compression, - const struct vb2_hash *file_hash) + const union cbfs_mdata *mdata, bool skip_verification) { size_t in_size = region_device_sz(rdev); size_t out_size = 0; void *map; - DEBUG("Decompressing %zu bytes to %p with algo %d\n", in_size, buffer, compression); + DEBUG("Decompressing %zu bytes from '%s' to %p with algo %d\n", + in_size, mdata->h.filename, buffer, compression); switch (compression) { case CBFS_COMPRESS_NONE: @@ -221,7 +228,7 @@ return 0; if (rdev_readat(rdev, buffer, 0, in_size) != in_size) return 0; - if (cbfs_file_hash_mismatch(buffer, in_size, file_hash)) + if (cbfs_file_hash_mismatch(buffer, in_size, mdata, skip_verification)) return 0; return in_size; @@ -235,7 +242,7 @@ if (map == NULL) return 0; - if (!cbfs_file_hash_mismatch(map, in_size, file_hash)) { + if (!cbfs_file_hash_mismatch(map, in_size, mdata, skip_verification)) { timestamp_add_now(TS_START_ULZ4F); out_size = ulz4fn(map, in_size, buffer, buffer_size); timestamp_add_now(TS_END_ULZ4F); @@ -252,7 +259,7 @@ if (map == NULL) return 0; - if (!cbfs_file_hash_mismatch(map, in_size, file_hash)) { + if (!cbfs_file_hash_mismatch(map, in_size, mdata, skip_verification)) { /* Note: timestamp not useful for memory-mapped media (x86) */ timestamp_add_now(TS_START_ULZMA); out_size = ulzman(map, in_size, buffer, buffer_size); @@ -411,13 +418,64 @@ return err; } +static void *do_alloc(union cbfs_mdata *mdata, struct region_device *rdev, + cbfs_allocator_t allocator, void *arg, size_t *size_out, + bool skip_verification) +{ + size_t size = region_device_sz(rdev); + void *loc = NULL; + + uint32_t compression = CBFS_COMPRESS_NONE; + const struct cbfs_file_attr_compression *cattr = cbfs_find_attr(mdata, + CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*cattr)); + if (cattr) { + compression = be32toh(cattr->compression); + size = be32toh(cattr->decompressed_size); + } + + if (size_out) + *size_out = size; + + /* allocator == NULL means do a cbfs_map() */ + if (allocator) { + loc = allocator(arg, size, mdata); + } else if (compression == CBFS_COMPRESS_NONE) { + void *mapping = rdev_mmap_full(rdev); + if (!mapping) + return NULL; + if (cbfs_file_hash_mismatch(mapping, size, mdata, skip_verification)) { + rdev_munmap(rdev, mapping); + return NULL; + } + return mapping; + } else if (!cbfs_cache.size) { + /* In order to use the cbfs_cache you need to add a CBFS_CACHE to your + * memlayout. For stages that don't have .data sections (x86 pre-RAM), + * it is not possible to add a CBFS_CACHE. */ + ERROR("Cannot map compressed file %s without cbfs_cache\n", mdata->h.filename); + return NULL; + } else { + loc = mem_pool_alloc(&cbfs_cache, size); + } + + if (!loc) { + ERROR("'%s' allocation failure\n", mdata->h.filename); + return NULL; + } + + size = cbfs_load_and_decompress(rdev, loc, size, compression, mdata, skip_verification); + if (!size) + return NULL; + + return loc; +} + void *_cbfs_alloc(const char *name, cbfs_allocator_t allocator, void *arg, size_t *size_out, bool force_ro, enum cbfs_type *type) { struct region_device rdev; bool preload_successful = false; union cbfs_mdata mdata; - void *loc = NULL; DEBUG("%s(name='%s', alloc=%p(%p), force_ro=%s, type=%d)\n", __func__, name, allocator, arg, force_ro ? "true" : "false", type ? *type : -1); @@ -436,72 +494,45 @@ } } - size_t size = region_device_sz(&rdev); - uint32_t compression = CBFS_COMPRESS_NONE; - const struct cbfs_file_attr_compression *cattr = cbfs_find_attr(&mdata, - CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*cattr)); - if (cattr) { - compression = be32toh(cattr->compression); - size = be32toh(cattr->decompressed_size); - } - - if (size_out) - *size_out = size; - - const struct vb2_hash *file_hash = NULL; - if (CONFIG(CBFS_VERIFICATION)) - file_hash = cbfs_file_hash(&mdata); - /* Update the rdev with the preload content */ if (!force_ro && get_preload_rdev(&rdev, name) == CB_SUCCESS) preload_successful = true; - /* allocator == NULL means do a cbfs_map() */ - if (allocator) { - loc = allocator(arg, size, &mdata); - } else if (compression == CBFS_COMPRESS_NONE) { - void *mapping = rdev_mmap_full(&rdev); + void *ret = do_alloc(&mdata, &rdev, allocator, arg, size_out, false); - if (!mapping) - goto out; - - if (cbfs_file_hash_mismatch(mapping, size, file_hash)) { - rdev_munmap(&rdev, mapping); - goto out; - } - - return mapping; - } else if (!cbfs_cache.size) { - /* - * In order to use the cbfs_cache you need to add a CBFS_CACHE to your - * memlayout. For stages that don't have .data sections (x86 pre-RAM), - * it is not possible to add a CBFS_CACHE. - */ - ERROR("Cannot map compressed file %s without cbfs_cache\n", mdata.h.filename); - goto out; - } else { - loc = mem_pool_alloc(&cbfs_cache, size); - } - - if (!loc) { - ERROR("'%s' allocation failure\n", mdata.h.filename); - goto out; - } - - size = cbfs_load_and_decompress(&rdev, loc, size, compression, file_hash); - - if (!size) - loc = NULL; - -out: - /* - * When using cbfs_preload we need to free the preload buffer after populating the - * destination buffer. - */ + /* When using cbfs_preload we need to free the preload buffer after populating the + * destination buffer. We know we must have a mem_rdev here, so extra mmap is fine. */ if (preload_successful) cbfs_unmap(rdev_mmap_full(&rdev)); - return loc; + return ret; +} + +void *_cbfs_unverified_area_alloc(const char *area, const char *name, + cbfs_allocator_t allocator, void *arg, size_t *size_out) +{ + struct region_device area_rdev, file_rdev; + union cbfs_mdata mdata; + size_t data_offset; + + DEBUG("%s(area='%s', name='%s', alloc=%p(%p))\n", __func__, area, name, allocator, arg); + + if (fmap_locate_area_as_rdev(area, &area_rdev)) + return NULL; + + if (cbfs_lookup(&area_rdev, name, &mdata, &data_offset, NULL)) { + ERROR("'%s' not found in '%s'\n", name, area); + return NULL; + } + + if (rdev_chain(&file_rdev, &area_rdev, data_offset, be32toh(mdata.h.len))) + return NULL; + + if (tspi_measure_cbfs_hook(&file_rdev, name, be32toh(mdata.h.type))) { + ERROR("error measuring '%s' in '%s'\n", name, area); + } + + return do_alloc(&mdata, &file_rdev, allocator, arg, size_out, true); } void *_cbfs_default_allocator(void *arg, size_t size, const union cbfs_mdata *unused) @@ -546,17 +577,13 @@ prog_set_entry(pstage, prog_start(pstage) + be32toh(sattr->entry_offset), NULL); - const struct vb2_hash *file_hash = NULL; - if (CONFIG(CBFS_VERIFICATION)) - file_hash = cbfs_file_hash(&mdata); - /* Hacky way to not load programs over read only media. The stages * 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_full(&rdev); rdev_munmap(&rdev, mapping); - if (cbfs_file_hash_mismatch(mapping, region_device_sz(&rdev), file_hash)) + if (cbfs_file_hash_mismatch(mapping, region_device_sz(&rdev), &mdata, false)) return CB_CBFS_HASH_MISMATCH; if (mapping == prog_start(pstage)) return CB_SUCCESS; @@ -573,7 +600,7 @@ } size_t fsize = cbfs_load_and_decompress(&rdev, prog_start(pstage), prog_size(pstage), - compression, file_hash); + compression, &mdata, false); if (!fsize) return CB_ERR; -- To view, visit https://review.coreboot.org/c/coreboot/+/59678 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I402265900f7075aa0c2f58d812c67ea63ddf2900 Gerrit-Change-Number: 59678 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021