The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/cf6b7a9c522a5b62714dee304ee4e6bec8a45ab8

commit cf6b7a9c522a5b62714dee304ee4e6bec8a45ab8
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Sep 28 17:10:46 2018

image_signing: workaround ecrw signing breakage

It looks like cbfstool removing & inserting blobs into the bios, even
if the contents are the same, break the signatures run over the region.
Until we can figure out what's going on, avoid re-adding content that's
the same to keep the signatures valid.

BRANCH=None
BUG= chromium:889716 
TEST=signing fizz image has valid vblock hashes

Change-Id: I00ba84cf22b6fffc594e60b78f91e7cb49c98f06
Reviewed-on: https://chromium-review.googlesource.com/1248201
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: C Shapiro <shapiroc@chromium.org>

[modify] https://crrev.com/cf6b7a9c522a5b62714dee304ee4e6bec8a45ab8/scripts/image_signing/common_minimal.sh

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/cros-signing/+/6d3def26a3fcc897b541bc28d80fb8984b6d30de

commit 6d3def26a3fcc897b541bc28d80fb8984b6d30de
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Sep 28 20:09:58 2018

Do we have logs of 'cbfstool print' before the file removal and after the file addition?

here's a spammy log from before my CL:1248201 that i think should include the debug details you want

Deleted: fizz-signer.log 723 KB

fizz-signer.log 723 KB View Download

From the description (all but last corrupted, rather than first corrupted), it sounds like the firmwares that actually shipped might be signed wrong? Any confirmation that this is the case or not?

Mike, we need '-r "FW_MAIN_A,FW_MAIN_B"' added to the print commands for before and after. It looks like we're just printing the RO CBFS -- not the RW CBFSes.

> it sounds like the firmwares that actually shipped might be signed wrong

i don't think that is correct.  i hand verified each rootkey/vblock passed with the respective bios images after my CL landed.  i suspect the cbfstool remove/add steps are not idempotent when the file removed/added is the same as what was there.

> we need '-r "FW_MAIN_A,FW_MAIN_B"' added to the print commands

new log attached

Deleted: fizz-signer-2.log 416 KB

fizz-signer-2.log 416 KB View Download

Thanks for the logs, Mike.

models/fizz_107ro_107rw/bios.bin

Before remove
ecrw                           0xf81c0    raw             66439 LZMA (131072 decompressed)
After remove
(empty)                        0xf81c0    null            66439 LZMA (131072 decompressed) <- this is a bug in that the attributes aren't cleared out
After add
ecrw                           0xf81c0    raw             66436 LZMA (131072 decompressed)

Size changed because of compression. But we should be re-signing after the addition. 

But if we're reusing the same image from one to the next that's probably not great. Though, if all the signing steps were always done it should just work, but if we're reusing the same base images to start with we're collecting differences. We should probably copy original for each step and work from there. 

To get an idea of the differences again:

Before:
fallback/postcar               0xf0240    stage           16088 none
fallback/dsdt.aml              0xf4180    raw             16361 none
ecrw                           0xf81c0    raw             66439 LZMA (131072 decompressed)
fallback/payload               0x108600   simple elf      85773 none

After Remove:
fallback/postcar               0xf0240    stage           16088 none
fallback/dsdt.aml              0xf4180    raw             16361 none
(empty)                        0xf81c0    null            66439 LZMA (131072 decompressed)
fallback/payload               0x108600   simple elf      85773 none

After Add:
fallback/postcar               0xf0240    stage           16088 none
fallback/dsdt.aml              0xf4180    raw             16361 none
ecrw                           0xf81c0    raw             66436 LZMA (131072 decompressed)
(empty)                        0x108580   null               88 none
fallback/payload               0x108600   simple elf      85773 none

I'm not entirely sure why having a dummy in the front fixes things. What is it like if all instructions were referencing the same versions?

before ecrw, the process was to sign the same FW_MAIN_A section with multiple keys for multiple devices.  see http://go/cros-loem-images for details.

