summaryrefslogtreecommitdiff
path: root/tests/futility
Commit message (Collapse)AuthorAgeFilesLines
* futility: Add read commandEvan Benn2023-01-062-0/+22
| | | | | | | | | | | | | | | | | | Add a command that reads AP firmware to a specified file path. BUG=b:260531154 BRANCH=None TEST=FEATURES=test emerge-grunt vboot_reference TEST=futility read /tmp/bios TEST=futility read /tmp/bios -p ec TEST=env SERVOD_NAME=grunt futility read /tmp/bios --servo Change-Id: I82fe0381b6f61ca4d67a9f5c27353e18ed4abe39 Signed-off-by: Evan Benn <evanbenn@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/4075310 Reviewed-by: Edward O'Callaghan <quasisec@chromium.org> Reviewed-by: Yu-Ping Wu <yupingso@chromium.org> Commit-Queue: Edward O'Callaghan <quasisec@chromium.org>
* futility: Add read/write flash capability to gbb commandEvan Benn2023-01-061-0/+6
| | | | | | | | | | | | | | | | | | | | | gbb command can read and modify flash in addition to acting on firmware files. BUG=b:260531154 BRANCH=None TEST=FEATURES=test emerge-grunt vboot_reference TEST=futility gbb -s --flags 0x0 /tmp/bios /tmp/bios2 TEST=futility gbb -g --flash TEST=futility gbb --set --flash --flags=0x40b9 --flash TEST=env SERVOD_NAME=grunt futility gbb --get --servo TEST=env SERVOD_NAME=grunt futility gbb --set --servo --flags=0 Change-Id: I66b008ed7325d125eb305e84185e53eccd243898 Signed-off-by: Evan Benn <evanbenn@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/4075311 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org> Commit-Queue: Edward O'Callaghan <quasisec@chromium.org> Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
* futility: updater: Detect the model via FRID for non-host programmersSam McNally2022-12-121-0/+17
| | | | | | | | | | | | | | | | | | | | | | | When updating with --archive and a non-host programmer (and thus no reliable crosid to discover the appropriate firmware manifest key), and no explicit --model parameter is passed, try to detect the model by matching the FRID of the current firmware with one of the host firmware images in the archive. Add a --detect-model-only flag to perform the same matching, but report the detected model name and exit. This can be used in combination with the manifest to automatically select an appropriate EC image to pass to flash_ec. BUG=b:253966060 TEST=futility update -a firmware.tar.bz2 --servo BRANCH=None Signed-off-by: Sam McNally <sammc@chromium.org> Change-Id: I25fa0f109d0d8052179b220251d4720438b93bc4 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3965584 Reviewed-by: Edward O'Callaghan <quasisec@chromium.org> Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* futility: try ignoring GBB flags when validating GSCVDVadim Bendebury2022-12-011-4/+15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | GBB flags contents are ignored when AP RO ranges hash is calculated. The embedded verification will succeed only if the flags are cleared, but the command line tool should not fail because of nonzero GBB flags. This patch adds add additional pass when validating to see if validation succeeds with GBB flags zeroed. Also adding a debug printout to allow the user to see ranges covered by the signature when validating an image and modifying the tests to accommodate passing when GBB flags are non-zero. BRANCH=none BUG=none TEST=successfully validated AP RO signature with the same image with and without cleared gbb flags. When checking the image with nonzero flags the 'Ranges digest matches with zeroed GBB flags' warning message is printed. invoking 'make runtests' succeeds. Signed-off-by: Vadim Bendebury <vbendeb@chromium.org> Change-Id: I3e38924f14697a3efd058286f9579d89e5161910 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/4049934 Commit-Queue: Yu-Ping Wu <yupingso@chromium.org> Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* futility: updater: Only apply the preserve_me quirk for autoupdates.Sam McNally2022-12-011-1/+16
| | | | | | | | | | | | | | | | | | | | | | The preserve_me applies for all non-factory updates for firmware with the quirk enabled. It was only really intended to apply to firmware updates during autoupdates, that is --mode=autoupdate. Instead, we checked for an archive, which is always set, possibly a fallback directory archive rather than an archive file, resulting in it being used except for --mode=factory. Switch the condition to TRY_UPDATE_AUTO instead so only --mode=autoupdate enables the preserve_me quirk. BUG=b:255447297 TEST=futility update -i /tmp/image.bin doesn't apply the quirk futility update -i /tmp/image.bin -m autoupdate applies the quirk BRANCH=None Change-Id: I7459f027a918dc70cbde1bfc6f5da2b549bcc513 Signed-off-by: Sam McNally <sammc@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/4050014 Reviewed-by: Karthikeyan Ramasubramanian <kramasub@google.com> Reviewed-by: Edward O'Callaghan <quasisec@chromium.org> Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* futility: updater: Use flashrom dummy programmer to implement --emulateSam McNally2022-11-171-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | futility update --emulate and flashrom's dummy programmer serve similar purposes - both provide support for using a file instead of a real ROM. The current --emulate implementation involves special-casing before interacting with flashrom and pre-filling in the current image contents; an appropriately-configured dummy programmer and unmodified flashrom interactions could accomplish the same outcome with a more centralised handling of --emulate. Other --emulate interactions mock out non-flashrom interactions, so need to continue handling --emulate specially for now. Switch --emulate to use the dummy programmer. Add an extra field to store the original programmer option for deciding on whether the apply the preserve_me quirk. BUG=b:253966060 TEST=unit tests BRANCH=None Signed-off-by: Sam McNally <sammc@chromium.org> Change-Id: I687749523f54edcb9dd41cfc85614949b9d6607a Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3965582 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org> Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
* Makefile: Fix and simplify the RUNTEST test wrapperEvan Benn2022-10-251-2/+1
| | | | | | | | | | | | | | | | | | | | | Remove the qemu logic from the Makefile. Document the RUNTEST, BUILD_RUN and SRC_RUN variables. Ensure those variables are used consistently throughout the Makefile and test scripts. BUG=b:231084609 BRANCH=None TEST=FEATURES=test emerge vboot_reference TEST=FEATURES=test emerge-amd64-generic vboot_reference TEST=FEATURES=test emerge-hatch coreboot TEST=(coreboot upstream with this patch) make all TEST=make BUILD=build1 runtests TEST=make BUILD=build2 RUNTEST=env runtests Cq-Depend: chromium:3934904 Change-Id: Ifd18463d681bedbf7464165f2df0181474b36791 Signed-off-by: Evan Benn <evanbenn@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3831828 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* gscvd: presume GBB flags are zero when hashing the RO space contentsstabilize-15208.Bstabilize-15207.BVadim Bendebury2022-10-225-0/+108
| | | | | | | | | | | | | | | | | | | | | | | | | | | It is still being debated who is supposed to make sure that the GBB flags are set to zero before the root of trust validation is granted to the AP firmware image, but as of today the approach is that the GBB flags must be zero at AP RO validation time. The problem is that when AP RO space signature is created GBB flags can be set to a non-zero value. With this patch when AP RO areas contents is hashed, in case GBB flags are included in one of the ranges, the flags are not read from the flash, and substituted with zero. During validation the real flags value is used. A unit test is added to verify various futility gscvd GBB related situations, the blobs for the unit test were extracted from a Nivviks firmware image. BRANCH=none BUG=b:245799496, b:253540670 TEST='./tests/futility/test_gscvd.sh' and 'make runfutiltests' succeed Change-Id: I2f047b990cf71ea24d191fc690da08e25ebb10cc Signed-off-by: Vadim Bendebury <vbendeb@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3958581 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* treewide: Fix copyrights and extra new lines at end of fileJakub Czapiga2022-10-2121-21/+21
| | | | | | | | | | | | | BUG=none BRANCH=none TEST=make runtests Signed-off-by: Jakub Czapiga <jacz@semihalf.com> Change-Id: If93a65ba58c4973d4b344229c7ee26685395bbbf Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3964274 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org> Commit-Queue: Jakub Czapiga <czapiga@google.com> Tested-by: Jakub Czapiga <czapiga@google.com>
* treewide: Fix license headers to conform with linterJakub Czapiga2022-10-043-3/+3
| | | | | | | | | | | | | BRANCH=none BUG=none TEST=cros lint Signed-off-by: Jakub Czapiga <jacz@semihalf.com> Change-Id: I7710c43c8c70cf257a898f22c42ecbf350e125a2 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3925702 Commit-Queue: Jakub Czapiga <czapiga@google.com> Reviewed-by: Yu-Ping Wu <yupingso@chromium.org> Tested-by: Jakub Czapiga <czapiga@google.com>
* tests/futility/test_update.sh: Document test_update functionstabilize-15054.98.Bstabilize-15054.26.Bstabilize-15054.115.Brelease-R106-15054.BEvan Benn2022-08-171-0/+6
| | | | | | | | | | | BUG=b:231084609 BRANCH=None TEST=None Change-Id: Id76f2469faa13c136c6ec2761577acec4ad810e5 Signed-off-by: Evan Benn <evanbenn@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3831833 Reviewed-by: Hung-Te Lin <hungte@chromium.org>
* futility: Add --keyset option to sign command for BIOS and kernelstabilize-14998.Bfactory-foobar-15000.BJakub Czapiga2022-07-223-36/+16
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This patch adds --keyset option for sign command for BIOS_IMAGE, RAW_FIRMWARE, RAW_KERNEL and KERN_PREAMBLE file types. The default value of this option is '/usr/share/vboot/devkeys'. It allows futility to load public and private keys, and keyblocks from under this path, when they were not provided manually using their respective options. Files loaded by default for BIOS_IMAGE and RAW_FIRMWARE: - ${keysetdir}/firmware_data_key.vbprivk - ${keysetdir}/firmware.keyblock - ${keysetdir}/kernel_subkey.vbpubk Files loaded by default for RAW_KERNEL: - ${keysetdir}/kernel_data_key.vbprivk - ${keysetdir}/kernel.keyblock File loaded by default for KERN_PREAMBLE: - ${keysetdir}/kernel_data_key.vbprivk BUG=none BRANCH=none TEST=make runfutiltests Signed-off-by: Jakub Czapiga <jacz@semihalf.com> Change-Id: Ic4026d501d88e0de7d2c6f52c7494c639d08bd15 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3740601 Auto-Submit: Jakub Czapiga <czapiga@google.com> Reviewed-by: Julius Werner <jwerner@chromium.org> Commit-Queue: Julius Werner <jwerner@chromium.org> Tested-by: Jakub Czapiga <czapiga@google.com>
* tests/common/tests.h: rename test_common.h to common/tests.hHsin-Te Yuan2022-07-152-2/+2
| | | | | | | | | | | | | | Create tests/common/ to put some common files. BUG=none BRANCH=none TEST=make runtests Signed-off-by: Hsin-Te Yuan <yuanhsinte@google.com> Change-Id: I8918b7a1e62d47fca6074ef123e2de6f46f1aa00 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3754814 Reviewed-by: Hsuan Ting Chen <roccochen@chromium.org> Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* futility/file_type_bios: Rework image signingJakub Czapiga2022-06-3030-37/+431
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | This patch reworks whole BIOS image signing to support images with CBFS, and with ponly RW/A slot. CBFS images will now be truncated to eliminate unnecessary empty space, and will sign only the part of firmware area which contains the data, and not empty space. This patch also adds more checks for potential errors, and does not allow for signing incorrect nor uses data from structures, which might not be valid. futility sign command tests are also greatly extended to cover a wide variety of possible errors, which have to be handled correctly. BUG=b:197114807 TEST=sudo emerge vboot_reference TEST=build whole chromeos-bootimage after making it and coreboot use `futility sign --type bios ...` TEST=make runtests BRANCH=none Signed-off-by: Jakub Czapiga <jacz@semihalf.com> Cq-Depend: chromium:3707104 Change-Id: I7c84aa38776e8890a87f0e9b7ec7f32d86f82c13 Disallow-Recycled-Builds: test-failures Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3575325 Tested-by: Jakub Czapiga <czapiga@google.com> Reviewed-by: Julius Werner <jwerner@chromium.org> Auto-Submit: Jakub Czapiga <czapiga@google.com> Commit-Queue: Jakub Czapiga <czapiga@google.com>
* tests: Fix most of errors reported by shellcheckJakub Czapiga2022-06-2318-582/+608
| | | | | | | | | | | | BUG=none BRANCH=none TEST=make runtests Signed-off-by: Jakub Czapiga <czapiga@google.com> Change-Id: I364ac6ace35705f1cfdaec71297523d4c2132b75 Disallow-Recycled-Builds: test-failures Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3695417 Reviewed-by: Julius Werner <jwerner@chromium.org>
* futility: Remove --devsign and --devkeyblockJakub Czapiga2022-05-308-44/+23
| | | | | | | | | | | | | | | | | | | | | This feature has not been needed since pre-2012 devices which have long since reached their end of life. We can safely remove it to simplify the code. Also remove ZGB image, as it is no longer needed. BUG=b:197114807 TEST=sudo FEATURES=test emerge vboot_reference BRANCH=none Signed-off-by: Jakub Czapiga <jacz@semihalf.com> Cq-Depend: chromium:3650757 Change-Id: I889dc6300c5cb72bdfcb9c2b66d63e97d3f8c862 Disallow-Recycled-Builds: test-failures Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3578968 Commit-Queue: Jakub Czapiga <czapiga@google.com> Auto-Submit: Jakub Czapiga <czapiga@google.com> Tested-by: Jakub Czapiga <czapiga@google.com> Reviewed-by: Julius Werner <jwerner@chromium.org>
* futility/file_type_bios: Remove old FlashMap area namesJakub Czapiga2022-04-277-8069/+0
| | | | | | | | | | | | | | | | | | | | Old names are not in use for very long time, so remove them. BUG=b:197114807 TEST=cros-workon-volteer start vboot_reference && \ FW_NAME=voxel emerge-volteer vboot_reference coreboot chromeos-bootimage TEST=sudo FEATURES=test emerge vboot_reference BRANCH=none Signed-off-by: Jakub Czapiga <jacz@semihalf.com> Change-Id: I07916b82a721481c982b291e228df0772e0fc2a2 Disallow-Recycled-Builds: test-failures Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3575323 Auto-Submit: Jakub Czapiga <czapiga@google.com> Commit-Queue: Jakub Czapiga <czapiga@google.com> Reviewed-by: Julius Werner <jwerner@chromium.org> Tested-by: Jakub Czapiga <czapiga@google.com>
* futility: updater: rename 'customlabel_tag' to 'custom_label_tag'Hung-Te Lin2022-03-181-2/+2
| | | | | | | | | | | | | | | | Per discussion, the 'custom_label_tag' is easier to read than 'customlabel_tag'. We should rename it before any real devices have started using the different names. BUG=b:169766857 TEST=make; build and run test BRANCH=None Change-Id: I3672e7b20bc85f79796470ba1a58c2896d26ff88 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3534491 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org> Commit-Queue: Yu-Ping Wu <yupingso@chromium.org>
* futility: updater: Change 'whitelabel_tag' to 'customlabel_tag'Hung-Te Lin2022-03-072-38/+39
| | | | | | | | | | | | | | | | | | | | Support new VPD name 'customlabel_tag' for the custom label program. For shipped devices (firmware is already locked and write protected) we still support the legacy name. The quirk 'allow_empty_wl_tag' also renamed to 'allow_empty_customlabel_tag'. This is usually not recommended, but given no devices have used this quirk in the CBFS quirks, it should be fine to change the quirk name. BUG=b:169766857 TEST=make; build and run test BRANCH=None Change-Id: Ia29051a4e829d853cc60488f286d575c20f52f20 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3503199 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* futility/updater: Ignore preserve_me quirks for non-host programmers.Sam McNally2022-02-131-0/+14
| | | | | | | | | | | | | | | | | | | | | | | | | The preserve_me quirk allows avoiding modifying the ME region while it may be running. Its apply function attempts to skip itself when not flashing the OS-bundled firmware by checking for whether an archive is set, but this doesn't work since in the absence of an actual archive file, a filesystem archive implementation is used instead. While flashing over a non-host programmer the ME is not running and therefore it is safe to update the ME region. Add unit test cases for the preserve_me quirk applying successfully when using the default host programmer and being skipped when using another programmer. BUG=b:213706510 TEST=futility update -p dummy... with preserve_me quirk skips the quirk; chromeos-firmwareupdate with a preserve_me quirk applies the quirk BRANCH=none Change-Id: Ie5578c9b3cf7eba55626bb931589bf360fe28269 Signed-off-by: Sam McNally <sammc@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3450060 Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
* futility: drop sudo from test scriptstabilize-14498.Bstabilize-14496.Bfirmware-brya-14505.71.BNikolai Artemiev2022-02-011-2/+2
| | | | | | | | | | | | | | Command does not need to run under sudo and sudo will not be available once we start running the test scripts under platform2_test.py. BUG=b:207787495 BRANCH=none TEST=`cros_run_unit_tests --board grunt --packages vboot_reference` Signed-off-by: Nikolai Artemiev <nartemiev@google.com> Change-Id: I795519c4b45e410f5ddc3c55dceab1ae1de02dbc Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3428421 Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
* vboot: introduce minios_kernel.keyblockJoel Kitching2021-07-054-5/+5
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | miniOS requires a distinct kernel data key, whose dev key pair is added in this CL as minios_kernel_data_key.vb{pub,priv}k. A distinct keyblock is also required. The keyblock should set the kernel keyblock flag MINIOS_1. Other keyblocks are modified appropriately to set MINIOS_0. Keyblocks were generated using the following commands: $ futility vbutil_keyblock --flags 23 --datapubkey tests/devkeys/ec_data_key.vbpubk --signprivate tests/devkeys/ec_root_key.vbprivk --pack tests/devkeys/ec.keyblock Keyblock file: tests/devkeys/ec.keyblock Signature valid Flags: 23 !DEV DEV !REC !MINIOS Data key algorithm: 7 RSA4096 SHA256 Data key version: 1 Data key sha1sum: 5833470fe934be76753cb6501dbb8fbf88ab272b $ futility vbutil_keyblock --flags 23 --datapubkey tests/devkeys/firmware_data_key.vbpubk --signprivate tests/devkeys/root_key.vbprivk --pack tests/devkeys/firmware.keyblock Keyblock file: tests/devkeys/firmware.keyblock Signature valid Flags: 23 !DEV DEV !REC !MINIOS Data key algorithm: 7 RSA4096 SHA256 Data key version: 1 Data key sha1sum: e2c1c92d7d7aa7dfed5e8375edd30b7ae52b7450 $ futility vbutil_keyblock --flags 27 --datapubkey tests/devkeys/recovery_kernel_data_key.vbpubk --signprivate tests/devkeys/recovery_key.vbprivk --pack tests/devkeys/recovery_kernel.keyblock Keyblock file: tests/devkeys/recovery_kernel.keyblock Signature valid Flags: 27 !DEV DEV REC !MINIOS Data key algorithm: 11 RSA8192 SHA512 Data key version: 1 Data key sha1sum: e78ce746a037837155388a1096212ded04fb86eb $ futility vbutil_keyblock --flags 43 --datapubkey tests/devkeys/minios_kernel_data_key.vbpubk --signprivate tests/devkeys/recovery_key.vbprivk --pack tests/devkeys/minios_kernel.keyblock Keyblock file: tests/devkeys/minios_kernel.keyblock Signature valid Flags: 43 !DEV DEV REC MINIOS Data key algorithm: 8 RSA4096 SHA512 Data key version: 1 Data key sha1sum: 65441886bc54cbfe3a7308b650806f4b61d8d142 $ futility vbutil_keyblock --flags 23 --datapubkey tests/devkeys/kernel_data_key.vbpubk --signprivate tests/devkeys/kernel_subkey.vbprivk --pack tests/devkeys/kernel.keyblock Keyblock file: tests/devkeys/kernel.keyblock Signature valid Flags: 23 !DEV DEV !REC !MINIOS Data key algorithm: 4 RSA2048 SHA256 Data key version: 1 Data key sha1sum: d6170aa480136f1f29cf339a5ab1b960585fa444 $ futility vbutil_keyblock --flags 26 --datapubkey tests/devkeys/installer_kernel_data_key.vbpubk --signprivate tests/devkeys/recovery_key.vbprivk --pack tests/devkeys/installer_kernel.keyblock Keyblock file: tests/devkeys/installer_kernel.keyblock Signature valid Flags: 26 DEV REC !MINIOS Data key algorithm: 11 RSA8192 SHA512 Data key version: 1 Data key sha1sum: e78ce746a037837155388a1096212ded04fb86eb BUG=b:188121855 TEST=make clean && make runtests BRANCH=none Signed-off-by: Joel Kitching <kitching@google.com> Change-Id: I5b3e4def83ff29ca156b3c84dfcb8398f4985e67 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2965485 Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Julius Werner <jwerner@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org>
* futility: update: add new quirk 'no_check_platform'Hung-Te Lin2020-12-091-0/+10
| | | | | | | | | | | | | | | | | | | | | | Some devices may have flashed firmware with different platform name in their early stage (especially in the first build of leading devices), so we do want to provide an explicit way (not just --force) to skip checking platform name. The change CL:2059621 does not help because the loaded system firmware looks good. This is implemented as a quirk so we can enable it using a CBFS quirk file, making it easier to be deployed by auto update. BRANCH=None BUG=None TEST=make runtests Change-Id: I888d5848921d31c9b7cba1b96c42d38fda71927e Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2573999 Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
* futility: update: load quirks from firmware image CBFS filestabilize-rust-13555.BHung-Te Lin2020-10-161-0/+12
| | | | | | | | | | | | | | | | | | | | The firmware updater now looks at CBFS 'FW_MAIN_A' (RW A) and if a text file 'updater_quirks' is found, the contents will be fetched to setup default quirks. This helps sharing same customization across multiple firmware images (for different models) shared by same unibuild OS image. Without that, we have to maintain a large list of hard-coded model names in firmware updater source. BRANCH=none BUG=b:169284414 TEST=make runtests Signed-off-by: Hung-Te Lin <hungte@chromium.org> Change-Id: I938bffe9f16bc3adee0dc3efb6976efe581c6d8c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2426093 Reviewed-by: Karthikeyan Ramasubramanian <kramasub@chromium.org>
* futility: update: support multi-line quirksHung-Te Lin2020-10-151-0/+5
| | | | | | | | | | | | | | | To support loading quirks from external files, we want to skip tab (\t) and new line characters (\n, \r). BRANCH=none BUG=b:169284414 TEST=make runtests Change-Id: If314d6cf36907837ce9c36b73337976ee0c6fad1 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2467305 Reviewed-by: Karthikeyan Ramasubramanian <kramasub@chromium.org> Commit-Queue: Karthikeyan Ramasubramanian <kramasub@chromium.org>
* futility: update: Add '--gbb_flags FLAGS' to override GBB flagsHung-Te Lin2020-08-291-1/+7
| | | | | | | | | | | | | | | Developers may want to use the new GBB flags when flashing a firmware image. That can be done by --factory, but it's also more convenient to have a new parameter for overriding the flags with a new value. BRANCH=none BUG=b:166569397 TEST=make runtests Change-Id: If9dce9b1f2fbb27655ad2a111ba75ab83375fb7a Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2382991 Reviewed-by: Julius Werner <jwerner@chromium.org>
* test_update.sh: Avoid using grep -q together with -o pipefailJulius Werner2020-02-062-2/+2
| | | | | | | | | | | | | | | | | | | | | | Piping something into 'grep -q' when the shell option '-o pipefail' is set is racy: 'grep -q' exits immediately after seeing the first occurence of the pattern, so if the process at the front of the pipe hasn't written all its data into the pipe buffer yet, it will still try to write more after grep has already exited and die with a SIGPIPE. The recommended solution seems to be using a <<<"herestring" instead. (Also add the test's return code to the FAILED output in run_test_scripts.sh to aid future test script debugging.) BRANCH=None BUG=chromium:1048048 TEST=make runtests Change-Id: I2f2589f223d9179d694565f5733535d4270699ea Signed-off-by: Julius Werner <jwerner@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2039946 Reviewed-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Hung-Te Lin <hungte@chromium.org>
* vboot: unify common.sh for testing scriptsstabilize-12881.BJoel Kitching2020-02-0312-157/+95
| | | | | | | | | | | | | | | | Use tests/common.sh instead of tests/futility/common.sh. Correct SCRIPT_DIR value to allow running run_test_scripts.sh standalone without using Makefile. BUG=b:124141368, chromium:605348 TEST=make clean && make runfutiltests BRANCH=none Change-Id: I107952826ea9a3a3816d9c13206aa48bee63ac6c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2014236 Tested-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org> Reviewed-by: Julius Werner <jwerner@chromium.org>
* tests: Update futility show expected test outputJulius Werner2019-11-021-1/+1
| | | | | | | | | | | | | | | | | | If you read the manpage for printf *really* closely, it seems that the %#x token only prints the 0x prefix for non-zero values. Huh... never knew that (and our firmware implementations in fact don't honor that, but glibc does). Anyway, I think we're fine with either behavior but this broke the expected output for one of our futility tests, which this patch fixes (originally broken in CL:1840191). BRANCH=None BUG=None TEST=make runtests Change-Id: Id54ff6f56e02333ab01b09b75deb16f47da01bc3 Signed-off-by: Julius Werner <jwerner@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1885411 Reviewed-by: Joel Kitching <kitching@chromium.org>
* vboot: remove BUILD_ASSERT macroJoel Kitching2019-10-231-1/+2
| | | | | | | | | | | | | | | Use _Static_assert() instead. BUG=b:124141368 TEST=make clean && make runtests BRANCH=none Change-Id: I42a18442a8bff1ab346f8ba784e9e6fc0366de9a Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1786388 Tested-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org> Reviewed-by: Julius Werner <jwerner@chromium.org>
* vboot: standardize on "keyblock" as one wordJoel Kitching2019-10-237-9/+9
| | | | | | | | | | | | | | | | Stardardize on inconsistency between "keyblock" and "key block" both in code, comments, and textual output. BUG=b:124141368, chromium:968464 TEST=make clean && make runtests BRANCH=none Change-Id: Ib8819a2426c1179286663f21f0d254f3de9d94a4 Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1786385 Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org>
* vboot: fix up some more includesJoel Kitching2019-09-231-0/+1
| | | | | | | | | | | | | | | | Should have no extra line breaks in between local includes, and should be sorted alphabetically. BUG=b:124141368 TEST=make clean && make runtests BRANCH=none Change-Id: I83c25d30d7376712857314965a7d93f57190aa3f Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1776281 Tested-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org> Reviewed-by: Julius Werner <jwerner@chromium.org>
* vboot: fix up some headers, includes, comments, spacingJoel Kitching2019-08-283-6/+5
| | | | | | | | | | | | | BUG=b:124141368 TEST=make clean && make runtests BRANCH=none Change-Id: Id97f544da845f7070555e5e8cc6e782b2d45c300 Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1758151 Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org>
* vboot: remove Boot Descriptor Block (BDB) library and utilitiesJoel Kitching2019-07-245-103/+0
| | | | | | | | | | | | | | | | Remove unused BDB code, previously created for a cancelled SoC project. BUG=b:124141368, chromium:986177 TEST=make clean && make runtests BRANCH=none Change-Id: I91faf97d9850f8afb816fa324ad9a4d9f3842888 Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1710336 Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org>
* vboot: deprecate v1 GoogleBinaryBlockHeader structJoel Kitching2019-05-171-4/+4
| | | | | | | | | | | | | | | | | Deprecate internal usage of GoogleBinaryBlockHeader struct in favour of vb2_gbb_header struct. Keep the v1 struct around until we remove references in other repos. BUG=b:124141368, chromium:954774 TEST=make clean && make runtests BRANCH=none Change-Id: I396d2e624bd5dcac9c461cc86e8175e8f7692d26 Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/1583826 Commit-Ready: Joel Kitching <kitching@chromium.org> Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Joel Kitching <kitching@chromium.org>
* Makefile: Enable more warnings for host utilities / testsJulius Werner2019-05-141-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | This patch adds a bunch of more warnings that are already enabled in coreboot and thus already enabled for firmware builds anyway (because coreboot just passes its CFLAGS through). Enabling it in the vboot Makefile means they also apply to host utilities and tests, which sounds desirable for consistency. Fix enough of the cruft and bad coding practices that accumulated over the years of not having warnings enabled to get it to build again (this includes making functions static, removing dead code, cleaning up prototypes, etc.). Also remove -fno-strict-aliasing from the x86 firmware build options, because it's not clear why it's there (coreboot isn't doing this, so presumably it's not needed). BRANCH=None BUG=None TEST=make runtests Change-Id: Ie4a42083c4770a4eca133b22725be9ba85b24184 Signed-off-by: Julius Werner <jwerner@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1598721 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
* Remove code for displaying screen from GBBDaisuke Nojiri2019-05-074-35/+2
| | | | | | | | | | | | | | | | | | | This patch removes the code displaying vboot screens using bitmap and layout data stored in GBB. bmpblk_utility, and futility support for BmpBlock is also removed. BUG=chromium:622501,chrome-os-partner:54619,b:124141368 BRANCH=none CQ-DEPEND=CL:373123 TEST=Verified screens on eve && emerge-eve chromeos-bootimage && make runtests Change-Id: I1a8dd8ff0162965e81df121d5a87ea64310a0854 Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/367882 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Joel Kitching <kitching@chromium.org>
* vboot: fix test_rwsig.sh permissions issueJoel Kitching2019-04-101-2/+5
| | | | | | | | | | | | | | | | | | | Copy hammer_dev.bin to temporary file before running `futility sign` to avoid permissions issue when running under ebuild environment. Also correct an indentation issue. BUG=chromium:950425, chromium:605348 TEST=FEATURES=test USE=cros_host ebuild vboot_reference-9999.ebuild test BRANCH=none Change-Id: I689be46d30b7bf78c6643e88a094e4f4ab311e20 Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/1557662 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
* vboot: futility test outputs should use relative directoryJoel Kitching2019-03-1316-40/+41
| | | | | | | | | | | | | | | | | | | | | | | | Not everyone uses /mnt/host/source as their development environment. If running "make runtests" from a different directory, test_show_contents.sh fails, reporting different stdout for the various futility tests that it runs. Update test_show_contents.sh to use relative test paths, and update the expected output of futility runs. Also fix consistency of quoted variables. BUG=b:124141368 TEST=/work/vboot/src/repohooks/pre-upload.py TEST=make clean && make runtests BRANCH=none Change-Id: I35fd81734b6318a506613eb4f04bb7055709feef Signed-off-by: Joel Kitching <kitching@google.com> Reviewed-on: https://chromium-review.googlesource.com/1517062 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Joel Kitching <kitching@chromium.org> Reviewed-by: Hung-Te Lin <hungte@chromium.org>
* futility: updater: Unit test for preserving sections using FMAP flagsHung-Te Lin2019-03-122-0/+18
| | | | | | | | | | | | | | | | | | | | | In CL:1495054 the updater has different logic when the firmware image has FMAP_AREA_PRESERVE in FMAP flags. This needs to be verified in unit test. The new test tries to set 010=0x08 (FMAP_AREA_PRESERVE) in RO_VPD area flag but not RW_VPD, with RO and RW VPD both being provisioned in source (from) image. The legacy path would update both while the new path will only update RO, so we can make sure the flag-based preservation is working as expected. BUG=chromium:936768 TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=None Change-Id: I07d232444344397b80344ccc9b56f8af3256e043 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1514452 Reviewed-by: Joel Kitching <kitching@chromium.org>
* futility: updater: Report key hash on TPM failureHung-Te Lin2019-03-121-2/+2
| | | | | | | | | | | | | | | | | | | | | When write protection is not enabled and updater sees TPM Anti-Rollback failure, the log will only report TPM failure (example: crbug.com/937961). This is hard to figure out if the failure was caused by re-key or other reasons. In try-rw and rw update, the updater will always check rootkey compatibility before checking TPM anti-rollback, so we should do the same thing on full update (RO+RW). With this change, the updater will report key mismatch before failing with TPM anti-rollback. BUG=chromium:937961 TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=None Change-Id: I2f035450995387b198f990467e4f416e6c7b746e Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1514007 Reviewed-by: Joel Kitching <kitching@chromium.org>
* futility: updater: Use model name as default whitelabel signaturestabilize-11895.95.Bstabilize-11895.89.Bstabilize-11895.72.Bstabilize-11895.118.Bstabilize-11895.109.Bstabilize-11895.108.Brelease-R74-11895.BHung-Te Lin2019-03-071-0/+13
| | | | | | | | | | | | | | | In Unibuild, the white label models may use (per model) PreMP key for devices without VPD 'whitelabel_tag' - this helps dogfooders and lab machines to run and update properly. BUG=b:126800200 TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=none Change-Id: I7249e3fb1a2b7ab8ed281d2aa317aee6cde8f8db Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1501614 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
* futility: updater: Improve error message when key conflictsHung-Te Lin2019-02-131-4/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Many firmware developers will try to flash a local built firmware (i.e, DEV key signed) on a MP device (with write protection enabled). The updater used to provide feedback like: ERROR: verify_keyblock: Failed verifying key block. INFO: Current (RO) firmware image has root key: ade780ffd0...732867181bae WARNING: Target (RW) image is signed by rootkey: b11d74edd2...e1135b49e7f0. ERROR: RW not signed by same RO root key >> FAILED: Firmware updater aborted. This is correctly identifying the root cause, but not helpful for developers to figure out what to do, and may be confused with the DEV re-key safety check (which needs --force). Also, when developers try to do "--mode=factory --force", the message was: updater_setup_config: Factory mode needs WP disabled. Where the 'WP' is again not clear enough. With this change, we're improving the error messages so that: - Being consistent on 'root key' instead of 'rootkey'. - Being consistent for having period for error messages, except those ended with root key hash (for easier copy-paste). - Say 'Write Protection' instead of 'WP'. - When re-keying with WP enabled, print a better hint: "To change keys in RO area, you have to first remove write protection (https://goo.gl/ces83U)." BUG=None TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=none Change-Id: Ia74d7b113766d09428a4d0897918b4f17b4afae7 Reviewed-on: https://chromium-review.googlesource.com/1465709 Commit-Ready: Hung-Te Lin <hungte@chromium.org> Tested-by: Hung-Te Lin <hungte@chromium.org> Reviewed-by: Matthew Blecker <matthewb@chromium.org>
* futility: updater: Correct HWID digest when preserving HWIDstabilize-11647.70.Bstabilize-11647.104.Brelease-R73-11647.BHung-Te Lin2019-01-161-0/+18
| | | | | | | | | | | | | | | | | | | | | | | | | | | Starting from GBB 1.2, a digest is stored in GBB and must be updated whenever the HWID string is changed. In shell script version of updater, the digest is automatically updated when we do "futility gbb -s --hwid=XXX", but in native updater implementation we only updated the HWID string and left digest unchanged, this leaves devices generating wrong PCR1 values. `cmd_gbb_utility` updates the digest by calling `update_hwid_digest` using vboot1 structure, so we should introduce a new vboot2 friendly function, `vb2_change_hwid`, which changes both HWID string and digest at same time. Note this has no impact for end user's devices with write protection enabled. Only changes dogfood units AU results. BUG=b:122248649 TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=none Change-Id: I6ad2754e6df3c9dd66d71c560a2afc26d14eae33 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1411932 Reviewed-by: Joel Kitching <kitching@chromium.org>
* futility: update: Fix 'smm_store' unit testHung-Te Lin2018-12-131-2/+2
| | | | | | | | | | | | | | In CL:1351178 the SMM store file name has been changed to 'smm_store' so we have to also change test script. BUG=b:120060878 TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility Change-Id: Idc98517cc46a848bb77335214a11fbc9303590f2 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1375494 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Reviewed-by: Joel Kitching <kitching@chromium.org>
* futility: updater: Add 'image.bin' as host image name in archiveHung-Te Lin2018-12-113-16/+29
| | | | | | | | | | | | | | | | | | The firmware updater archive is going to rename the prefix of host (AP) firmware image from 'bios' to 'image' (CL:1318712), to be more consistent with firmware package output. We need to include both old and new names in updater manifest construction. For --mode=output, we will produce both 'bios.bin' and 'image.bin'. In future there should be only 'image.bin' after migration is completed. BUG=b:65745723 TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=None Change-Id: I8b7e3bc2953b70525fb14fcf6aadaf6d1e00e4aa Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1327862
* futility: updater: Revise error message when model is not defined in manifeststabilize-11306.BHung-Te Lin2018-11-271-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | "Model '%s' is not defined in manifest." is not very easy to understand for people who are debugging devices in early stages. We should provide better instructions. For example, running with Coral updater will now show: ERROR: manifest_find_model: Cannot get model name. You are probably running an image for wrong board, or a device in early stage that 'mosys' command is not ready, or image from old (or factory) branches that Unified Build config is not updated yet for 'mosys'. Please check command 'mosys platform model', which should output one of the supported models below: unprovisioned_meep sparky orbatrix unprovisioned_fleex grabbiter bobba unprovisioned_bobba mimrock fleex meep yorp phaser360 sparky360 phaser bobba360 unprovisioned_phaser bip BUG=chromium:875551 TEST=TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=None Change-Id: Ib17fcb654d1530b94c44cf21aaa28717841f11ed Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1351171 Reviewed-by: Cheng-Han Yang <chenghan@chromium.org> Reviewed-by: Ting Shen <phoenixshen@chromium.org>
* futility: updater: Add new quirk 'allow_empty_wltag'Hung-Te Lin2018-11-241-0/+4
| | | | | | | | | | | | | | | There were devices shipped as "only device" (no key set) and then became one of the "white label" family. This is now no longer valid on newer devices but we have to support the legacy ones, for example Reks. BUG=chromium:906962 TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=None Change-Id: I437be08726ab2c46229062689bf765ac6837ca5d Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1345610 Reviewed-by: Youcheng Syu <youcheng@chromium.org>
* futility: update: Add `--host_only` argumentHung-Te Lin2018-10-231-0/+5
| | | | | | | | | | | | | | | | | | | | | | | The legacy firmware updater can update explicitly only some type of images by using `--[no]update_main`, `--[no]update_ec`, `--[no]update_pd`. Since software sync is introduced, usually it does not make sense to only update EC or PD; instead the real request is to "ignore provided EC and PD images and update only host". The new `--host_only` argument provides an easy way to ignore images in command line (`--ec_image`, `--pd_image`) and archives (`ec.bin`, `pd.bin`). BUG=chromium:875551 TEST=TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=None Change-Id: Idf403680880cd58a00867172ccec97fd60c1b826 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1295210 Reviewed-by: Randall Spangler <rspangler@chromium.org>
* futility: updater: Support --mode=output and --output_dirHung-Te Lin2018-10-221-0/+13
| | | | | | | | | | | | | | For backward compatibility, we need to support the 'output' mode in legacy firmware updater. The output must select right files according to system model, and apply all white label transform if needed. BUG=chromium:875551 TEST=TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility BRANCH=None Change-Id: Ib433647317fa97387aa4a7f8f2101b47e6ca2123 Signed-off-by: Hung-Te Lin <hungte@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1282084