coreboot-gerrit April 2021

coreboot-gerrit@coreboot.org

1 participants
6268 discussions

Change in coreboot[master]: mb/prodrive/hermes: Drop Vref configuration for older boards
by Justin van Son (Code Review) 03 Apr '21

03 Apr '21

Attention is currently required from: Patrick Rudolph, Christian Walter, Angel Pons. Justin van Son has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51919 ) Change subject: mb/prodrive/hermes: Drop Vref configuration for older boards ...................................................................... Patch Set 1: (2 comments) File src/mainboard/prodrive/hermes/variants/r04/hda_verb.c: https://review.coreboot.org/c/coreboot/+/51919/comment/62d36b66_d67bca7a PS1, Line 42: return AZALIA_PIN_DESC( > Should this use a separate association? Maybe 0x4 instead of 0xf? […] with an association of 0x4, it would be 0x90100140. The third to last nibble is also used by the ALC888 for VREF output, Retaskable, Sensing support, jack detect override in that order. Here just setting jack detect override should be enough, though. https://review.coreboot.org/c/coreboot/+/51919/comment/cf3d8ea8_5b1f1fca PS1, Line 69: 0x018707e1, /* port B VREF 50% of LDO-OUT */ > will drop them here. Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/51919 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I41c207f97dad6c9107c1999eb46d2d6304a6c217 Gerrit-Change-Number: 51919 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Justin van Son <justin.van.son(a)prodrive-technologies.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 03 Apr 2021 08:41:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Justin van Son <justin.van.son(a)prodrive-technologies.com> Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya: Move early GPIO config earlier
by EricR Lai (Code Review) 03 Apr '21

03 Apr '21

Attention is currently required from: Tim Wawrzynczak. EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52062 ) Change subject: mb/google/brya: Move early GPIO config earlier ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/52062 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4a4fadcc091bf9b1c9894f9afaf42baff63c73a3 Gerrit-Change-Number: 52062 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Comment-Date: Sat, 03 Apr 2021 03:47:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya: Enable south XHCI ports 1 and 2
by EricR Lai (Code Review) 03 Apr '21

03 Apr '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Ronak Kanabar. EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52088 ) Change subject: mb/google/brya: Enable south XHCI ports 1 and 2 ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/52088 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b86a98b18234ba309ddf2f30b80d78472951637 Gerrit-Change-Number: 52088 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Sat, 03 Apr 2021 03:46:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya: Enable south XHCI ports 1 and 2
by Furquan Shaikh (Code Review) 03 Apr '21

03 Apr '21

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/52088 ) Change subject: mb/google/brya: Enable south XHCI ports 1 and 2 ...................................................................... mb/google/brya: Enable south XHCI ports 1 and 2 FSP v2081 has a bug where it uses the information about south XHCI ports to enable TCSS XHCI ports. This change works around this bug by enabling south XHCI ports 1 and 2 in brya baseboard devicetree. brya0 already enables south XHCI port 1 in overridetree.cb, however, it is still enabled in baseboard/devicetree in case more variants are added to brya before FSP is fixed. BUG=b:184324979 TEST=Verified that TCSS XHCI ports 1 and 2 are now enabled. Change-Id: I4b86a98b18234ba309ddf2f30b80d78472951637 Signed-off-by: Furquan Shaikh <furquan(a)google.com> --- M src/mainboard/google/brya/variants/baseboard/devicetree.cb 1 file changed, 6 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/52088/1 diff --git a/src/mainboard/google/brya/variants/baseboard/devicetree.cb b/src/mainboard/google/brya/variants/baseboard/devicetree.cb index 3155d04..d7e2522 100644 --- a/src/mainboard/google/brya/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/brya/variants/baseboard/devicetree.cb @@ -39,6 +39,12 @@ register "usb2_ports[9]" = "USB2_PORT_MID(OC_SKIP)" # M.2 Bluetooth register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC3)" # USB3/2 Type A port A0 + # TODO(b/184324979): Workaround to enable TCSS ports. FSP v2081 + # uses port enable for south XHCI ports to determine if TCSS + # ports should be enabled. Until FSP is fixed, enable south + # XHCI ports 1 and 2. + register "usb3_ports[1]" = "USB3_PORT_DEFAULT(OC_SKIP)" + register "usb3_ports[2]" = "USB3_PORT_DEFAULT(OC_SKIP)" register "usb3_ports[3]" = "USB3_PORT_DEFAULT(OC_SKIP)" # M.2 WWAN register "SerialIoI2cMode" = "{ -- To view, visit https://review.coreboot.org/c/coreboot/+/52088 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4b86a98b18234ba309ddf2f30b80d78472951637 Gerrit-Change-Number: 52088 Gerrit-PatchSet: 1 Gerrit-Owner: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: newchange