with ecrw, we have one key per board (not per device), so we sign & insert the ec.bin into FW_MAIN_A.  it looks like that insertion is not idempotent hence we break previous signatures of FW_MAIN_A.

since the ec.bin can't actually be signed with different keys, having it be part of signer_config.csv is a bit weird.  people could (incorrectly) specify different ec.bin values for different devices which def wouldn't work.  as sanity, maybe we should:
- load the signer_config.csv
- make sure there are no lines that specify the same bios image but different ec images
- sign & insert all the ec.bin images
  - this also avoids the unnecessary signing duplication we do today
- walk all the (device, bios) pairs and sign the (now stable) FW_MAIN_[AB] blocks

> it sounds like the firmwares that actually shipped might be signed wrong
i don't think that is correct.  i hand verified each rootkey/vblock passed with the respective bios images after my CL landed. 

Thanks for checking, sounds great!

> But if we're reusing the same image from one to the next that's 
> probably not great.

This is not expected and it's probably causing the bug. cbfstool is invoked in chromeos-bootimage.ebuild with '-p' option to preserve extra space for signature size increase. store_file_in_cbfs is intended to be the last processor in the chain so it doesn't bother to add padding.

So, with a different key, ecrw is probably bloated, exceeding the originally allocated size.

> We should probably copy original for each step and work from there. 

I think this should fix the issue.

>> We should probably copy original for each step and work from there. 
>
> I think this should fix the issue.

either i'm not fully understanding what's being suggested, or i don't see how that could possibly work.  we explicitly need to sign the same bios image with multiple root/firmware keys.  that is fundamental to the loem design.

this design doc explains things:
  http://doc/1tNj_kkhbCIkf6J2B5nKCNFlZPLw5O19HFlI1WdX9z4I

here's how i reproduced things locally.  you'll need to revert my cf6b7a9c522a5b62714dee304ee4e6bec8a45ab8 to see the failure locally.

(0) create a scratch space for everything.
mkdir ~/crbug889716

(1) create a loem keyset inside the sdk.
cd ~/crbug889716
/mnt/host/source/src/platform/vboot_reference/scripts/keygeneration/create_new_keys.sh --android --output keys
cd keys
chmod 755 .
chmod 644 *
/mnt/host/source/src/platform/vboot_reference/scripts/keygeneration/add_loem_keys.sh 5
cat <<EOF >loem.ini
[loem]
1 = KENCH
2 = TEEMO
3 = SION
4 = DEFAULT
5 = CTL
6 = VIEWSONIC
EOF

(2) get a relevant recovery image.
cd ~/crbug889716
gsutil cp gs://chromeos-releases/beta-channel/fizz/10323.64.0/ChromeOS-recovery-R65-10323.64.0-fizz.tar.xz ./
tar xf ChromeOS-recovery-R65-10323.64.0-fizz.tar.xz

(3) sign the recovery image.
cd ~/crbug889716
/mnt/host/source/src/platform/vboot_reference/scripts/image_signing/sign_official_build.sh \
  recovery recovery_image.bin $PWD/keys signed.bin

(4) unpack the firmware in the image.
cd ~/crbug889716
mkdir root
sudo losetup -P /dev/loop3 signed.bin
sudo mount -o ro /dev/loop3p3 root
cp ./root/usr/sbin/chromeos-firmwareupdate ./
./chromeos-firmwareupdate --sb_extract fw/
sudo umount root
sudo losetup -d /dev/loop3

(5) check the signed firmware.
cd ~/crbug889716
futility dump_fmap -x fw/models/fizz_107ro_107rw/bios.bin FW_MAIN_A
futility vbutil_firmware --verify fw/keyset/vblock_A.teemo --signpubkey fw/keyset/rootkey.teemo --fv FW_MAIN_A
futility vbutil_firmware --verify fw/keyset/vblock_A.kench --signpubkey fw/keyset/rootkey.kench --fv FW_MAIN_A

at this point, teemo prob passes but kench fails due to this bug.