1 0

Change in em100[master]: em100: Make trace mode output more resilient
by Stefan Reinauer (Code Review) 02 Apr '21

02 Apr '21

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/48866 ) Change subject: em100: Make trace mode output more resilient ...................................................................... em100: Make trace mode output more resilient When redirecting em100 output to a file, say with | tee <filename>, we won't see any output from the tool until actual SPI commands are coming in. This is annoying, as you don't know when the download is finished, if your workflow is for example as follows: 1. start em100 in trace mode 2. wait for download to em100 to finish 3. power-cycle / turn on the machine Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Change-Id: I36916ab71d3b929deb4d8cfed7b22a2ce90b2672 --- M em100.c 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/em100 refs/changes/66/48866/1 diff --git a/em100.c b/em100.c index d7afc34..0317565 100644 --- a/em100.c +++ b/em100.c @@ -1260,7 +1260,8 @@ printf("terminal"); } - printf(". Press CTL-C to exit.\n\n"); + printf(". Press CTRL-C to exit.\n\n"); + fflush(stdout); while (!exit_requested && usb_errors < MAX_USB_ERRORS) { int ret; -- To view, visit https://review.coreboot.org/c/em100/+/48866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: master Gerrit-Change-Id: I36916ab71d3b929deb4d8cfed7b22a2ce90b2672 Gerrit-Change-Number: 48866 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newchange

3 3

Change in em100[master]: Add a brief mode for trace output
by Stefan Reinauer (Code Review) 02 Apr '21

02 Apr '21

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/48728 ) Change subject: Add a brief mode for trace output ...................................................................... Add a brief mode for trace output The log with -b|--brief will look like this: 0x5a @ 0x00000000 (read SFDP) 0x5a @ 0x00000008 (read SFDP) 0x5a @ 0x00000030 (read SFDP) 0xb7 (enter 4b mode) 0x03 @ 0x00000010 (read) 0x03 @ 0x00000014 (read) 0x03 @ 0x00000030 (read) 0x03 @ 0x00000040 (read) Instead of this: Time: 000000.00000000 command # 1 : 0x5a - read SFDP Table 00000000 : 53 46 44 50 00 01 01 ff Time: 000000.00000575 command # 2 : 0x5a - read SFDP Table 00000008 : 00 00 01 09 30 00 00 ff Time: 000000.00001149 command # 3 : 0x5a - read SFDP Table 00000030 : e5 20 f3 ff ff ff ff 0f 44 eb 08 6b 08 3b 04 bb Time: 000000.00002053 command # 4 : 0xb7 - enter 4-byte address mode Time: 000000.00002687 command # 5 : 0x03 - read 00000000 : 10 5a a5 f0 0f Time: 000000.00003088 command # 6 : 0x03 - read 00000000 : 14 03 00 04 05 06 03 10 15 20 01 21 26 Time: 000000.00003825 command # 7 : 0x03 - read 00000000 : 30 f5 00 00 00 00 00 00 00 00 00 00 00 Time: 000000.00004569 command # 8 : 0x03 - read 00000000 : 40 00 00 00 00 00 08 ff 0f 23 00 ff 07 01 00 02 00000010 : 00 03 00 12 00 13 00 22 00 ff 7f 00 00 Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Change-Id: Icdaa3444bba03be8bf122239fd6da805cf840e52 --- M em100.c M em100.h M trace.c 3 files changed, 53 insertions(+), 27 deletions(-) git pull ssh://review.coreboot.org:29418/em100 refs/changes/28/48728/1 diff --git a/em100.c b/em100.c index 67ac43c..7a6f910 100644 --- a/em100.c +++ b/em100.c @@ -790,6 +790,7 @@ {"terminal", 0, 0, 'T'}, {"traceconsole", 0, 0, 'R'}, {"length", 1, 0, 'L'}, + {"brief", 0, 0, 'b'}, {"compatible", 0, 0, 'C'}, {NULL, 0, 0, 0} }; @@ -812,6 +813,7 @@ " -T|--terminal: terminal mode\n" " -R|--traceconsole: trace console mode\n" " -L|--length HEX_VAL: length of buffer for traceconsole mode\n" + " -b|--brief: brief mode for traces\n" " -F|--firmware-update FILE|auto: update EM100pro firmware (dangerous)\n" " -f|--firmware-dump FILE: export raw EM100pro firmware to file\n" " -g|--firmware-write FILE: export EM100pro firmware to DPFW file\n" @@ -848,7 +850,7 @@ struct sigaction signal_action; struct em100 *em100 = &em100_state; - while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhTRL:", + while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhTRL:b", longopts, &idx)) != -1) { switch (opt) { case 'c': @@ -930,6 +932,9 @@ case 'L': sscanf(optarg, "%lx", &address_length); break; + case 'b': + trace_brief = 1; + break; case 'C': compatibility = 1; break; diff --git a/em100.h b/em100.h index bf1bb87..4ba78a9 100644 --- a/em100.h +++ b/em100.h @@ -126,6 +126,7 @@ int set_led(struct em100 *em100, led_state_t led_state); /* trace.c */ +extern int trace_brief; #define EM100_SPECIFIC_CMD 0x11 #define EM100_MSG_SIGNATURE 0x47364440 diff --git a/trace.c b/trace.c index df3fdd9..c04afcc 100644 --- a/trace.c +++ b/trace.c @@ -15,6 +15,8 @@ #include <string.h> #include "em100.h" +int trace_brief = 0; + /* SPI Trace related operations */ /** @@ -282,44 +284,62 @@ j = MAX_TRACE_BLOCKLENGTH; } - printf("\nTime: %06lld.%08lld", + if (trace_brief) { + if (start_timestamp) + start_timestamp = 0; + + if (spi_cmd_vals->address_type != ADDR_NONE) + printf("0x%02x @ 0x%08lx (%s)\n", + spi_command, address, spi_cmd_vals->cmd_name); + else + printf("0x%02x (%s)\n", + spi_command, spi_cmd_vals->cmd_name); + } else { + printf("\nTime: %06lld.%08lld", (timestamp - start_timestamp) / 100000000, (timestamp - start_timestamp) % 100000000); - printf(" command # %-6d : 0x%02x - %s", + printf(" command # %-6d : 0x%02x - %s", ++counter, spi_command, spi_cmd_vals->cmd_name); + } + curpos = 0; outbytes = 0; } - /* this exploits 8bit wrap around in curpos */ - unsigned char blocklen = (data[2 + i*8 + 1] - curpos); - blocklen /= 8; + if (trace_brief) { + if (outbytes) + outbytes++; + } else { + /* this exploits 8bit wrap around in curpos */ + unsigned char blocklen = (data[2 + i*8 + 1] - curpos); + blocklen /= 8; - for (; j < blocklen; j++) { - if (outbytes == 0) { - switch (spi_cmd_vals->address_type) { - case ADDR_DYNAMIC: - case ADDR_3B: - case ADDR_4B: - printf("\n%08lx : ", - addr_offset + address); - break; - case ADDR_NO_OFF_3B: - printf("\n%08lx : ", address); - break; - case ADDR_NONE: - printf("\n : "); - break; + for (; j < blocklen; j++) { + if (outbytes == 0) { + switch (spi_cmd_vals->address_type) { + case ADDR_DYNAMIC: + case ADDR_3B: + case ADDR_4B: + printf("\n%08lx : ", + addr_offset + address); + break; + case ADDR_NO_OFF_3B: + printf("\n%08lx : ", address); + break; + case ADDR_NONE: + printf("\n : "); + break; + } } - } - printf("%02x ", data[i * 8 + 4 + j]); - outbytes++; - if (outbytes == 16) { - outbytes = 0; - address += 16; + printf("%02x ", data[i * 8 + 4 + j]); + outbytes++; + if (outbytes == 16) { + outbytes = 0; + address += 16; + } } } // this is because the em100 counts funny -- To view, visit https://review.coreboot.org/c/em100/+/48728 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: master Gerrit-Change-Id: Icdaa3444bba03be8bf122239fd6da805cf840e52 Gerrit-Change-Number: 48728 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newchange

3 5

Change in coreboot[master]: mem_pool: Track the last two allocations (not just one)
by Julius Werner (Code Review) 02 Apr '21

02 Apr '21

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/+/52087 to review the following change. Change subject: mem_pool: Track the last two allocations (not just one) ...................................................................... mem_pool: Track the last two allocations (not just one) This patch changes the mem_pool implementation to track the last two allocations (instead of just the last) and allow them both to be freed if the mem_pool_free() calls come in in reverse order. This is intended as a specific optimization for the CBFS cache case when a compressed file is mapped on a platform that doesn't natively support memory-mapping flash. In this case, cbfs_map() (chaining through to _cbfs_alloc() with allocator == NULL) will call mem_pool_alloc(&cbfs_cache) to allocate space for the uncompressed file data. It will then call cbfs_load_and_decompress() to fill that allocation, which will notice the compression and in turn call rdev_mmap_full() to map the compressed data (which on platforms without memory-mapped flash usually results in a second call to mem_pool_alloc(&cbfs_cache)). It then runs the decompression algorithm and calls rdev_munmap() on the compressed data buffer (the latter one in the allocation sequence), leading to a mem_pool_free(). The remaining buffer with the uncompressed data is returned out of cbfs_map() to the caller, which should eventually call cbfs_unmap() to mem_pool_free() that as well. This patch allows this simple case to succeed without leaking any permanent allocations on the cache. (More complicated cases where the caller maps other files before cbfs_unmap()ing the first one may still lead to leaks, but those are very rare in practice.) Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: Ic5c4c56a8482752ed65e10cf35565f9b2d3e4b17 --- M src/commonlib/include/commonlib/mem_pool.h M src/commonlib/mem_pool.c 2 files changed, 20 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/52087/1 diff --git a/src/commonlib/include/commonlib/mem_pool.h b/src/commonlib/include/commonlib/mem_pool.h index bde9e41..6c85397 100644 --- a/src/commonlib/include/commonlib/mem_pool.h +++ b/src/commonlib/include/commonlib/mem_pool.h @@ -7,11 +7,14 @@ #include <stdint.h> /* - * The memory pool allows one to allocate memory from a fixed size buffer - * that also allows freeing semantics for reuse. However, the current - * limitation is that the most recent allocation is the only one that - * can be freed. If one tries to free any allocation that isn't the - * most recently allocated it will result in a leak within the memory pool. + * The memory pool allows one to allocate memory from a fixed size buffer that + * also allows freeing semantics for reuse. However, the current limitation is + * that only the two most recent allocations can be freed (in exact reverse + * order). If one tries to free any allocation that isn't at the top of the + * allocation stack, or one allocates more than two buffers in a row without + * freeing, it will result in a leak within the memory pool. (Two allocations + * were chosen to optimize for the CBFS cache case which may need two buffers + * to map a single compressed file, and will free them in reverse order.) * * The memory returned by allocations are at least 8 byte aligned. Note * that this requires the backing buffer to start on at least an 8 byte @@ -22,20 +25,23 @@ uint8_t *buf; size_t size; uint8_t *last_alloc; + uint8_t *second_to_last_alloc; size_t free_offset; }; -#define MEM_POOL_INIT(buf_, size_) \ - { \ - .buf = (buf_), \ - .size = (size_), \ - .last_alloc = NULL, \ - .free_offset = 0, \ +#define MEM_POOL_INIT(buf_, size_) \ + { \ + .buf = (buf_), \ + .size = (size_), \ + .last_alloc = NULL, \ + .second_to_last_alloc = NULL, \ + .free_offset = 0, \ } static inline void mem_pool_reset(struct mem_pool *mp) { mp->last_alloc = NULL; + mp->second_to_last_alloc = NULL; mp->free_offset = 0; } diff --git a/src/commonlib/mem_pool.c b/src/commonlib/mem_pool.c index 2ad1099..c300c65 100644 --- a/src/commonlib/mem_pool.c +++ b/src/commonlib/mem_pool.c @@ -17,6 +17,7 @@ p = &mp->buf[mp->free_offset]; mp->free_offset += sz; + mp->second_to_last_alloc = mp->last_alloc; mp->last_alloc = p; return p; @@ -29,6 +30,7 @@ return; mp->free_offset = mp->last_alloc - mp->buf; + mp->last_alloc = mp->second_to_last_alloc; /* No way to track allocation before this one. */ - mp->last_alloc = NULL; + mp->second_to_last_alloc = NULL; } -- To view, visit https://review.coreboot.org/c/coreboot/+/52087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic5c4c56a8482752ed65e10cf35565f9b2d3e4b17 Gerrit-Change-Number: 52087 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: verstage: Add debug print when returning from verstage
by Julius Werner (Code Review) 02 Apr '21