if you want to speed things up a bit, you can hack up sign_official_build.sh:
- copy the chromeos-firmwareupdate script out of the disk image (see step 4 above)
- change sign_official_build.sh so that, at the top of sign_image_file(), it calls `resign_firmware_payload chromeos-firmwareupdate` directly and then `exit 0`
- change resign_firmware_payload() so that it skips the loop setup code and just set `local firmware_bundle="$1"`

The problem (or a problem) is when you run

  futility vbutil_firmware --verify fw/keyset/vblock_A.teemo --signpubkey fw/keyset/rootkey.teemo --fv FW_MAIN_A

FW_MAIN_A is not necessarily the right firmware body. This command

  futility dump_fmap -x fw/models/fizz_107ro_107rw/bios.bin FW_MAIN_A

extracts FW_MAIN_A from fizz_107ro_107rw/bios.bin but as signer_config.csv says:

  fizz,models/fizz_103ro_103rw/bios.bin,DEFAULT,models/fizz_103ro_103rw/ec.bin
  kench,models/fizz_107ro_107rw/bios.bin,KENCH,models/fizz_107ro_107rw/ec.bin
  sion,models/fizz_103ro_103rw/bios.bin,SION,models/fizz_103ro_103rw/ec.bin
  teemo,models/fizz_107ro_107rw/bios.bin,TEEMO,models/fizz_107ro_107rw/ec.bin
  ...

some use fizz_103ro_103rw/bios.bin and others use fizz_107ro_107rw/bios.bin. So, if you extract right FW_MAIN_A, 'futility vbutil_firmware' should pass for all OEMs.

I extracted all intermediate bios.bin created by sign_official_build.sh. They all pass verification test by 'futility show'.

i don't see a problem there.  the csv says to:
- sign 103 with fizz & sion
- sign 107 with kench & teemo

so trying to verify 107 with fizz & sion doesn't make sense, nor does verifying 103 with kench & teemo.

if you disable the ecrw logic entirely, the signing works fine for both images.

A fix has been proposed: https://review.coreboot.org/c/coreboot/+/28886.

It was caused by cbfstool not clearing space occupied by ecrw when it's removed first time (for Kench). When it's removed first time, a null entry is created between ecrw and fallback/payload. So, ecrw is cleared as expected when it's removed second time (for Teemo).

So, ecrw is written to a dirty space for Kench and to a clean space for Teemo. In the repro above, FW_MAIN_A is extracted from bios.bin which is last processed for the latter case (Teemo, and successive models). Thus, verification fails for Kench (because it expects ecrw in uncleared space).

What do you mean null entry? You mean empty? And i don't understand the ordering w.r.t. "cleared as expected when it's removed second time". How do we remove it twice?

What I still don't understand is that if we're signing FW_MAIN_x after adding EC, how does verification fail? The signature would include whatever garbage was left over. Or are we not actually processing each instruction fully when signing? i.e. copy orig .bin, remove ecrw entries, sign ecrw, add ecrw entries, sign FW_MAIN_x? I don't see how the signature could ever fail in that situation.

1. ecrw is extracted from ec.bin
2. ecrw is signed
3. ecrw is removed from bios.bin
4. ecrw is written to bios.bin
5. bios.bin* is signed. vblock_a.kench is produced
6. repeat 1 through 4
7. bios.bin** is signed. vblock_a.teemo is produced

vblock_a.kench has a signature based on bios.bin*. vblock_a.teemo has a signature based on bios.bin**. The problem is bios.bin* and bios.bin** are different even though we're writing the same ecrw. Here is how the discrepancy happens:

Before ecrw is removed, cbfs tightly packed:
  fallback/dsdt.aml
  ecrw
  fallback/payload

When ecrw is removed, cbfs looks like:
  fallback/dsdt.aml
  empty
  junk
  fallback/payload

After ecrw is written for kench, cbfs looks like:
  fallback/dsdt.aml
  ecrw + 0xe7
  empty
  fallback/payload

0xe7 is a leftover from the junk. When ecrw is removed, cbfs looks like:
  fallback/dsdt.aml
  empty
  fallback/payload