02 Apr '21

1 0

Change in coreboot[master]: decompressor: Add CBFS_VERIFICATION support
by Julius Werner (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Martin Roth, 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/+/52085 to review the following change. Change subject: decompressor: Add CBFS_VERIFICATION support ...................................................................... decompressor: Add CBFS_VERIFICATION support CBFS_VERIFICATION requires the CBFS metadata hash anchor to be linked into an uncompressed stage, but for platforms using COMPRESS_BOOTBLOCK, this is only the decompressor stage. The first CBFS accesses are made in the bootblock stage after decompression, so if we want to make CBFS_VERIFICATION work on those platforms, we have to pass the metadata hash anchor from the decompressor into the bootblock. This patch does just that. (Note that this relies on the decompressor data remaining valid in memory for as long as the metadata hash anchor is needed. This is always true even for OVERLAP_DECOMPRESSOR_ROMSTAGE() situations because the FMAP and CBFS metadata necessarily need to have finished verification before a new stage could be loaded.) Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I2e6d7384cfb8339a24369eb6c01fc12f911c974e --- M src/include/bootblock_common.h M src/include/metadata_hash.h M src/lib/Kconfig.cbfs_verification M src/lib/Makefile.inc M src/lib/bootblock.c M src/lib/decompressor.c M src/lib/metadata_hash.c 7 files changed, 33 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/52085/1 diff --git a/src/include/bootblock_common.h b/src/include/bootblock_common.h index da627d2..fccd235 100644 --- a/src/include/bootblock_common.h +++ b/src/include/bootblock_common.h @@ -35,6 +35,7 @@ /* This is the argument structure passed from decompressor to bootblock. */ struct bootblock_arg { uint64_t base_timestamp; + void *metadata_hash_anchor; uint32_t num_timestamps; struct timestamp_entry timestamps[]; }; diff --git a/src/include/metadata_hash.h b/src/include/metadata_hash.h index 2d3b8a8..bfa7ef1 100644 --- a/src/include/metadata_hash.h +++ b/src/include/metadata_hash.h @@ -6,6 +6,11 @@ #include <commonlib/bsd/metadata_hash.h> +/* Return a pointer to the whole anchor. Only used for decompressor builds. */ +void *metadata_hash_export_anchor(void); +/* Import a pointer that points to the anchor. Only used for decompressor builds. */ +void metadata_hash_import_anchor(void *ptr); + /* Verify the an FMAP data structure with the FMAP hash that is stored together with the CBFS metadata hash in the bootblock's metadata hash anchor (when CBFS verification is enabled). */ vb2_error_t metadata_hash_verify_fmap(const void *fmap_base, size_t fmap_size); diff --git a/src/lib/Kconfig.cbfs_verification b/src/lib/Kconfig.cbfs_verification index a28df1f..fa90d9d 100644 --- a/src/lib/Kconfig.cbfs_verification +++ b/src/lib/Kconfig.cbfs_verification @@ -6,7 +6,6 @@ config CBFS_VERIFICATION bool # TODO: make user selectable once it works - depends on !COMPRESS_BOOTBLOCK # TODO: figure out decompressor anchor depends on !VBOOT_STARTS_BEFORE_BOOTBLOCK # this is gonna get tricky... select VBOOT_LIB help diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 074cb2a..358035d 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -38,6 +38,7 @@ decompressor-$(CONFIG_GENERIC_GPIO_LIB) += gpio.c decompressor-y += memchr.c decompressor-y += memcmp.c +decompressor-$(CONFIG_CBFS_VERIFICATION) += metadata_hash.c decompressor-y += prog_ops.c decompressor-$(CONFIG_COLLECT_TIMESTAMPS) += timestamp.c diff --git a/src/lib/bootblock.c b/src/lib/bootblock.c index 1509c8c..23fb392 100644 --- a/src/lib/bootblock.c +++ b/src/lib/bootblock.c @@ -4,6 +4,7 @@ #include <bootblock_common.h> #include <console/console.h> #include <delay.h> +#include <metadata_hash.h> #include <option.h> #include <post.h> #include <program_loading.h> @@ -88,6 +89,8 @@ void _start(struct bootblock_arg *arg); void _start(struct bootblock_arg *arg) { + if (CONFIG(CBFS_VERIFICATION)) + metadata_hash_import_anchor(arg->metadata_hash_anchor); bootblock_main_with_timestamp(arg->base_timestamp, arg->timestamps, arg->num_timestamps); } diff --git a/src/lib/decompressor.c b/src/lib/decompressor.c index 8ae9358..1d160e0 100644 --- a/src/lib/decompressor.c +++ b/src/lib/decompressor.c @@ -3,6 +3,7 @@ #include <bootblock_common.h> #include <commonlib/bsd/compression.h> #include <delay.h> +#include <metadata_hash.h> #include <program_loading.h> #include <symbols.h> #include <timestamp.h> @@ -42,6 +43,9 @@ if (CONFIG(COLLECT_TIMESTAMPS)) arg.base_timestamp = timestamp_get(); + if (CONFIG(CBFS_VERIFICATION)) + arg.metadata_hash_anchor = metadata_hash_export_anchor(); + decompressor_soc_init(); if (CONFIG(COLLECT_TIMESTAMPS)) diff --git a/src/lib/metadata_hash.c b/src/lib/metadata_hash.c index a823c5f..a4a4b9e 100644 --- a/src/lib/metadata_hash.c +++ b/src/lib/metadata_hash.c @@ -5,6 +5,7 @@ #include <metadata_hash.h> #include <symbols.h> +#if !CONFIG(COMPRESS_BOOTBLOCK) || ENV_DECOMPRESSOR __attribute__((used, section(".metadata_hash_anchor"))) static struct metadata_hash_anchor metadata_hash_anchor = { /* This is the only place in all of coreboot where we actually need to use this. */ @@ -12,15 +13,30 @@ .cbfs_hash = { .algo = CONFIG_CBFS_HASH_ALGO } }; +static struct metadata_hash_anchor *anchor_ptr = &metadata_hash_anchor; + +void *metadata_hash_export_anchor(void) +{ + return anchor_ptr; +} +#else +static struct metadata_hash_anchor *anchor_ptr = NULL; + +void metadata_hash_import_anchor(void *ptr) +{ + anchor_ptr = ptr; +} +#endif + struct vb2_hash *metadata_hash_get(void) { - return &metadata_hash_anchor.cbfs_hash; + return &anchor_ptr->cbfs_hash; } vb2_error_t metadata_hash_verify_fmap(const void *fmap_buffer, size_t fmap_size) { - struct vb2_hash hash = { .algo = metadata_hash_anchor.cbfs_hash.algo }; - memcpy(hash.raw, metadata_hash_anchor_fmap_hash(&metadata_hash_anchor), + struct vb2_hash hash = { .algo = anchor_ptr->cbfs_hash.algo }; + memcpy(hash.raw, metadata_hash_anchor_fmap_hash(anchor_ptr), vb2_digest_size(hash.algo)); return vb2_hash_verify(fmap_buffer, fmap_size, &hash); } -- To view, visit https://review.coreboot.org/c/coreboot/+/52085 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2e6d7384cfb8339a24369eb6c01fc12f911c974e Gerrit-Change-Number: 52085 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: cbfs: Add file data hashing for CONFIG_CBFS_VERIFICATION
by Julius Werner (Code Review) 02 Apr '21

02 Apr '21

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/+/52084 to review the following change. Change subject: cbfs: Add file data hashing for CONFIG_CBFS_VERIFICATION ...................................................................... cbfs: Add file data hashing for CONFIG_CBFS_VERIFICATION This patch adds file data hashing for CONFIG_CBFS_VERIFICATION. With this, all CBFS accesses using the new CBFS APIs (cbfs_load/_map/_alloc and variants) will be fully verified when verification is enabled. (Note that some use of legacy APIs remains and thus the CBFS_VERIFICATION feature is not fully finished.) Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: Ic9fff279f69cf3b7c38a0dc2ff3c970eaa756aa8 --- M src/commonlib/bsd/cbfs_private.c M src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h M src/lib/cbfs.c 3 files changed, 74 insertions(+), 13 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/52084/1 diff --git a/src/commonlib/bsd/cbfs_private.c b/src/commonlib/bsd/cbfs_private.c index 91f81e0..16bab7c 100644 --- a/src/commonlib/bsd/cbfs_private.c +++ b/src/commonlib/bsd/cbfs_private.c @@ -190,3 +190,26 @@ return NULL; } + +const struct vb2_hash *cbfs_file_hash(const union cbfs_mdata *mdata) +{ + /* Hashes are variable-length attributes, so need to manually check the length. */ + const struct cbfs_file_attr_hash *attr = + cbfs_find_attr(mdata, CBFS_FILE_ATTR_TAG_HASH, 0); + if (!attr) + return NULL; /* no hash */ + const size_t asize = be32toh(attr->len); + + const struct vb2_hash *hash = &attr->hash; + const size_t hsize = vb2_digest_size(hash->algo); + if (!hsize) { + ERROR("Hash algo %u for '%s' unsupported.\n", hash->algo, mdata->h.filename); + return NULL; + } + if (hsize != asize - offsetof(struct cbfs_file_attr_hash, hash.raw)) { + ERROR("Hash attribute size for '%s' (%zu) incorrect for algo %u.\n", + mdata->h.filename, asize, hash->algo); + return NULL; + } + return hash; +} diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h index df13427..ed5c378 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h @@ -24,4 +24,7 @@ else caller is responsible for checking the |len| field to avoid reading out-of-bounds. */ const void *cbfs_find_attr(const union cbfs_mdata *mdata, uint32_t attr_tag, size_t size_check); +/* Returns pointer to CBFS file hash structure in metadata attributes, or NULL if invalid. */ +const struct vb2_hash *cbfs_file_hash(const union cbfs_mdata *mdata); + #endif /* _COMMONLIB_BSD_CBFS_MDATA_H_ */ diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 9135b2f..23e8f41 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -186,11 +186,27 @@ return true; } -static size_t cbfs_load_and_decompress(const struct region_device *rdev, - void *buffer, size_t buffer_size, uint32_t compression) +static inline bool cbfs_file_hash_mismatch(const void *buffer, size_t size, + const struct vb2_hash *file_hash) +{ + /* Avoid linking hash functions when verification is disabled. */ + if (!CONFIG(CBFS_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; + + printk(BIOS_CRIT, "CBFS file hash mismatch!\n"); + return true; +} + +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) { size_t in_size = region_device_sz(rdev); - size_t out_size; + size_t out_size = 0; void *map; DEBUG("Decompressing %zu bytes to %p with algo %d\n", in_size, buffer, compression); @@ -201,6 +217,8 @@ return 0; if (rdev_readat(rdev, buffer, 0, in_size) != in_size) return 0; + if (cbfs_file_hash_mismatch(buffer, in_size, file_hash)) + return 0; return in_size; case CBFS_COMPRESS_LZ4: @@ -213,9 +231,11 @@ if (map == NULL) return 0; - timestamp_add_now(TS_START_ULZ4F); - out_size = ulz4fn(map, in_size, buffer, buffer_size); - timestamp_add_now(TS_END_ULZ4F); + if (!cbfs_file_hash_mismatch(map, in_size, file_hash)) { + timestamp_add_now(TS_START_ULZ4F); + out_size = ulz4fn(map, in_size, buffer, buffer_size); + timestamp_add_now(TS_END_ULZ4F); + } rdev_munmap(rdev, map); @@ -228,10 +248,12 @@ if (map == NULL) return 0; - /* Note: timestamp not useful for memory-mapped media (x86) */ - timestamp_add_now(TS_START_ULZMA); - out_size = ulzman(map, in_size, buffer, buffer_size); - timestamp_add_now(TS_END_ULZMA); + if (!cbfs_file_hash_mismatch(map, in_size, file_hash)) { + /* Note: timestamp not useful for memory-mapped media (x86) */ + timestamp_add_now(TS_START_ULZMA); + out_size = ulzman(map, in_size, buffer, buffer_size); + timestamp_add_now(TS_END_ULZMA); + } rdev_munmap(rdev, map); @@ -318,11 +340,18 @@ if (size_out) *size_out = size; + const struct vb2_hash *file_hash = NULL; + if (CONFIG(CBFS_VERIFICATION)) + file_hash = cbfs_file_hash(&mdata); + /* allocator == NULL means do a cbfs_map() */ if (allocator) { loc = allocator(arg, size, &mdata); } else if (compression == CBFS_COMPRESS_NONE) { - return rdev_mmap_full(&rdev); + void *mapping = rdev_mmap_full(&rdev); + if (!mapping || cbfs_file_hash_mismatch(mapping, size, file_hash)) + return NULL; + return mapping; } else if (!CBFS_CACHE_AVAILABLE) { ERROR("Cannot map compressed file %s on x86\n", mdata.h.filename); return NULL; @@ -335,7 +364,7 @@ return NULL; } - size = cbfs_load_and_decompress(&rdev, loc, size, compression); + size = cbfs_load_and_decompress(&rdev, loc, size, compression, file_hash); if (!size) return NULL; @@ -384,12 +413,18 @@ 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)) + return CB_CBFS_HASH_MISMATCH; if (mapping == prog_start(pstage)) return CB_SUCCESS; } @@ -405,7 +440,7 @@ } size_t fsize = cbfs_load_and_decompress(&rdev, prog_start(pstage), prog_size(pstage), - compression); + compression, file_hash); if (!fsize) return CB_ERR; -- To view, visit https://review.coreboot.org/c/coreboot/+/52084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic9fff279f69cf3b7c38a0dc2ff3c970eaa756aa8 Gerrit-Change-Number: 52084 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Aaron Durbin <adurbin(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 April 2021