This time there is no junk left in the deleted space. After ecrw is written for teemo, cbfs looks like:
  fallback/dsdt.aml
  ecrw + 0xff
  empty
  fallback/payload

thanks for tracking that down

should the upstream change include a unittest ?  seems like subtle behavior that's easy to break.

I get it now. I thought we were using the bios.bin that we signed FW_MAIN_X from and stuffing things back in. Instead we're just placing certain regions into the same image repeatedly.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/9a4a28561a29379423bb14daea9c8c0c48ee8afe

commit 9a4a28561a29379423bb14daea9c8c0c48ee8afe
Author: Daisuke Nojiri <dnojiri@chromium.org>
Date: Fri Oct 05 18:37:24 2018

UPSTREAM: cbfstool: Clear entry being removed in all cases

Currently, an entry being removed is cleared only if the next entry
is also null or deleted.

This patch ensures the entry being removed is cleared regardless of
the next entry type.

BUG= chromium:889716 
BRANCH=none
TEST=Run cbfstool bios.bin remove -n ecrw.
Verify bios.bin has 0xFF in the space of the removed entry.
TEST=Run cbfstool bios.bin remove -n fallback/payload (located at the end).
Verify fallback/payload is removed.
TEST=Run sign_official_build.sh on recovery_image.bin. Extract
firmware contents from chromeos-firmwareupdate in the resigned image.
Run 'futility vbutil_firmware --verify' for vblock_A's and FW_MAIN_A
extracted from bios.bin. See the bug for details.

Change-Id: Ia3798603321c1cdd48eefeb4541f74674d10fe11
Signed-off-by: Martin Roth <martinroth@chromium.org>
Original-Commit-Id: 2b59c610d0eda1be7774f32cf417ee7f7c60c582
Original-Change-Id: I62540483da6cc35d0a604ec49b2f2b7b11ba9ce5
Original-Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
Original-Reviewed-on: https://review.coreboot.org/28886
Original-Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Original-Reviewed-by: Aaron Durbin <adurbin@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1263085
Commit-Ready: Patrick Georgi <pgeorgi@chromium.org>

[modify] https://crrev.com/9a4a28561a29379423bb14daea9c8c0c48ee8afe/util/cbfstool/cbfs_image.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/ea1a49673ec6736545156a7ca30198f957ebfb2f

commit ea1a49673ec6736545156a7ca30198f957ebfb2f
Author: Daisuke Nojiri <dnojiri@chromium.org>
Date: Wed Nov 28 04:00:24 2018

UPSTREAM: cbfstool: Clear entry being removed in all cases

Currently, an entry being removed is cleared only if the next entry
is also null or deleted.

This patch ensures the entry being removed is cleared regardless of
the next entry type.

BUG= chromium:889716 
BRANCH=none
TEST=Run cbfstool bios.bin remove -n ecrw.
Verify bios.bin has 0xFF in the space of the removed entry.
TEST=Run cbfstool bios.bin remove -n fallback/payload (located at the end).
Verify fallback/payload is removed.
TEST=Run sign_official_build.sh on recovery_image.bin. Extract
firmware contents from chromeos-firmwareupdate in the resigned image.
Run 'futility vbutil_firmware --verify' for vblock_A's and FW_MAIN_A
extracted from bios.bin. See the bug for details.

Change-Id: Ia3798603321c1cdd48eefeb4541f74674d10fe11
Signed-off-by: Martin Roth <martinroth@chromium.org>
Original-Commit-Id: 2b59c610d0eda1be7774f32cf417ee7f7c60c582
Original-Change-Id: I62540483da6cc35d0a604ec49b2f2b7b11ba9ce5
Original-Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
Original-Reviewed-on: https://review.coreboot.org/28886
Original-Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Original-Reviewed-by: Aaron Durbin <adurbin@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1263085
Commit-Ready: Patrick Georgi <pgeorgi@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/1352834

[modify] https://crrev.com/ea1a49673ec6736545156a7ca30198f957ebfb2f/util/cbfstool/cbfs_image.c