From 4c749971be7b53ffcb4c9ba817631cdf6e5afece Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:16 -0400 Subject: [PATCH 01/39] gitlab-ci: Fix indentation in some stanzas In a number of our stanzas we had multi-line commands that were one space short of alignment, correct this. Reviewed-by: Stephen Warren Reviewed-by: Simon Glass Tested-by: Stephen Warren Signed-off-by: Tom Rini --- .gitlab-ci.yml | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 967abed9f2..b91b5f67af 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -65,11 +65,11 @@ build all 32bit ARM platforms: stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E arm -x aarch64 || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; - exit $ret; - fi; + ./tools/buildman/buildman -o /tmp -P -E arm -x aarch64 || ret=$?; + if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -sdeP; + exit $ret; + fi; build all 64bit ARM platforms: tags: [ 'all' ] @@ -79,33 +79,33 @@ build all 64bit ARM platforms: - . /tmp/venv/bin/activate - pip install pyelftools - ret=0; - ./tools/buildman/buildman -o /tmp -P -E aarch64 || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; - exit $ret; - fi; + ./tools/buildman/buildman -o /tmp -P -E aarch64 || ret=$?; + if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -sdeP; + exit $ret; + fi; build all PowerPC platforms: tags: [ 'all' ] stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E powerpc || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; - exit $ret; - fi; + ./tools/buildman/buildman -o /tmp -P -E powerpc || ret=$?; + if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -sdeP; + exit $ret; + fi; build all other platforms: tags: [ 'all' ] stage: world build script: - ret=0; - ./tools/buildman/buildman -o /tmp -P -E -x arm,powerpc || ret=$?; - if [[ $ret -ne 0 && $ret -ne 129 ]]; then - ./tools/buildman/buildman -o /tmp -sdeP; - exit $ret; - fi; + ./tools/buildman/buildman -o /tmp -P -E -x arm,powerpc || ret=$?; + if [[ $ret -ne 0 && $ret -ne 129 ]]; then + ./tools/buildman/buildman -o /tmp -sdeP; + exit $ret; + fi; # QA jobs for code analytics # static code analysis with cppcheck (we can add --enable=all later) From 90d3d78a1c68a771291a3d4070e66ce72e5d0ea6 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:17 -0400 Subject: [PATCH 02/39] gitlab-ci: Prepend to PATH rather than replace it Currently we set the entire PATH rather than prepend the new paths that we need to have searched. This however breaks parts of the "virtualenv" that was have set up and need to use as that also will be modifying PATH. To fix this, prepend our new locations instead. Reviewed-by: Stephen Warren Reviewed-by: Simon Glass Tested-by: Stephen Warren Signed-off-by: Tom Rini --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b91b5f67af..ab294997e4 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -48,7 +48,7 @@ stages: # "-k something" even when $TEST_PY_TEST_SPEC doesnt need a custom # value. - export UBOOT_TRAVIS_BUILD_DIR=/tmp/.bm-work/${TEST_PY_BD}; - export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:/usr/bin:/bin; + export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:${PATH}; export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci; if [[ "${TEST_PY_BD}" != "" ]]; then ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} From 79883ef7dcc20cb329ffc307c17d77baac62c2bc Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:18 -0400 Subject: [PATCH 03/39] test/py: Split mark to multiple lines We inconsistently note multiple dependencies today in our tests, sometimes with a single line that declares multiple and sometimes multiple single lines. Current pytest seems to fail on the single line format so change to multiple declarations. Reviewed-by: Stephen Warren Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/tests/test_android/test_avb.py | 9 ++++++--- test/py/tests/test_mmc_wr.py | 4 +++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/test/py/tests/test_android/test_avb.py b/test/py/tests/test_android/test_avb.py index 8132423435..20ccaf6712 100644 --- a/test/py/tests/test_android/test_avb.py +++ b/test/py/tests/test_android/test_avb.py @@ -23,7 +23,8 @@ mmc_dev = 1 temp_addr = 0x90000000 temp_addr2 = 0x90002000 -@pytest.mark.buildconfigspec('cmd_avb', 'cmd_mmc') +@pytest.mark.buildconfigspec('cmd_avb') +@pytest.mark.buildconfigspec('cmd_mmc') def test_avb_verify(u_boot_console): """Run AVB 2.0 boot verification chain with avb subset of commands """ @@ -36,7 +37,8 @@ def test_avb_verify(u_boot_console): assert response.find(success_str) -@pytest.mark.buildconfigspec('cmd_avb', 'cmd_mmc') +@pytest.mark.buildconfigspec('cmd_avb') +@pytest.mark.buildconfigspec('cmd_mmc') def test_avb_mmc_uuid(u_boot_console): """Check if 'avb get_uuid' works, compare results with 'part list mmc 1' output @@ -93,7 +95,8 @@ def test_avb_is_unlocked(u_boot_console): assert response == 'Unlocked = 1' -@pytest.mark.buildconfigspec('cmd_avb', 'cmd_mmc') +@pytest.mark.buildconfigspec('cmd_avb') +@pytest.mark.buildconfigspec('cmd_mmc') def test_avb_mmc_read(u_boot_console): """Test mmc read operation """ diff --git a/test/py/tests/test_mmc_wr.py b/test/py/tests/test_mmc_wr.py index 8b18781eac..ab4c579744 100644 --- a/test/py/tests/test_mmc_wr.py +++ b/test/py/tests/test_mmc_wr.py @@ -35,7 +35,9 @@ env__mmc_wr_configs = ( """ -@pytest.mark.buildconfigspec('cmd_mmc','cmd_memory', 'cmd_random') +@pytest.mark.buildconfigspec('cmd_mmc') +@pytest.mark.buildconfigspec('cmd_memory') +@pytest.mark.buildconfigspec('cmd_random') def test_mmc_wr(u_boot_console, env__mmc_wr_config): """Test the "mmc write" command. From 3c941e048c824b94ae09fd9e1f91116f55ce0f1f Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Thu, 24 Oct 2019 11:59:19 -0400 Subject: [PATCH 04/39] test/py: Fix pytest4 deprecation warnings Fix the following spit from pytest: u-boot/test/py/conftest.py:438: RemovedInPytest4Warning: MarkInfo objects are deprecated as they contain merged marks which are hard to deal with correctly. Please use node.get_closest_marker(name) or node.iter_markers(name). Docs: https://docs.pytest.org/en/latest/mark.html#updating-code for board in mark.args: In both cases, the later suggestion is applicable. Reviewed-by: Stephen Warren Reviewed-by: Simon Glass Signed-off-by: Marek Vasut Cc: Igor Opaniuk [trini: Update for current file with a few more cases, un-pin pytest in CI] Tested-by: Simon Glass [on sandbox] Tested-by: Stephen Warren Signed-off-by: Tom Rini --- .gitlab-ci.yml | 2 +- .travis.yml | 2 +- test/py/conftest.py | 30 ++++++++++++------------------ 3 files changed, 14 insertions(+), 20 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ab294997e4..5a34321570 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -20,7 +20,7 @@ stages: - ln -s travis-ci /tmp/uboot-test-hooks/py/`hostname` - virtualenv /tmp/venv - . /tmp/venv/bin/activate - - pip install pytest==2.8.7 + - pip install pytest - pip install python-subunit - pip install coverage - grub-mkimage --prefix="" -o ~/grub_x86.efi -O i386-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd diff --git a/.travis.yml b/.travis.yml index 3aaed93346..c50270de5f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -49,7 +49,7 @@ install: - cat ~/.buildman - virtualenv /tmp/venv - . /tmp/venv/bin/activate - - pip install pytest==2.8.7 + - pip install pytest - pip install python-subunit - pip install pyelftools - grub-mkimage --prefix="" -o ~/grub_x86.efi -O i386-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd diff --git a/test/py/conftest.py b/test/py/conftest.py index 00d8ef8ba9..30c898b40a 100644 --- a/test/py/conftest.py +++ b/test/py/conftest.py @@ -431,11 +431,9 @@ def setup_boardspec(item): Nothing. """ - mark = item.get_marker('boardspec') - if not mark: - return required_boards = [] - for board in mark.args: + for boards in item.iter_markers('boardspec'): + board = boards.args[0] if board.startswith('!'): if ubconfig.board_type == board[1:]: pytest.skip('board "%s" not supported' % ubconfig.board_type) @@ -459,16 +457,14 @@ def setup_buildconfigspec(item): Nothing. """ - mark = item.get_marker('buildconfigspec') - if mark: - for option in mark.args: - if not ubconfig.buildconfig.get('config_' + option.lower(), None): - pytest.skip('.config feature "%s" not enabled' % option.lower()) - notmark = item.get_marker('notbuildconfigspec') - if notmark: - for option in notmark.args: - if ubconfig.buildconfig.get('config_' + option.lower(), None): - pytest.skip('.config feature "%s" enabled' % option.lower()) + for options in item.iter_markers('buildconfigspec'): + option = options.args[0] + if not ubconfig.buildconfig.get('config_' + option.lower(), None): + pytest.skip('.config feature "%s" not enabled' % option.lower()) + for option in item.iter_markers('notbuildconfigspec'): + option = options.args[0] + if ubconfig.buildconfig.get('config_' + option.lower(), None): + pytest.skip('.config feature "%s" enabled' % option.lower()) def tool_is_in_path(tool): for path in os.environ["PATH"].split(os.pathsep): @@ -491,10 +487,8 @@ def setup_requiredtool(item): Nothing. """ - mark = item.get_marker('requiredtool') - if not mark: - return - for tool in mark.args: + for tools in item.iter_markers('requiredtool'): + tool = tools.args[0] if not tool_is_in_path(tool): pytest.skip('tool "%s" not in $PATH' % tool) From fe1193e254faccc4e6629f1cfbc99d5ceb34428a Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:20 -0400 Subject: [PATCH 05/39] test/py: Automated conversion to Python 3 Use the 2to3 tool to perform numerous automatic conversions from Python 2 syntax to Python 3. Also fix whitespace problems that Python 3 catches that Python 2 did not. Reviewed-by: Stephen Warren Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/conftest.py | 9 ++--- test/py/multiplexed_log.py | 4 +- test/py/test.py | 4 +- test/py/tests/test_fit.py | 2 - test/py/tests/test_fpga.py | 40 +++++++++---------- test/py/tests/test_fs/conftest.py | 4 +- test/py/tests/test_log.py | 8 ++-- test/py/tests/test_mmc_wr.py | 66 +++++++++++++++---------------- 8 files changed, 64 insertions(+), 73 deletions(-) diff --git a/test/py/conftest.py b/test/py/conftest.py index 30c898b40a..5c19af1d50 100644 --- a/test/py/conftest.py +++ b/test/py/conftest.py @@ -19,13 +19,10 @@ import os.path import pytest from _pytest.runner import runtestprotocol import re -import StringIO +import io import sys -try: - import configparser -except: - import ConfigParser as configparser +import configparser # Globals: The HTML log file, and the connection to the U-Boot console. log = None @@ -169,7 +166,7 @@ def pytest_configure(config): with open(dot_config, 'rt') as f: ini_str = '[root]\n' + f.read() - ini_sio = StringIO.StringIO(ini_str) + ini_sio = io.StringIO(ini_str) parser = configparser.RawConfigParser() parser.readfp(ini_sio) ubconfig.buildconfig.update(parser.items('root')) diff --git a/test/py/multiplexed_log.py b/test/py/multiplexed_log.py index 637a3bd257..de0aacc659 100644 --- a/test/py/multiplexed_log.py +++ b/test/py/multiplexed_log.py @@ -5,8 +5,8 @@ # Generate an HTML-formatted log file containing multiple streams of data, # each represented in a well-delineated/-structured fashion. -import cgi import datetime +import html import os.path import shutil import subprocess @@ -334,7 +334,7 @@ $(document).ready(function () { data = data.replace(chr(13), '') data = ''.join((ord(c) in self._nonprint) and ('%%%02x' % ord(c)) or c for c in data) - data = cgi.escape(data) + data = html.escape(data) return data def _terminate_stream(self): diff --git a/test/py/test.py b/test/py/test.py index a5140945d4..0ce1838833 100755 --- a/test/py/test.py +++ b/test/py/test.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python2 +#!/usr/bin/env python3 # SPDX-License-Identifier: GPL-2.0 # Copyright (c) 2015 Stephen Warren @@ -7,8 +7,6 @@ # Wrapper script to invoke pytest with the directory name that contains the # U-Boot tests. -from __future__ import print_function - import os import os.path import sys diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py index e3210ed43f..4922b9dcc6 100755 --- a/test/py/tests/test_fit.py +++ b/test/py/tests/test_fit.py @@ -3,8 +3,6 @@ # # Sanity check of the FIT handling in U-Boot -from __future__ import print_function - import os import pytest import struct diff --git a/test/py/tests/test_fpga.py b/test/py/tests/test_fpga.py index e3bb7b41c7..ca7ef8ea40 100644 --- a/test/py/tests/test_fpga.py +++ b/test/py/tests/test_fpga.py @@ -175,29 +175,29 @@ def test_fpga_load_fail(u_boot_console): f, dev, addr, bit, bit_size = load_file_from_var(u_boot_console, 'bitstream_load') for cmd in ['dump', 'load', 'loadb']: - # missing dev parameter - expected = 'fpga: incorrect parameters passed' - output = u_boot_console.run_command('fpga %s %x $filesize' % (cmd, addr)) - #assert expected in output - assert expected_usage in output + # missing dev parameter + expected = 'fpga: incorrect parameters passed' + output = u_boot_console.run_command('fpga %s %x $filesize' % (cmd, addr)) + #assert expected in output + assert expected_usage in output - # more parameters - 0 at the end - expected = 'fpga: more parameters passed' - output = u_boot_console.run_command('fpga %s %x %x $filesize 0' % (cmd, dev, addr)) - #assert expected in output - assert expected_usage in output + # more parameters - 0 at the end + expected = 'fpga: more parameters passed' + output = u_boot_console.run_command('fpga %s %x %x $filesize 0' % (cmd, dev, addr)) + #assert expected in output + assert expected_usage in output - # 0 address - expected = 'fpga: zero fpga_data address' - output = u_boot_console.run_command('fpga %s %x 0 $filesize' % (cmd, dev)) - #assert expected in output - assert expected_usage in output + # 0 address + expected = 'fpga: zero fpga_data address' + output = u_boot_console.run_command('fpga %s %x 0 $filesize' % (cmd, dev)) + #assert expected in output + assert expected_usage in output - # 0 filesize - expected = 'fpga: zero size' - output = u_boot_console.run_command('fpga %s %x %x 0' % (cmd, dev, addr)) - #assert expected in output - assert expected_usage in output + # 0 filesize + expected = 'fpga: zero size' + output = u_boot_console.run_command('fpga %s %x %x 0' % (cmd, dev, addr)) + #assert expected in output + assert expected_usage in output @pytest.mark.buildconfigspec('cmd_fpga') @pytest.mark.buildconfigspec('cmd_echo') diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py index 9324657d21..354d17672f 100644 --- a/test/py/tests/test_fs/conftest.py +++ b/test/py/tests/test_fs/conftest.py @@ -508,8 +508,8 @@ def fs_obj_unlink(request, u_boot_config): # Test Case 2 check_call('mkdir %s/dir2' % mount_dir, shell=True) - for i in range(0, 20): - check_call('mkdir %s/dir2/0123456789abcdef%02x' + for i in range(0, 20): + check_call('mkdir %s/dir2/0123456789abcdef%02x' % (mount_dir, i), shell=True) # Test Case 4 diff --git a/test/py/tests/test_log.py b/test/py/tests/test_log.py index cb183444c6..75325fad61 100644 --- a/test/py/tests/test_log.py +++ b/test/py/tests/test_log.py @@ -27,9 +27,9 @@ def test_log(u_boot_console): """ for i in range(max_level): if mask & 1: - assert 'log_run() log %d' % i == lines.next() + assert 'log_run() log %d' % i == next(lines) if mask & 3: - assert 'func() _log %d' % i == lines.next() + assert 'func() _log %d' % i == next(lines) def run_test(testnum): """Run a particular test number (the 'log test' command) @@ -43,7 +43,7 @@ def test_log(u_boot_console): output = u_boot_console.run_command('log test %d' % testnum) split = output.replace('\r', '').splitlines() lines = iter(split) - assert 'test %d' % testnum == lines.next() + assert 'test %d' % testnum == next(lines) return lines def test0(): @@ -88,7 +88,7 @@ def test_log(u_boot_console): def test10(): lines = run_test(10) for i in range(7): - assert 'log_test() level %d' % i == lines.next() + assert 'log_test() level %d' % i == next(lines) # TODO(sjg@chromium.org): Consider structuring this as separate tests cons = u_boot_console diff --git a/test/py/tests/test_mmc_wr.py b/test/py/tests/test_mmc_wr.py index ab4c579744..05e5c1ee85 100644 --- a/test/py/tests/test_mmc_wr.py +++ b/test/py/tests/test_mmc_wr.py @@ -67,41 +67,39 @@ def test_mmc_wr(u_boot_console, env__mmc_wr_config): for i in range(test_iterations): - # Generate random data - cmd = 'random %s %x' % (src_addr, count_bytes) - response = u_boot_console.run_command(cmd) - good_response = '%d bytes filled with random data' % (count_bytes) - assert good_response in response + # Generate random data + cmd = 'random %s %x' % (src_addr, count_bytes) + response = u_boot_console.run_command(cmd) + good_response = '%d bytes filled with random data' % (count_bytes) + assert good_response in response - # Select MMC device - cmd = 'mmc dev %d' % devid - if is_emmc: - cmd += ' %d' % partid - response = u_boot_console.run_command(cmd) - assert 'no card present' not in response - if is_emmc: - partid_response = "(part %d)" % partid - else: - partid_response = "" - good_response = 'mmc%d%s is current device' % (devid, partid_response) - assert good_response in response + # Select MMC device + cmd = 'mmc dev %d' % devid + if is_emmc: + cmd += ' %d' % partid + response = u_boot_console.run_command(cmd) + assert 'no card present' not in response + if is_emmc: + partid_response = "(part %d)" % partid + else: + partid_response = "" + good_response = 'mmc%d%s is current device' % (devid, partid_response) + assert good_response in response - # Write data - cmd = 'mmc write %s %x %x' % (src_addr, sector, count_sectors) - response = u_boot_console.run_command(cmd) - good_response = 'MMC write: dev # %d, block # %d, count %d ... %d blocks written: OK' % ( - devid, sector, count_sectors, count_sectors) - assert good_response in response + # Write data + cmd = 'mmc write %s %x %x' % (src_addr, sector, count_sectors) + response = u_boot_console.run_command(cmd) + good_response = 'MMC write: dev # %d, block # %d, count %d ... %d blocks written: OK' % (devid, sector, count_sectors, count_sectors) + assert good_response in response - # Read data - cmd = 'mmc read %s %x %x' % (dst_addr, sector, count_sectors) - response = u_boot_console.run_command(cmd) - good_response = 'MMC read: dev # %d, block # %d, count %d ... %d blocks read: OK' % ( - devid, sector, count_sectors, count_sectors) - assert good_response in response + # Read data + cmd = 'mmc read %s %x %x' % (dst_addr, sector, count_sectors) + response = u_boot_console.run_command(cmd) + good_response = 'MMC read: dev # %d, block # %d, count %d ... %d blocks read: OK' % (devid, sector, count_sectors, count_sectors) + assert good_response in response - # Compare src and dst data - cmd = 'cmp.b %s %s %x' % (src_addr, dst_addr, count_bytes) - response = u_boot_console.run_command(cmd) - good_response = 'Total of %d byte(s) were the same' % (count_bytes) - assert good_response in response + # Compare src and dst data + cmd = 'cmp.b %s %s %x' % (src_addr, dst_addr, count_bytes) + response = u_boot_console.run_command(cmd) + good_response = 'Total of %d byte(s) were the same' % (count_bytes) + assert good_response in response From fd31fc172c626bdac2afd8af9f8482bfc21a5501 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:21 -0400 Subject: [PATCH 06/39] test/py: Manual python3 fixes - Modern pytest is more visible in telling us about parameters that we had not described, so describe a few more. - ConfigParser.readfp(...) is now configparser.read_file(...) - As part of the "strings vs bytes" conversions in Python 3, we use the default encoding/decoding of utf-8 but in some places tell Python to replace problematic conversions rather than throw a fatal error. - Fix a typo noticed while doing the above ("tot he" -> "to the"). - As suggested by Stephen, re-alphabetize the import list - Per Heinrich, replace how we write contents in test_fit.py Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/conftest.py | 9 ++++----- test/py/multiplexed_log.py | 11 ++++++++--- test/py/pytest.ini | 3 +++ test/py/tests/test_fit.py | 4 ++-- test/py/u_boot_spawn.py | 4 ++-- 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/test/py/conftest.py b/test/py/conftest.py index 5c19af1d50..bffee6b8a3 100644 --- a/test/py/conftest.py +++ b/test/py/conftest.py @@ -13,17 +13,16 @@ # - Implementing custom pytest markers. import atexit +import configparser import errno +import io import os import os.path import pytest -from _pytest.runner import runtestprotocol import re -import io +from _pytest.runner import runtestprotocol import sys -import configparser - # Globals: The HTML log file, and the connection to the U-Boot console. log = None console = None @@ -168,7 +167,7 @@ def pytest_configure(config): ini_str = '[root]\n' + f.read() ini_sio = io.StringIO(ini_str) parser = configparser.RawConfigParser() - parser.readfp(ini_sio) + parser.read_file(ini_sio) ubconfig.buildconfig.update(parser.items('root')) ubconfig.test_py_dir = test_py_dir diff --git a/test/py/multiplexed_log.py b/test/py/multiplexed_log.py index de0aacc659..545a774302 100644 --- a/test/py/multiplexed_log.py +++ b/test/py/multiplexed_log.py @@ -51,7 +51,7 @@ class LogfileStream(object): """Write data to the log stream. Args: - data: The data to write tot he file. + data: The data to write to the file. implicit: Boolean indicating whether data actually appeared in the stream, or was implicitly generated. A valid use-case is to repeat a shell prompt at the start of each separate log @@ -64,7 +64,8 @@ class LogfileStream(object): self.logfile.write(self, data, implicit) if self.chained_file: - self.chained_file.write(data) + # Chained file is console, convert things a little + self.chained_file.write((data.encode('ascii', 'replace')).decode()) def flush(self): """Flush the log stream, to ensure correct log interleaving. @@ -136,6 +137,10 @@ class RunAndLog(object): p = subprocess.Popen(cmd, cwd=cwd, stdin=None, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) (stdout, stderr) = p.communicate() + if stdout is not None: + stdout = stdout.decode('utf-8') + if stderr is not None: + stderr = stderr.decode('utf-8') output = '' if stdout: if stderr: @@ -215,7 +220,7 @@ class Logfile(object): Nothing. """ - self.f = open(fn, 'wt') + self.f = open(fn, 'wt', encoding='utf-8') self.last_stream = None self.blocks = [] self.cur_evt = 1 diff --git a/test/py/pytest.ini b/test/py/pytest.ini index 7e400682bf..e93d010f1f 100644 --- a/test/py/pytest.ini +++ b/test/py/pytest.ini @@ -8,3 +8,6 @@ markers = boardspec: U-Boot: Describes the set of boards a test can/can't run on. buildconfigspec: U-Boot: Describes Kconfig/config-header constraints. + notbuildconfigspec: U-Boot: Describes required disabled Kconfig options. + requiredtool: U-Boot: Required host tools for a test. + slow: U-Boot: Specific test will run slowly. diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py index 4922b9dcc6..356d9a20f2 100755 --- a/test/py/tests/test_fit.py +++ b/test/py/tests/test_fit.py @@ -153,7 +153,7 @@ def test_fit(u_boot_console): src = make_fname('u-boot.dts') dtb = make_fname('u-boot.dtb') with open(src, 'w') as fd: - print(base_fdt, file=fd) + fd.write(base_fdt) util.run_and_log(cons, ['dtc', src, '-O', 'dtb', '-o', dtb]) return dtb @@ -186,7 +186,7 @@ def test_fit(u_boot_console): its = make_its(params) util.run_and_log(cons, [mkimage, '-f', its, fit]) with open(make_fname('u-boot.dts'), 'w') as fd: - print(base_fdt, file=fd) + fd.write(base_fdt) return fit def make_kernel(filename, text): diff --git a/test/py/u_boot_spawn.py b/test/py/u_boot_spawn.py index b011a3e3da..4f898cdefe 100644 --- a/test/py/u_boot_spawn.py +++ b/test/py/u_boot_spawn.py @@ -113,7 +113,7 @@ class Spawn(object): Nothing. """ - os.write(self.fd, data) + os.write(self.fd, data.encode(errors='replace')) def expect(self, patterns): """Wait for the sub-process to emit specific data. @@ -171,7 +171,7 @@ class Spawn(object): events = self.poll.poll(poll_maxwait) if not events: raise Timeout() - c = os.read(self.fd, 1024) + c = os.read(self.fd, 1024).decode(errors='replace') if not c: raise EOFError() if self.logfile_read: From 8060209a9278593519f73ba763e67f7aa219a732 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:22 -0400 Subject: [PATCH 07/39] test/py: test_ut.py: Ensure we use bytes In the case of some unit tests we are working with providing a fake flash device that we have written some text strings in to. In this case we want to tell Python to encode things to bytes for us. Reviewed-by: Stephen Warren Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/tests/test_ut.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py index 62037d2c45..6c7b8dd2b3 100644 --- a/test/py/tests/test_ut.py +++ b/test/py/tests/test_ut.py @@ -10,14 +10,14 @@ def test_ut_dm_init(u_boot_console): fn = u_boot_console.config.source_dir + '/testflash.bin' if not os.path.exists(fn): - data = 'this is a test' - data += '\x00' * ((4 * 1024 * 1024) - len(data)) + data = b'this is a test' + data += b'\x00' * ((4 * 1024 * 1024) - len(data)) with open(fn, 'wb') as fh: fh.write(data) fn = u_boot_console.config.source_dir + '/spi.bin' if not os.path.exists(fn): - data = '\x00' * (2 * 1024 * 1024) + data = b'\x00' * (2 * 1024 * 1024) with open(fn, 'wb') as fh: fh.write(data) From 1813ace6fcc46097d1709a775e72948fcbf52fee Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:23 -0400 Subject: [PATCH 08/39] test/py: test_efi_selftest.py: Updates for python 3 support - In python 3 you must use raw strings for regex as other forms are deprecated and would require further changes to the pattern here. In one case this lets us have a simpler match pattern. - As strings are now Unicode our complex tests (Euro symbol, SHIFT+ALT+FN 5) we need to declare that as a bytes string and then decode it for use. Cc: Heinrich Schuchardt Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/tests/test_efi_selftest.py | 36 +++++++++++++++--------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py index d5430f9c12..ca01542088 100644 --- a/test/py/tests/test_efi_selftest.py +++ b/test/py/tests/test_efi_selftest.py @@ -59,7 +59,7 @@ def test_efi_selftest_text_input(u_boot_console): u_boot_console.run_command(cmd='setenv efi_selftest text input') output = u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False) - m = u_boot_console.p.expect(['To terminate type \'x\'']) + m = u_boot_console.p.expect([r'To terminate type \'x\'']) if m != 0: raise Exception('No prompt for \'text input\' test') u_boot_console.drain_console() @@ -68,7 +68,7 @@ def test_efi_selftest_text_input(u_boot_console): u_boot_console.run_command(cmd=chr(4), wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 4 \(unknown\), scan code 0 \(Null\)']) + [r'Unicode char 4 \(unknown\), scan code 0 \(Null\)']) if m != 0: raise Exception('EOT failed in \'text input\' test') u_boot_console.drain_console() @@ -76,7 +76,7 @@ def test_efi_selftest_text_input(u_boot_console): u_boot_console.run_command(cmd=chr(8), wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 8 \(BS\), scan code 0 \(Null\)']) + [r'Unicode char 8 \(BS\), scan code 0 \(Null\)']) if m != 0: raise Exception('BS failed in \'text input\' test') u_boot_console.drain_console() @@ -84,7 +84,7 @@ def test_efi_selftest_text_input(u_boot_console): u_boot_console.run_command(cmd=chr(9), wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 9 \(TAB\), scan code 0 \(Null\)']) + [r'Unicode char 9 \(TAB\), scan code 0 \(Null\)']) if m != 0: raise Exception('BS failed in \'text input\' test') u_boot_console.drain_console() @@ -92,7 +92,7 @@ def test_efi_selftest_text_input(u_boot_console): u_boot_console.run_command(cmd='a', wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 97 \(\'a\'\), scan code 0 \(Null\)']) + [r'Unicode char 97 \(\'a\'\), scan code 0 \(Null\)']) if m != 0: raise Exception('\'a\' failed in \'text input\' test') u_boot_console.drain_console() @@ -100,14 +100,14 @@ def test_efi_selftest_text_input(u_boot_console): u_boot_console.run_command(cmd=chr(27) + '[A', wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 0 \(Null\), scan code 1 \(Up\)']) + [r'Unicode char 0 \(Null\), scan code 1 \(Up\)']) if m != 0: raise Exception('UP failed in \'text input\' test') u_boot_console.drain_console() # Euro sign - u_boot_console.run_command(cmd='\xe2\x82\xac', wait_for_echo=False, + u_boot_console.run_command(cmd=b'\xe2\x82\xac'.decode(), wait_for_echo=False, send_nl=False, wait_for_prompt=False) - m = u_boot_console.p.expect(['Unicode char 8364 \(\'']) + m = u_boot_console.p.expect([r'Unicode char 8364 \(\'']) if m != 0: raise Exception('Euro sign failed in \'text input\' test') u_boot_console.drain_console() @@ -129,7 +129,7 @@ def test_efi_selftest_text_input_ex(u_boot_console): u_boot_console.run_command(cmd='setenv efi_selftest extended text input') output = u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False) - m = u_boot_console.p.expect(['To terminate type \'CTRL\+x\'']) + m = u_boot_console.p.expect([r'To terminate type \'CTRL\+x\'']) if m != 0: raise Exception('No prompt for \'text input\' test') u_boot_console.drain_console() @@ -138,7 +138,7 @@ def test_efi_selftest_text_input_ex(u_boot_console): u_boot_console.run_command(cmd=chr(4), wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 100 \\(\'d\'\\), scan code 0 \\(CTRL\\+Null\\)']) + [r'Unicode char 100 \(\'d\'\), scan code 0 \(CTRL\+Null\)']) if m != 0: raise Exception('EOT failed in \'text input\' test') u_boot_console.drain_console() @@ -146,7 +146,7 @@ def test_efi_selftest_text_input_ex(u_boot_console): u_boot_console.run_command(cmd=chr(8), wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 8 \(BS\), scan code 0 \(\+Null\)']) + [r'Unicode char 8 \(BS\), scan code 0 \(\+Null\)']) if m != 0: raise Exception('BS failed in \'text input\' test') u_boot_console.drain_console() @@ -154,7 +154,7 @@ def test_efi_selftest_text_input_ex(u_boot_console): u_boot_console.run_command(cmd=chr(9), wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 9 \(TAB\), scan code 0 \(\+Null\)']) + [r'Unicode char 9 \(TAB\), scan code 0 \(\+Null\)']) if m != 0: raise Exception('TAB failed in \'text input\' test') u_boot_console.drain_console() @@ -162,7 +162,7 @@ def test_efi_selftest_text_input_ex(u_boot_console): u_boot_console.run_command(cmd='a', wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 97 \(\'a\'\), scan code 0 \(Null\)']) + [r'Unicode char 97 \(\'a\'\), scan code 0 \(Null\)']) if m != 0: raise Exception('\'a\' failed in \'text input\' test') u_boot_console.drain_console() @@ -170,23 +170,23 @@ def test_efi_selftest_text_input_ex(u_boot_console): u_boot_console.run_command(cmd=chr(27) + '[A', wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 0 \(Null\), scan code 1 \(\+Up\)']) + [r'Unicode char 0 \(Null\), scan code 1 \(\+Up\)']) if m != 0: raise Exception('UP failed in \'text input\' test') u_boot_console.drain_console() # Euro sign - u_boot_console.run_command(cmd='\xe2\x82\xac', wait_for_echo=False, + u_boot_console.run_command(cmd=b'\xe2\x82\xac'.decode(), wait_for_echo=False, send_nl=False, wait_for_prompt=False) - m = u_boot_console.p.expect(['Unicode char 8364 \(\'']) + m = u_boot_console.p.expect([r'Unicode char 8364 \(\'']) if m != 0: raise Exception('Euro sign failed in \'text input\' test') u_boot_console.drain_console() # SHIFT+ALT+FN 5 - u_boot_console.run_command(cmd='\x1b\x5b\x31\x35\x3b\x34\x7e', + u_boot_console.run_command(cmd=b'\x1b\x5b\x31\x35\x3b\x34\x7e'.decode(), wait_for_echo=False, send_nl=False, wait_for_prompt=False) m = u_boot_console.p.expect( - ['Unicode char 0 \(Null\), scan code 15 \(SHIFT\+ALT\+FN 5\)']) + [r'Unicode char 0 \(Null\), scan code 15 \(SHIFT\+ALT\+FN 5\)']) if m != 0: raise Exception('SHIFT+ALT+FN 5 failed in \'text input\' test') u_boot_console.drain_console() From d2b5240c9a0303e2d2ed3cd540d7e32b6f198664 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:24 -0400 Subject: [PATCH 09/39] test/py: Update test_fs to decode check_output calls The check_output function from the subprocess Python module by default returns data as encoded bytes and leaves decoding to the application. Given our uses of the call, it makes the most sense to immediately decode the results. Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/tests/test_fs/conftest.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py index 354d17672f..1949f91619 100644 --- a/test/py/tests/test_fs/conftest.py +++ b/test/py/tests/test_fs/conftest.py @@ -300,38 +300,38 @@ def fs_obj_basic(request, u_boot_config): # Generate the md5sums of reads that we will test against small file out = check_output( 'dd if=%s bs=1M skip=0 count=1 2> /dev/null | md5sum' - % small_file, shell=True) + % small_file, shell=True).decode() md5val = [ out.split()[0] ] # Generate the md5sums of reads that we will test against big file # One from beginning of file. out = check_output( 'dd if=%s bs=1M skip=0 count=1 2> /dev/null | md5sum' - % big_file, shell=True) + % big_file, shell=True).decode() md5val.append(out.split()[0]) # One from end of file. out = check_output( 'dd if=%s bs=1M skip=2499 count=1 2> /dev/null | md5sum' - % big_file, shell=True) + % big_file, shell=True).decode() md5val.append(out.split()[0]) # One from the last 1MB chunk of 2GB out = check_output( 'dd if=%s bs=1M skip=2047 count=1 2> /dev/null | md5sum' - % big_file, shell=True) + % big_file, shell=True).decode() md5val.append(out.split()[0]) # One from the start 1MB chunk from 2GB out = check_output( 'dd if=%s bs=1M skip=2048 count=1 2> /dev/null | md5sum' - % big_file, shell=True) + % big_file, shell=True).decode() md5val.append(out.split()[0]) # One 1MB chunk crossing the 2GB boundary out = check_output( 'dd if=%s bs=512K skip=4095 count=2 2> /dev/null | md5sum' - % big_file, shell=True) + % big_file, shell=True).decode() md5val.append(out.split()[0]) umount_fs(mount_dir) @@ -390,7 +390,7 @@ def fs_obj_ext(request, u_boot_config): % min_file, shell=True) out = check_output( 'dd if=%s bs=1K 2> /dev/null | md5sum' - % min_file, shell=True) + % min_file, shell=True).decode() md5val = [ out.split()[0] ] # Calculate md5sum of Test Case 4 @@ -399,7 +399,7 @@ def fs_obj_ext(request, u_boot_config): check_call('dd if=%s of=%s bs=1K seek=5 count=20' % (min_file, tmp_file), shell=True) out = check_output('dd if=%s bs=1K 2> /dev/null | md5sum' - % tmp_file, shell=True) + % tmp_file, shell=True).decode() md5val.append(out.split()[0]) # Calculate md5sum of Test Case 5 @@ -408,7 +408,7 @@ def fs_obj_ext(request, u_boot_config): check_call('dd if=%s of=%s bs=1K seek=5 count=5' % (min_file, tmp_file), shell=True) out = check_output('dd if=%s bs=1K 2> /dev/null | md5sum' - % tmp_file, shell=True) + % tmp_file, shell=True).decode() md5val.append(out.split()[0]) # Calculate md5sum of Test Case 7 @@ -417,7 +417,7 @@ def fs_obj_ext(request, u_boot_config): check_call('dd if=%s of=%s bs=1K seek=20 count=20' % (min_file, tmp_file), shell=True) out = check_output('dd if=%s bs=1K 2> /dev/null | md5sum' - % tmp_file, shell=True) + % tmp_file, shell=True).decode() md5val.append(out.split()[0]) check_call('rm %s' % tmp_file, shell=True) @@ -582,11 +582,11 @@ def fs_obj_symlink(request, u_boot_config): # Generate the md5sums of reads that we will test against small file out = check_output( 'dd if=%s bs=1M skip=0 count=1 2> /dev/null | md5sum' - % small_file, shell=True) + % small_file, shell=True).decode() md5val = [out.split()[0]] out = check_output( 'dd if=%s bs=10M skip=0 count=1 2> /dev/null | md5sum' - % medium_file, shell=True) + % medium_file, shell=True).decode() md5val.extend([out.split()[0]]) umount_fs(mount_dir) From 8add4fa4177bf697ad892497fe65259dad642366 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:25 -0400 Subject: [PATCH 10/39] test/py: Rework test.py to be a different kind of wrapper Now that we have moved to being based on pytest for python3 we need to make our test.py wrapper more robust in terms of only calling python3 rather than possibly finding and using python2. To do this, change from execvp()'ing pytest to invoking the package itself via python. In the event that pytest is unavailable we still get a user-friendly error: pkg_resources.DistributionNotFound: The 'pytest' distribution was not found and is required by the application Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/test.py | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/test/py/test.py b/test/py/test.py index 0ce1838833..bee88d96bc 100755 --- a/test/py/test.py +++ b/test/py/test.py @@ -10,23 +10,11 @@ import os import os.path import sys - -# Get rid of argv[0] -sys.argv.pop(0) +from pkg_resources import load_entry_point # argv; py.test test_directory_name user-supplied-arguments -args = ['py.test', os.path.dirname(__file__) + '/tests'] +args = [os.path.dirname(__file__) + '/tests'] args.extend(sys.argv) -try: - os.execvp('py.test', args) -except: - # Log full details of any exception for detailed analysis - import traceback - traceback.print_exc() - # Hint to the user that they likely simply haven't installed the required - # dependencies. - print(''' -exec(py.test) failed; perhaps you are missing some dependencies? -See test/py/README.md for the list.''', file=sys.stderr) - sys.exit(1) +if __name__ == '__main__': + sys.exit(load_entry_point('pytest', 'console_scripts', 'pytest')(args)) From ddaa8bed3dea59201392e9f516a9f2dbb12654d9 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:26 -0400 Subject: [PATCH 11/39] test/py: Update docs, add requirements.txt for pip To be more closely aligned with Python community best practices, we need to better document our usage of pip and make use of a requirements.txt file that shows the versions of the tools that we are using. This will aide in ensuring reproducibility of our tests as well. Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/README.md | 45 ++++++++++++++++++++++------------------ test/py/requirements.txt | 22 ++++++++++++++++++++ 2 files changed, 47 insertions(+), 20 deletions(-) create mode 100644 test/py/requirements.txt diff --git a/test/py/README.md b/test/py/README.md index 2156661d6c..3cbe01b73e 100644 --- a/test/py/README.md +++ b/test/py/README.md @@ -21,19 +21,26 @@ involves executing some binary and interacting with its stdin/stdout. You will need to implement various "hook" scripts that are called by the test suite at the appropriate time. -On Debian or Debian-like distributions, the following packages are required. -Some packages are required to execute any test, and others only for specific -tests. Similar package names should exist in other distributions. +In order to run the testsuite at a minimum we require that both python3 and +pip for python3 be installed. All of the required python modules are +described in the requirements.txt file in this directory and can be installed +with the command ```pip install -r requirements.txt``` -| Package | Version tested (Ubuntu 14.04) | -| -------------- | ----------------------------- | -| python | 2.7.5-5ubuntu3 | -| python-pytest | 2.5.1-1 | -| python-subunit | - | -| gdisk | 0.8.8-1ubuntu0.1 | -| dfu-util | 0.5-1 | -| dtc | 1.4.0+dfsg-1 | -| openssl | 1.0.1f-1ubuntu2.22 | +In order to execute certain tests on their supported platforms other tools +will be required. The following is an incomplete list: + +| Package | +| -------------- | +| gdisk | +| dfu-util | +| dtc | +| openssl | +| sudo OR guestmount | +| e2fsprogs | +| dosfstools | + +Please use the apporirate commands for your distribution to match these tools +up with the package that provides them. The test script supports either: @@ -45,18 +52,16 @@ The test script supports either: ### Using `virtualenv` to provide requirements -Older distributions (e.g. Ubuntu 10.04) may not provide all the required -packages, or may provide versions that are too old to run the test suite. One -can use the Python `virtualenv` script to locally install more up-to-date -versions of the required packages without interfering with the OS installation. -For example: +The recommended way to run the test suite, in order to ensure reproducibility +is to use `virtualenv` to set up the necessary environment. This can be done +via the following commands: ```bash $ cd /path/to/u-boot -$ sudo apt-get install python python-virtualenv -$ virtualenv venv +$ sudo apt-get install python3 python3-virtualenv +$ virtualenv -p /usr/bin/python3 venv $ . ./venv/bin/activate -$ pip install pytest +$ pip install -r test/py/requirements.txt ``` ## Testing sandbox diff --git a/test/py/requirements.txt b/test/py/requirements.txt new file mode 100644 index 0000000000..cf251186f4 --- /dev/null +++ b/test/py/requirements.txt @@ -0,0 +1,22 @@ +atomicwrites==1.3.0 +attrs==19.3.0 +coverage==4.5.4 +extras==1.0.0 +fixtures==3.0.0 +importlib-metadata==0.23 +linecache2==1.0.0 +more-itertools==7.2.0 +packaging==19.2 +pbr==5.4.3 +pluggy==0.13.0 +py==1.8.0 +pyparsing==2.4.2 +pytest==5.2.1 +python-mimeparse==1.6.0 +python-subunit==1.3.0 +six==1.12.0 +testtools==2.3.0 +traceback2==1.4.0 +unittest2==1.1.0 +wcwidth==0.1.7 +zipp==0.6.0 From 085b8978b997a1a901a98926ddbe16459aa6a746 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:27 -0400 Subject: [PATCH 12/39] gitlab/travis: Rework how and when we use virtualenv in order to use python3 As things stand today, we have tools that CI requires where "python" must be "python2". We need to use a virtualenv and pip in order to ensure that our pytest tests can be run. Rework things slightly so that: - On Travis-CI, we install python-pyelftools for the platforms that require pyelftools to be installed. - On GitLab-CI, we move to a newer base image that includes python3-pip and continue to use a virtualenv per job that needs it, for the correct set of packages. Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- .gitlab-ci.yml | 10 ++++------ .travis.yml | 30 +++++++++++++++--------------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5a34321570..9b295ac710 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -2,7 +2,7 @@ # Grab our configured image. The source for this is found at: # https://gitlab.denx.de/u-boot/gitlab-ci-runner -image: trini/u-boot-gitlab-ci-runner:bionic-20190912.1-03Oct2019 +image: trini/u-boot-gitlab-ci-runner:bionic-20191010-20Oct2019 # We run some tests in different order, to catch some failures quicker. stages: @@ -18,11 +18,6 @@ stages: - git clone --depth=1 git://github.com/swarren/uboot-test-hooks.git /tmp/uboot-test-hooks - ln -s travis-ci /tmp/uboot-test-hooks/bin/`hostname` - ln -s travis-ci /tmp/uboot-test-hooks/py/`hostname` - - virtualenv /tmp/venv - - . /tmp/venv/bin/activate - - pip install pytest - - pip install python-subunit - - pip install coverage - grub-mkimage --prefix="" -o ~/grub_x86.efi -O i386-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd - grub-mkimage --prefix="" -o ~/grub_x64.efi -O x86_64-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd - mkdir ~/grub2-arm @@ -47,6 +42,9 @@ stages: # never prevent any test from running. That way, we can always pass # "-k something" even when $TEST_PY_TEST_SPEC doesnt need a custom # value. + - virtualenv -p /usr/bin/python3 /tmp/venv + - . /tmp/venv/bin/activate + - pip install -r test/py/requirements.txt - export UBOOT_TRAVIS_BUILD_DIR=/tmp/.bm-work/${TEST_PY_BD}; export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:${PATH}; export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci; diff --git a/.travis.yml b/.travis.yml index c50270de5f..2369da97f9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -21,7 +21,9 @@ addons: - build-essential - libsdl1.2-dev - python - - python-virtualenv + - python-pyelftools + - python3-virtualenv + - python3-pip - swig - libpython-dev - iasl @@ -47,11 +49,6 @@ install: - echo -e "arc = /tmp/arc_gnu_2018.09_prebuilt_uclibc_le_archs_linux_install" >> ~/.buildman - echo -e "\n[toolchain-alias]\nsh = sh2\n" >> ~/.buildman - cat ~/.buildman - - virtualenv /tmp/venv - - . /tmp/venv/bin/activate - - pip install pytest - - pip install python-subunit - - pip install pyelftools - grub-mkimage --prefix="" -o ~/grub_x86.efi -O i386-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd - grub-mkimage --prefix="" -o ~/grub_x64.efi -O x86_64-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd - mkdir ~/grub2-arm @@ -136,15 +133,6 @@ script: cp ~/grub_x64.efi $UBOOT_TRAVIS_BUILD_DIR/; cp ~/grub2-arm/usr/lib/grub2/arm-efi/grub.efi $UBOOT_TRAVIS_BUILD_DIR/grub_arm.efi; cp ~/grub2-arm64/usr/lib/grub2/arm64-efi/grub.efi $UBOOT_TRAVIS_BUILD_DIR/grub_arm64.efi; - if [[ "${TEST_PY_BD}" != "" ]]; then - ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} - -k "${TEST_PY_TEST_SPEC:-not a_test_which_does_not_exist}" - --build-dir "$UBOOT_TRAVIS_BUILD_DIR"; - ret=$?; - if [[ $ret -ne 0 ]]; then - exit $ret; - fi; - fi; if [[ -n "${TEST_PY_TOOLS}" ]]; then PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt" PATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}" @@ -154,6 +142,18 @@ script: PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt" PATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}" ./tools/dtoc/dtoc -t; + fi; + if [[ "${TEST_PY_BD}" != "" ]]; then + virtualenv -p /usr/bin/python3 /tmp/venv; + . /tmp/venv/bin/activate; + pip install -r test/py/requirements.txt; + ./test/py/test.py --bd ${TEST_PY_BD} ${TEST_PY_ID} + -k "${TEST_PY_TEST_SPEC:-not a_test_which_does_not_exist}" + --build-dir "$UBOOT_TRAVIS_BUILD_DIR"; + ret=$?; + if [[ $ret -ne 0 ]]; then + exit $ret; + fi; fi matrix: From 15579631bc6b644eb504b1d9503174bd06b93439 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 24 Oct 2019 11:59:28 -0400 Subject: [PATCH 13/39] test/py: Use raw strings more to avoid deprecation warnings We have two further uses of raw string usage in the test/py codebase that are used under CI. The first of which is under the bind test and is a direct update. The second of which is to strip VT100 codes from the match buffer. While switching this to a raw string is also a direct update, the comment it notes that problems were encountered on Ubuntu 14.04 (and whatever Python 2 version that was) that required slight tweaks to the regex. Replace that now that we're saying Python 3.5 is the minimum. Reviewed-by: Simon Glass Tested-by: Stephen Warren Tested-by: Simon Glass [on sandbox] Signed-off-by: Tom Rini --- test/py/tests/test_bind.py | 4 ++-- test/py/u_boot_spawn.py | 5 +---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/py/tests/test_bind.py b/test/py/tests/test_bind.py index 2d48484c6a..20c6050342 100644 --- a/test/py/tests/test_bind.py +++ b/test/py/tests/test_bind.py @@ -9,11 +9,11 @@ def in_tree(response, name, uclass, drv, depth, last_child): lines = [x.strip() for x in response.splitlines()] leaf = ' ' * 4 * depth; if not last_child: - leaf = leaf + '\|' + leaf = leaf + r'\|' else: leaf = leaf + '`' leaf = leaf + '-- ' + name - line = (' *{:10.10} [0-9]* \[ [ +] \] {:20.20} {}$' + line = (r' *{:10.10} [0-9]* \[ [ +] \] {:20.20} {}$' .format(uclass, drv, leaf)) prog = re.compile(line) for l in lines: diff --git a/test/py/u_boot_spawn.py b/test/py/u_boot_spawn.py index 4f898cdefe..6991b78cca 100644 --- a/test/py/u_boot_spawn.py +++ b/test/py/u_boot_spawn.py @@ -42,10 +42,7 @@ class Spawn(object): self.after = '' self.timeout = None # http://stackoverflow.com/questions/7857352/python-regex-to-match-vt100-escape-sequences - # Note that re.I doesn't seem to work with this regex (or perhaps the - # version of Python in Ubuntu 14.04), hence the inclusion of a-z inside - # [] instead. - self.re_vt100 = re.compile('(\x1b\[|\x9b)[^@-_a-z]*[@-_a-z]|\x1b[@-_a-z]') + self.re_vt100 = re.compile(r'(\x1b\[|\x9b)[^@-_]*[@-_]|\x1b[@-_]', re.I) (self.pid, self.fd) = pty.fork() if self.pid == 0: From 3135022cb4db37ccf7691b03f6a99cfe6afb878d Mon Sep 17 00:00:00 2001 From: Keerthy Date: Thu, 24 Oct 2019 13:52:28 +0530 Subject: [PATCH 14/39] gpio: da8xx_gpio: Fix the _gpio_direction_output function _gpio_direction_output function currently calls gpio_set_value with the wrong gpio number. gpio_set_value in the uclass driver expects a different gpio number and the _gpio_direction_output is currently providing the number specific to the bank. Hence fix it by calling the _gpio_set_value function instead. Reported-by: Faiz Abbas Fixes: 8e51c0f254 ("dm: gpio: Add DM compatibility to GPIO driver for Davinci") Signed-off-by: Keerthy --- drivers/gpio/da8xx_gpio.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/gpio/da8xx_gpio.c b/drivers/gpio/da8xx_gpio.c index bd79448164..0a50c68d72 100644 --- a/drivers/gpio/da8xx_gpio.c +++ b/drivers/gpio/da8xx_gpio.c @@ -342,13 +342,6 @@ int gpio_free(unsigned int gpio) } #endif -static int _gpio_direction_output(struct davinci_gpio *bank, unsigned int gpio, int value) -{ - clrbits_le32(&bank->dir, 1U << GPIO_BIT(gpio)); - gpio_set_value(gpio, value); - return 0; -} - static int _gpio_direction_input(struct davinci_gpio *bank, unsigned int gpio) { setbits_le32(&bank->dir, 1U << GPIO_BIT(gpio)); @@ -377,6 +370,13 @@ static int _gpio_get_dir(struct davinci_gpio *bank, unsigned int gpio) return in_le32(&bank->dir) & (1U << GPIO_BIT(gpio)); } +static int _gpio_direction_output(struct davinci_gpio *bank, unsigned int gpio, + int value) +{ + clrbits_le32(&bank->dir, 1U << GPIO_BIT(gpio)); + _gpio_set_value(bank, gpio, value); + return 0; +} #ifndef CONFIG_DM_GPIO void gpio_info(void) From 90037eb483d3b84eab214a080e41efde85d9f2de Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Wed, 23 Oct 2019 15:39:41 -0400 Subject: [PATCH 15/39] Makefile: Fix printing problem in size_check on overflow When we have an excess size growth, fix the "limit" printf call to pass in just the limit variable rather than the string bytes to the format character. Signed-off-by: Tom Rini Reviewed-by: Simon Goldschmidt --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 58a5721b3d..a96c6ce81e 100644 --- a/Makefile +++ b/Makefile @@ -346,7 +346,7 @@ define size_check limit=$$( printf "%d" $2 ); \ if test $$actual -gt $$limit; then \ echo "$1 exceeds file size limit:" >&2; \ - echo " limit: $$(printf %#x bytes $$limit) bytes" >&2; \ + echo " limit: $$(printf %#x $$limit) bytes" >&2; \ echo " actual: $$(printf %#x $$actual) bytes" >&2; \ echo " excess: $$(printf %#x $$((actual - limit))) bytes" >&2;\ exit 1; \ From c9acae3396e752d0ebe2880ab8304170e0d560a4 Mon Sep 17 00:00:00 2001 From: Walter Lozano Date: Tue, 22 Oct 2019 11:50:19 -0300 Subject: [PATCH 16/39] dts: Kconfig: Fix help for SPL_OF_CONTROL As initially this feature was implemented as a negative CONFIG and later it was redesigned to be positive the help text should be updated to reflect this change. This commit updates the help text to match the current implementation. Signed-off-by: Walter Lozano Reviewed-by: Simon Glass --- dts/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dts/Kconfig b/dts/Kconfig index c9ab66cccc..424193dfad 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -44,7 +44,7 @@ config SPL_OF_CONTROL depends on SPL && OF_CONTROL help Some boards use device tree in U-Boot but only have 4KB of SRAM - which is not enough to support device tree. Enable this option to + which is not enough to support device tree. Disable this option to allow such boards to be supported by U-Boot SPL. config TPL_OF_CONTROL From c261fef51ca0fcb9cc752ddb4ffa4e134379b784 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 19 Oct 2019 09:06:38 +0200 Subject: [PATCH 17/39] checkpatch.pl: update from Linux kernel v5.4-rc3 Update from upstream. Just minor changes like checking that the author has also done a sign-off. Signed-off-by: Heinrich Schuchardt --- scripts/checkpatch.pl | 446 +++++++++++++++++++++++++++++++----------- 1 file changed, 333 insertions(+), 113 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 373094e59e..6fcc66afb0 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1,9 +1,11 @@ #!/usr/bin/env perl +# SPDX-License-Identifier: GPL-2.0 +# # (c) 2001, Dave Jones. (the file handling bit) # (c) 2005, Joel Schopp (the ugly bit) # (c) 2007,2008, Andy Whitcroft (new conditions, test suite) # (c) 2008-2010 Andy Whitcroft -# Licensed under the terms of the GNU GPL License version 2 +# (c) 2010-2018 Joe Perches use strict; use warnings; @@ -11,6 +13,7 @@ use POSIX; use File::Basename; use Cwd 'abs_path'; use Term::ANSIColor qw(:constants); +use Encode qw(decode encode); my $P = $0; my $D = dirname(abs_path($P)); @@ -58,7 +61,9 @@ my $codespellfile = "/usr/share/codespell/dictionary.txt"; my $conststructsfile = "$D/const_structs.checkpatch"; my $typedefsfile = ""; my $color = "auto"; -my $allow_c99_comments = 1; +my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE +# git output parsing needs US English output, so first set backtick child process LANGUAGE +my $git_command ='export LANGUAGE=en_US.UTF-8; git'; sub help { my ($exitcode) = @_; @@ -238,11 +243,11 @@ $check_orig = $check; my $exit = 0; +my $perl_version_ok = 1; if ($^V && $^V lt $minimum_perl_version) { + $perl_version_ok = 0; printf "$P: requires at least perl version %vd\n", $minimum_perl_version; - if (!$ignore_perl_version) { - exit(1); - } + exit(1) if (!$ignore_perl_version); } #if no filenames are given, push '-' to read patch from stdin @@ -344,9 +349,10 @@ our $Sparse = qr{ __force| __iomem| __must_check| - __init_refok| __kprobes| __ref| + __refconst| + __refdata| __rcu| __private }x; @@ -376,6 +382,7 @@ our $Attribute = qr{ __noclone| __deprecated| __read_mostly| + __ro_after_init| __kprobes| $InitAttribute| ____cacheline_aligned| @@ -461,8 +468,19 @@ our $logFunctions = qr{(?x: seq_vprintf|seq_printf|seq_puts )}; +our $allocFunctions = qr{(?x: + (?:(?:devm_)? + (?:kv|k|v)[czm]alloc(?:_node|_array)? | + kstrdup(?:_const)? | + kmemdup(?:_nul)?) | + (?:\w+)?alloc_skb(?:ip_align)? | + # dev_alloc_skb/netdev_alloc_skb, et al + dma_alloc_coherent +)}; + our $signature_tags = qr{(?xi: Signed-off-by:| + Co-developed-by:| Acked-by:| Tested-by:| Reviewed-by:| @@ -568,6 +586,27 @@ foreach my $entry (@mode_permission_funcs) { } $mode_perms_search = "(?:${mode_perms_search})"; +our %deprecated_apis = ( + "synchronize_rcu_bh" => "synchronize_rcu", + "synchronize_rcu_bh_expedited" => "synchronize_rcu_expedited", + "call_rcu_bh" => "call_rcu", + "rcu_barrier_bh" => "rcu_barrier", + "synchronize_sched" => "synchronize_rcu", + "synchronize_sched_expedited" => "synchronize_rcu_expedited", + "call_rcu_sched" => "call_rcu", + "rcu_barrier_sched" => "rcu_barrier", + "get_state_synchronize_sched" => "get_state_synchronize_rcu", + "cond_synchronize_sched" => "cond_synchronize_rcu", +); + +#Create a search pattern for all these strings to speed up a loop below +our $deprecated_apis_search = ""; +foreach my $entry (keys %deprecated_apis) { + $deprecated_apis_search .= '|' if ($deprecated_apis_search ne ""); + $deprecated_apis_search .= $entry; +} +$deprecated_apis_search = "(?:${deprecated_apis_search})"; + our $mode_perms_world_writable = qr{ S_IWUGO | S_IWOTH | @@ -845,6 +884,17 @@ sub is_maintained_obsolete { return $status =~ /obsolete/i; } +sub is_SPDX_License_valid { + my ($license) = @_; + + return 1 if (!$tree || which("python") eq "" || !(-e "$root/scripts/spdxcheck.py") || !(-e "$root/.git")); + + my $root_path = abs_path($root); + my $status = `cd "$root_path"; echo "$license" | python scripts/spdxcheck.py -`; + return 0 if ($status ne ""); + return 1; +} + my $camelcase_seeded = 0; sub seed_camelcase_includes { return if ($camelcase_seeded); @@ -856,7 +906,7 @@ sub seed_camelcase_includes { $camelcase_seeded = 1; if (-e ".git") { - my $git_last_include_commit = `git log --no-merges --pretty=format:"%h%n" -1 -- include`; + my $git_last_include_commit = `${git_command} log --no-merges --pretty=format:"%h%n" -1 -- include`; chomp $git_last_include_commit; $camelcase_cache = ".checkpatch-camelcase.git.$git_last_include_commit"; } else { @@ -884,7 +934,7 @@ sub seed_camelcase_includes { } if (-e ".git") { - $files = `git ls-files "include/*.h"`; + $files = `${git_command} ls-files "include/*.h"`; @include_files = split('\n', $files); } @@ -908,13 +958,13 @@ sub git_commit_info { return ($id, $desc) if ((which("git") eq "") || !(-e ".git")); - my $output = `git log --no-color --format='%H %s' -1 $commit 2>&1`; + my $output = `${git_command} log --no-color --format='%H %s' -1 $commit 2>&1`; $output =~ s/^\s*//gm; my @lines = split("\n", $output); return ($id, $desc) if ($#lines < 0); - if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous\./) { + if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous/) { # Maybe one day convert this block of bash into something that returns # all matching commit ids, but it's very slow... # @@ -958,7 +1008,7 @@ if ($git) { } else { $git_range = "-1 $commit_expr"; } - my $lines = `git log --no-color --no-merges --pretty=format:'%H %s' $git_range`; + my $lines = `${git_command} log --no-color --no-merges --pretty=format:'%H %s' $git_range`; foreach my $line (split(/\n/, $lines)) { $line =~ /^([0-9a-fA-F]{40,40}) (.*)$/; next if (!defined($1) || !defined($2)); @@ -973,6 +1023,7 @@ if ($git) { } my $vname; +$allow_c99_comments = !defined $ignore_type{"C99_COMMENT_TOLERANCE"}; for my $filename (@ARGV) { my $FILE; if ($git) { @@ -1024,11 +1075,11 @@ if (!$quiet) { hash_show_words(\%use_type, "Used"); hash_show_words(\%ignore_type, "Ignored"); - if ($^V lt 5.10.0) { + if (!$perl_version_ok) { print << "EOM" NOTE: perl $^V is not modern enough to detect all possible issues. - An upgrade to at least perl v5.10.0 is suggested. + An upgrade to at least perl $minimum_perl_version is suggested. EOM } if ($exit) { @@ -2233,10 +2284,14 @@ sub process { our $clean = 1; my $signoff = 0; + my $author = ''; + my $authorsignoff = 0; my $is_patch = 0; + my $is_binding_patch = -1; my $in_header_lines = $file ? 0 : 1; my $in_commit_log = 0; #Scanning lines before patch my $has_commit_log = 0; #Encountered lines before patch + my $commit_log_lines = 0; #Number of commit log lines my $commit_log_possible_stack_dump = 0; my $commit_log_long_line = 0; my $commit_log_has_diff = 0; @@ -2375,6 +2430,14 @@ sub process { my $rawline = $rawlines[$linenr - 1]; +# check if it's a mode change, rename or start of a patch + if (!$in_commit_log && + ($line =~ /^ mode change [0-7]+ => [0-7]+ \S+\s*$/ || + ($line =~ /^rename (?:from|to) \S+\s*$/ || + $line =~ /^diff --git a\/[\w\/\.\_\-]+ b\/\S+\s*$/))) { + $is_patch = 1; + } + #extract the line range in the file after the patch is applied if (!$in_commit_log && $line =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@(.*)/) { @@ -2475,6 +2538,19 @@ sub process { $check = $check_orig; } $checklicenseline = 1; + + if ($realfile !~ /^MAINTAINERS/) { + my $last_binding_patch = $is_binding_patch; + + $is_binding_patch = () = $realfile =~ m@^(?:Documentation/devicetree/|include/dt-bindings/)@; + + if (($last_binding_patch != -1) && + ($last_binding_patch ^ $is_binding_patch)) { + WARN("DT_SPLIT_BINDING_PATCH", + "DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.txt\n"); + } + } + next; } @@ -2486,6 +2562,18 @@ sub process { $cnt_lines++ if ($realcnt != 0); +# Verify the existence of a commit log if appropriate +# 2 is used because a $signature is counted in $commit_log_lines + if ($in_commit_log) { + if ($line !~ /^\s*$/) { + $commit_log_lines++; #could be a $signature + } + } elsif ($has_commit_log && $commit_log_lines < 2) { + WARN("COMMIT_MESSAGE", + "Missing commit description - Add an appropriate one\n"); + $commit_log_lines = 2; #warn only once + } + # Check if the commit log has what seems like a diff which can confuse patch if ($in_commit_log && !$commit_log_has_diff && (($line =~ m@^\s+diff\b.*a/[\w/]+@ && @@ -2507,10 +2595,24 @@ sub process { } } +# Check the patch for a From: + if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { + $author = $1; + $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); + $author =~ s/"//g; + } + # Check the patch for a signoff: if ($line =~ /^\s*signed-off-by:/i) { $signoff++; $in_commit_log = 0; + if ($author ne '') { + my $l = $line; + $l =~ s/"//g; + if ($l =~ /^\s*signed-off-by:\s*\Q$author\E/i) { + $authorsignoff = 1; + } + } } # Check if MAINTAINERS is being updated. If so, there's probably no need to @@ -2587,6 +2689,24 @@ sub process { } else { $signatures{$sig_nospace} = 1; } + +# Check Co-developed-by: immediately followed by Signed-off-by: with same name and email + if ($sign_off =~ /^co-developed-by:$/i) { + if ($email eq $author) { + WARN("BAD_SIGN_OFF", + "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline); + } + if (!defined $lines[$linenr]) { + WARN("BAD_SIGN_OFF", + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline); + } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) { + WARN("BAD_SIGN_OFF", + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]); + } elsif ($1 ne $email) { + WARN("BAD_SIGN_OFF", + "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]); + } + } } # Check email subject for common tools that don't need to be mentioned @@ -2596,12 +2716,6 @@ sub process { "A patch subject line should describe the change not the tool that found it\n" . $herecurr); } -# Check for old stable address - if ($line =~ /^\s*cc:\s*.*?.*$/i) { - ERROR("STABLE_ADDRESS", - "The 'stable' address should be 'stable\@vger.kernel.org'\n" . $herecurr); - } - # Check for unwanted Gerrit info if ($in_commit_log && $line =~ /^\s*change-id:/i) { ERROR("GERRIT_CHANGE_ID", @@ -2613,8 +2727,10 @@ sub process { ($line =~ /^\s*(?:WARNING:|BUG:)/ || $line =~ /^\s*\[\s*\d+\.\d{6,6}\s*\]/ || # timestamp - $line =~ /^\s*\[\<[0-9a-fA-F]{8,}\>\]/)) { - # stack dump address + $line =~ /^\s*\[\<[0-9a-fA-F]{8,}\>\]/) || + $line =~ /^(?:\s+\w+:\s+[0-9a-fA-F]+){3,3}/ || + $line =~ /^\s*\#\d+\s*\[[0-9a-fA-F]+\]\s*\w+ at [0-9a-fA-F]+/) { + # stack dump address styles $commit_log_possible_stack_dump = 1; } @@ -2786,6 +2902,17 @@ sub process { } } +# check for invalid commit id + if ($in_commit_log && $line =~ /(^fixes:|\bcommit)\s+([0-9a-f]{6,40})\b/i) { + my $id; + my $description; + ($id, $description) = git_commit_info($2, undef, undef); + if (!defined($id)) { + WARN("UNKNOWN_COMMIT_ID", + "Unknown commit id '$2', maybe rebased or not pulled?\n" . $herecurr); + } + } + # ignore non-hunk lines and lines being removed next if (!$hunk_line || $line =~ /^-/); @@ -2915,7 +3042,7 @@ sub process { my @compats = $rawline =~ /\"([a-zA-Z0-9\-\,\.\+_]+)\"/g; my $dt_path = $root . "/Documentation/devicetree/bindings/"; - my $vp_file = $dt_path . "vendor-prefixes.txt"; + my $vp_file = $dt_path . "vendor-prefixes.yaml"; foreach my $compat (@compats) { my $compat2 = $compat; @@ -2930,7 +3057,7 @@ sub process { next if $compat !~ /^([a-zA-Z0-9\-]+)\,/; my $vendor = $1; - `grep -Eq "^$vendor\\b" $vp_file`; + `grep -Eq "\\"\\^\Q$vendor\E,\\.\\*\\":" $vp_file`; if ( $? >> 8 ) { WARN("UNDOCUMENTED_DT_STRING", "DT compatible string vendor \"$vendor\" appears un-documented -- check $vp_file\n" . $herecurr); @@ -2954,10 +3081,24 @@ sub process { $comment = '..'; } +# check SPDX comment style for .[chsS] files + if ($realfile =~ /\.[chsS]$/ && + $rawline =~ /SPDX-License-Identifier:/ && + $rawline !~ m@^\+\s*\Q$comment\E\s*@) { + WARN("SPDX_LICENSE_TAG", + "Improper SPDX comment style for '$realfile', please use '$comment' instead\n" . $herecurr); + } + if ($comment !~ /^$/ && - $rawline !~ /^\+\Q$comment\E SPDX-License-Identifier: /) { + $rawline !~ m@^\+\Q$comment\E SPDX-License-Identifier: @) { WARN("SPDX_LICENSE_TAG", "Missing or malformed SPDX-License-Identifier tag in line $checklicenseline\n" . $herecurr); + } elsif ($rawline =~ /(SPDX-License-Identifier: .*)/) { + my $spdx_license = $1; + if (!is_SPDX_License_valid($spdx_license)) { + WARN("SPDX_LICENSE_TAG", + "'$spdx_license' is not supported in LICENSES/...\n" . $herecurr); + } } } } @@ -2965,6 +3106,14 @@ sub process { # check we are in a valid source file if not then ignore this hunk next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/); +# check for using SPDX-License-Identifier on the wrong line number + if ($realline != $checklicenseline && + $rawline =~ /\bSPDX-License-Identifier:/ && + substr($line, @-, @+ - @-) eq "$;" x (@+ - @-)) { + WARN("SPDX_LICENSE_TAG", + "Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr); + } + # line length limit (with some exclusions) # # There are a few types of lines that may extend beyond $max_line_length: @@ -3062,6 +3211,12 @@ sub process { } } +# check for assignments on the start of a line + if ($sline =~ /^\+\s+($Assignment)[^=]/) { + CHK("ASSIGNMENT_CONTINUATIONS", + "Assignment operator '$1' should be on the previous line\n" . $hereprev); + } + # check for && or || at the start of a line if ($rawline =~ /^\+\s*(&&|\|\|)/) { CHK("LOGICAL_CONTINUATIONS", @@ -3069,7 +3224,7 @@ sub process { } # check indentation starts on a tab stop - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) { my $indent = length($1); if ($indent % 8) { @@ -3082,7 +3237,7 @@ sub process { } # check multi-line statement indentation matches previous line - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:\*\s*)*$Lval\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) { $prevline =~ /^\+(\t*)(.*)$/; my $oldindent = $1; @@ -3239,7 +3394,7 @@ sub process { # known declaration macros $sline =~ /^\+\s+$declaration_macros/ || # start of struct or union or enum - $sline =~ /^\+\s+(?:union|struct|enum|typedef)\b/ || + $sline =~ /^\+\s+(?:static\s+)?(?:const\s+)?(?:union|struct|enum|typedef)\b/ || # start or end of block or continuation of declaration $sline =~ /^\+\s+(?:$|[\{\}\.\#\"\?\:\(\[])/ || # bitfield continuation @@ -3771,19 +3926,48 @@ sub process { "type '$tmp' should be specified in [[un]signed] [short|int|long|long long] order\n" . $herecurr); } +# check for unnecessary int declarations of short/long/long long + while ($sline =~ m{\b($TypeMisordered(\s*\*)*|$C90_int_types)\b}g) { + my $type = trim($1); + next if ($type !~ /\bint\b/); + next if ($type !~ /\b(?:short|long\s+long|long)\b/); + my $new_type = $type; + $new_type =~ s/\b\s*int\s*\b/ /; + $new_type =~ s/\b\s*(?:un)?signed\b\s*/ /; + $new_type =~ s/^const\s+//; + $new_type = "unsigned $new_type" if ($type =~ /\bunsigned\b/); + $new_type = "const $new_type" if ($type =~ /^const\b/); + $new_type =~ s/\s+/ /g; + $new_type = trim($new_type); + if (WARN("UNNECESSARY_INT", + "Prefer '$new_type' over '$type' as the int is unnecessary\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\b\Q$type\E\b/$new_type/; + } + } + # check for static const char * arrays. if ($line =~ /\bstatic\s+const\s+char\s*\*\s*(\w+)\s*\[\s*\]\s*=\s*/) { WARN("STATIC_CONST_CHAR_ARRAY", "static const char * array should probably be static const char * const\n" . $herecurr); - } + } + +# check for initialized const char arrays that should be static const + if ($line =~ /^\+\s*const\s+(char|unsigned\s+char|_*u8|(?:[us]_)?int8_t)\s+\w+\s*\[\s*(?:\w+\s*)?\]\s*=\s*"/) { + if (WARN("STATIC_CONST_CHAR_ARRAY", + "const array should probably be static const\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/(^.\s*)const\b/${1}static const/; + } + } # check for static char foo[] = "bar" declarations. if ($line =~ /\bstatic\s+char\s+(\w+)\s*\[\s*\]\s*=\s*"/) { WARN("STATIC_CONST_CHAR_ARRAY", "static char array declaration should probably be static const char\n" . $herecurr); - } + } # check for const const where is not a pointer or array type if ($sline =~ /\bconst\s+($BasicType)\s+const\b/) { @@ -3957,7 +4141,7 @@ sub process { # function brace can't be on same line, except for #defines of do while, # or if closed on same line - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $sline =~ /$Type\s*$Ident\s*$balanced_parens\s*\{/ && $sline !~ /\#\s*define\b.*do\s*\{/ && $sline !~ /}/) { @@ -4083,7 +4267,7 @@ sub process { my ($where, $prefix) = ($-[1], $1); if ($prefix !~ /$Type\s+$/ && ($where != 0 || $prefix !~ /^.\s+$/) && - $prefix !~ /[{,]\s+$/) { + $prefix !~ /[{,:]\s+$/) { if (ERROR("BRACKET_SPACE", "space prohibited before open square bracket '['\n" . $herecurr) && $fix) { @@ -4473,11 +4657,11 @@ sub process { #need space before brace following if, while, etc if (($line =~ /\(.*\)\{/ && $line !~ /\($Type\)\{/) || - $line =~ /do\{/) { + $line =~ /\b(?:else|do)\{/) { if (ERROR("SPACING", "space required before the open brace '{'\n" . $herecurr) && $fix) { - $fixed[$fixlinenr] =~ s/^(\+.*(?:do|\)))\{/$1 {/; + $fixed[$fixlinenr] =~ s/^(\+.*(?:do|else|\)))\{/$1 {/; } } @@ -4491,7 +4675,7 @@ sub process { # closing brace should have a space following it when it has anything # on the line - if ($line =~ /}(?!(?:,|;|\)))\S/) { + if ($line =~ /}(?!(?:,|;|\)|\}))\S/) { if (ERROR("SPACING", "space required after that close brace '}'\n" . $herecurr) && $fix) { @@ -4568,7 +4752,7 @@ sub process { # check for unnecessary parentheses around comparisons in if uses # when !drivers/staging or command-line uses --strict if (($realfile !~ m@^(?:drivers/staging/)@ || $check_orig) && - $^V && $^V ge 5.10.0 && defined($stat) && + $perl_version_ok && defined($stat) && $stat =~ /(^.\s*if\s*($balanced_parens))/) { my $if_stat = $1; my $test = substr($2, 1, -1); @@ -4605,7 +4789,7 @@ sub process { # return is not a function if (defined($stat) && $stat =~ /^.\s*return(\s*)\(/s) { my $spacing = $1; - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $stat =~ /^.\s*return\s*($balanced_parens)\s*;\s*$/) { my $value = $1; $value = deparenthesize($value); @@ -4632,7 +4816,7 @@ sub process { } # if statements using unnecessary parentheses - ie: if ((foo == bar)) - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $line =~ /\bif\s*((?:\(\s*){2,})/) { my $openparens = $1; my $count = $openparens =~ tr@\(@\(@; @@ -4649,7 +4833,7 @@ sub process { # avoid cases like "foo + BAR < baz" # only fix matches surrounded by parentheses to avoid incorrect # conversions like "FOO < baz() + 5" being "misfixed" to "baz() > FOO + 5" - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $line =~ /^\+(.*)\b($Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) { my $lead = $1; my $const = $2; @@ -4841,17 +5025,6 @@ sub process { while ($line =~ m{($Constant|$Lval)}g) { my $var = $1; -#gcc binary extension - if ($var =~ /^$Binary$/) { - if (WARN("GCC_BINARY_CONSTANT", - "Avoid gcc v4.3+ binary constant extension: <$var>\n" . $herecurr) && - $fix) { - my $hexval = sprintf("0x%x", oct($var)); - $fixed[$fixlinenr] =~ - s/\b$var\b/$hexval/; - } - } - #CamelCase if ($var !~ /^$Constant$/ && $var =~ /[A-Z][a-z]|[a-z][A-Z]/ && @@ -4939,6 +5112,7 @@ sub process { if (defined $define_args && $define_args ne "") { $define_args = substr($define_args, 1, length($define_args) - 2); $define_args =~ s/\s*//g; + $define_args =~ s/\\\+?//g; @def_args = split(",", $define_args); } @@ -5032,10 +5206,10 @@ sub process { next if ($arg =~ /\.\.\./); next if ($arg =~ /^type$/i); my $tmp_stmt = $define_stmt; - $tmp_stmt =~ s/\b(typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g; + $tmp_stmt =~ s/\b(sizeof|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g; $tmp_stmt =~ s/\#+\s*$arg\b//g; $tmp_stmt =~ s/\b$arg\s*\#\#//g; - my $use_cnt = $tmp_stmt =~ s/\b$arg\b//g; + my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g; if ($use_cnt > 1) { CHK("MACRO_ARG_REUSE", "Macro argument reuse '$arg' - possible side-effects?\n" . "$herectx"); @@ -5074,7 +5248,7 @@ sub process { # do {} while (0) macro tests: # single-statement macros do not need to be enclosed in do while (0) loop, # macro should not end with a semicolon - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $realfile !~ m@/vmlinux.lds.h$@ && $line =~ /^.\s*\#\s*define\s+$Ident(\()?/) { my $ln = $linenr; @@ -5115,16 +5289,6 @@ sub process { } } -# make sure symbols are always wrapped with VMLINUX_SYMBOL() ... -# all assignments may have only one of the following with an assignment: -# . -# ALIGN(...) -# VMLINUX_SYMBOL(...) - if ($realfile eq 'vmlinux.lds.h' && $line =~ /(?:(?:^|\s)$Ident\s*=|=\s*$Ident(?:\s|$))/) { - WARN("MISSING_VMLINUX_SYMBOL", - "vmlinux.lds.h needs VMLINUX_SYMBOL() around C-visible symbols\n" . $herecurr); - } - # check for redundant bracing round if etc if ($line =~ /(^.*)\bif\b/ && $1 !~ /else\s*$/) { my ($level, $endln, @chunks) = @@ -5330,15 +5494,28 @@ sub process { } # concatenated string without spaces between elements - if ($line =~ /$String[A-Z_]/ || $line =~ /[A-Za-z0-9_]$String/) { - CHK("CONCATENATED_STRING", - "Concatenated strings should use spaces between elements\n" . $herecurr); + if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /[A-Za-z0-9_]$String/) { + if (CHK("CONCATENATED_STRING", + "Concatenated strings should use spaces between elements\n" . $herecurr) && + $fix) { + while ($line =~ /($String)/g) { + my $extracted_string = substr($rawline, $-[0], $+[0] - $-[0]); + $fixed[$fixlinenr] =~ s/\Q$extracted_string\E([A-Za-z0-9_])/$extracted_string $1/; + $fixed[$fixlinenr] =~ s/([A-Za-z0-9_])\Q$extracted_string\E/$1 $extracted_string/; + } + } } # uncoalesced string fragments if ($line =~ /$String\s*"/) { - WARN("STRING_FRAGMENTS", - "Consecutive strings are generally better as a single string\n" . $herecurr); + if (WARN("STRING_FRAGMENTS", + "Consecutive strings are generally better as a single string\n" . $herecurr) && + $fix) { + while ($line =~ /($String)(?=\s*")/g) { + my $extracted_string = substr($rawline, $-[0], $+[0] - $-[0]); + $fixed[$fixlinenr] =~ s/\Q$extracted_string\E\s*"/substr($extracted_string, 0, -1)/e; + } + } } # check for non-standard and hex prefixed decimal printf formats @@ -5374,9 +5551,14 @@ sub process { # warn about #if 0 if ($line =~ /^.\s*\#\s*if\s+0\b/) { - CHK("REDUNDANT_CODE", - "if this code is redundant consider removing it\n" . - $herecurr); + WARN("IF_0", + "Consider removing the code enclosed by this #if 0 and its #endif\n" . $herecurr); + } + +# warn about #if 1 + if ($line =~ /^.\s*\#\s*if\s+1\b/) { + WARN("IF_1", + "Consider removing the #if 1 and its #endif\n" . $herecurr); } # check for needless "if () fn()" uses @@ -5423,7 +5605,8 @@ sub process { my ($s, $c) = ctx_statement_block($linenr - 3, $realcnt, 0); # print("line: <$line>\nprevline: <$prevline>\ns: <$s>\nc: <$c>\n\n\n"); - if ($s =~ /(?:^|\n)[ \+]\s*(?:$Type\s*)?\Q$testval\E\s*=\s*(?:\([^\)]*\)\s*)?\s*(?:devm_)?(?:[kv][czm]alloc(?:_node|_array)?\b|kstrdup|kmemdup|(?:dev_)?alloc_skb)/) { + if ($s =~ /(?:^|\n)[ \+]\s*(?:$Type\s*)?\Q$testval\E\s*=\s*(?:\([^\)]*\)\s*)?\s*$allocFunctions\s*\(/ && + $s !~ /\b__GFP_NOWARN\b/ ) { WARN("OOM_MESSAGE", "Possible unnecessary 'out of memory' message\n" . $hereprev); } @@ -5447,7 +5630,7 @@ sub process { } # check for mask then right shift without a parentheses - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $line =~ /$LvalOrFunc\s*\&\s*($LvalOrFunc)\s*>>/ && $4 !~ /^\&/) { # $LvalOrFunc may be &foo, ignore if so WARN("MASK_THEN_SHIFT", @@ -5455,7 +5638,7 @@ sub process { } # check for pointer comparisons to NULL - if ($^V && $^V ge 5.10.0) { + if ($perl_version_ok) { while ($line =~ /\b$LvalOrFunc\s*(==|\!=)\s*NULL\b/g) { my $val = $1; my $equal = "!"; @@ -5544,7 +5727,7 @@ sub process { # ignore udelay's < 10, however if (! ($delay < 10) ) { CHK("USLEEP_RANGE", - "usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt\n" . $herecurr); + "usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst\n" . $herecurr); } if ($delay > 2000) { WARN("LONG_UDELAY", @@ -5556,7 +5739,7 @@ sub process { if ($line =~ /\bmsleep\s*\((\d+)\);/) { if ($1 < 20) { WARN("MSLEEP", - "msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt\n" . $herecurr); + "msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst\n" . $herecurr); } } @@ -5698,13 +5881,6 @@ sub process { "__packed is preferred over __attribute__((packed))\n" . $herecurr); } -# Check for new packed members, warn to use care - if ($realfile !~ m@\binclude/uapi/@ && - $line =~ /\b(__attribute__\s*\(\s*\(.*\bpacked|__packed)\b/) { - WARN("NEW_PACKED", - "Adding new packed members is to be done with care\n" . $herecurr); - } - # Check for __attribute__ aligned, prefer __aligned if ($realfile !~ m@\binclude/uapi/@ && $line =~ /\b__attribute__\s*\(\s*\(.*aligned/) { @@ -5712,6 +5888,18 @@ sub process { "__aligned(size) is preferred over __attribute__((aligned(size)))\n" . $herecurr); } +# Check for __attribute__ section, prefer __section + if ($realfile !~ m@\binclude/uapi/@ && + $line =~ /\b__attribute__\s*\(\s*\(.*_*section_*\s*\(\s*("[^"]*")/) { + my $old = substr($rawline, $-[1], $+[1] - $-[1]); + my $new = substr($old, 1, -1); + if (WARN("PREFER_SECTION", + "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/; + } + } + # Check for __attribute__ format(printf, prefer __printf if ($realfile !~ m@\binclude/uapi/@ && $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf/) { @@ -5734,7 +5922,7 @@ sub process { } # Check for __attribute__ weak, or __weak declarations (may have link issues) - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $line =~ /(?:$Declare|$DeclareMisordered)\s*$Ident\s*$balanced_parens\s*(?:$Attribute)?\s*;/ && ($line =~ /\b__attribute__\s*\(\s*\(.*\bweak\b/ || $line =~ /\b__weak\b/)) { @@ -5816,25 +6004,25 @@ sub process { } # check for vsprintf extension %p misuses - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && $1 !~ /^_*volatile_*$/) { - my $specifier; - my $extension; - my $bad_specifier = ""; my $stat_real; my $lc = $stat =~ tr@\n@@; $lc = $lc + $linenr; for (my $count = $linenr; $count <= $lc; $count++) { + my $specifier; + my $extension; + my $bad_specifier = ""; my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); $fmt =~ s/%%//g; while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { $specifier = $1; $extension = $2; - if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOx]/) { + if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) { $bad_specifier = $specifier; last; } @@ -5863,7 +6051,7 @@ sub process { } # Check for misused memsets - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*$FuncArg\s*\)/) { @@ -5881,7 +6069,7 @@ sub process { } # Check for memcpy(foo, bar, ETH_ALEN) that could be ether_addr_copy(foo, bar) -# if ($^V && $^V ge 5.10.0 && +# if ($perl_version_ok && # defined $stat && # $stat =~ /^\+(?:.*?)\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) { # if (WARN("PREFER_ETHER_ADDR_COPY", @@ -5892,7 +6080,7 @@ sub process { # } # Check for memcmp(foo, bar, ETH_ALEN) that could be ether_addr_equal*(foo, bar) -# if ($^V && $^V ge 5.10.0 && +# if ($perl_version_ok && # defined $stat && # $stat =~ /^\+(?:.*?)\bmemcmp\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) { # WARN("PREFER_ETHER_ADDR_EQUAL", @@ -5901,7 +6089,7 @@ sub process { # check for memset(foo, 0x0, ETH_ALEN) that could be eth_zero_addr # check for memset(foo, 0xFF, ETH_ALEN) that could be eth_broadcast_addr -# if ($^V && $^V ge 5.10.0 && +# if ($perl_version_ok && # defined $stat && # $stat =~ /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) { # @@ -5923,7 +6111,7 @@ sub process { # } # typecasts on min/max could be min_t/max_t - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /^\+(?:.*?)\b(min|max)\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { if (defined $2 || defined $7) { @@ -5947,23 +6135,23 @@ sub process { } # check usleep_range arguments - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /^\+(?:.*?)\busleep_range\s*\(\s*($FuncArg)\s*,\s*($FuncArg)\s*\)/) { my $min = $1; my $max = $7; if ($min eq $max) { WARN("USLEEP_RANGE", - "usleep_range should not use min == max args; see Documentation/timers/timers-howto.txt\n" . "$here\n$stat\n"); + "usleep_range should not use min == max args; see Documentation/timers/timers-howto.rst\n" . "$here\n$stat\n"); } elsif ($min =~ /^\d+$/ && $max =~ /^\d+$/ && $min > $max) { WARN("USLEEP_RANGE", - "usleep_range args reversed, use min then max; see Documentation/timers/timers-howto.txt\n" . "$here\n$stat\n"); + "usleep_range args reversed, use min then max; see Documentation/timers/timers-howto.rst\n" . "$here\n$stat\n"); } } # check for naked sscanf - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $line =~ /\bsscanf\b/ && ($stat !~ /$Ident\s*=\s*sscanf\s*$balanced_parens/ && @@ -5977,7 +6165,7 @@ sub process { } # check for simple sscanf that should be kstrto - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $line =~ /\bsscanf\b/) { my $lc = $stat =~ tr@\n@@; @@ -6049,7 +6237,7 @@ sub process { } # check for function definitions - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /^.\s*(?:$Storage\s+)?$Type\s*($Ident)\s*$balanced_parens\s*{/s) { $context_function = $1; @@ -6081,22 +6269,22 @@ sub process { } } -# check for pointless casting of kmalloc return - if ($line =~ /\*\s*\)\s*[kv][czm]alloc(_node){0,1}\b/) { +# check for pointless casting of alloc functions + if ($line =~ /\*\s*\)\s*$allocFunctions\b/) { WARN("UNNECESSARY_CASTS", "unnecessary cast may hide bugs, see http://c-faq.com/malloc/mallocnocast.html\n" . $herecurr); } # alloc style # p = alloc(sizeof(struct foo), ...) should be p = alloc(sizeof(*p), ...) - if ($^V && $^V ge 5.10.0 && - $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*([kv][mz]alloc(?:_node)?)\s*\(\s*(sizeof\s*\(\s*struct\s+$Lval\s*\))/) { + if ($perl_version_ok && + $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc(?:_node)?)\s*\(\s*(sizeof\s*\(\s*struct\s+$Lval\s*\))/) { CHK("ALLOC_SIZEOF_STRUCT", "Prefer $3(sizeof(*$1)...) over $3($4...)\n" . $herecurr); } # check for k[mz]alloc with multiplies that could be kmalloc_array/kcalloc - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) { my $oldfunc = $3; @@ -6125,8 +6313,9 @@ sub process { } # check for krealloc arg reuse - if ($^V && $^V ge 5.10.0 && - $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*krealloc\s*\(\s*\1\s*,/) { + if ($perl_version_ok && + $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*krealloc\s*\(\s*($Lval)\s*,/ && + $1 eq $3) { WARN("KREALLOC_ARG_REUSE", "Reusing the krealloc arg is almost always a bug\n" . $herecurr); } @@ -6194,7 +6383,7 @@ sub process { } # check for switch/default statements without a break; - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /^\+[$;\s]*(?:case[$;\s]+\w+[$;\s]*:[$;\s]*|)*[$;\s]*\bdefault[$;\s]*:[$;\s]*;/g) { my $cnt = statement_rawlines($stat); @@ -6270,6 +6459,20 @@ sub process { "please use device_initcall() or more appropriate function instead of __initcall() (see include/linux/init.h)\n" . $herecurr); } +# check for spin_is_locked(), suggest lockdep instead + if ($line =~ /\bspin_is_locked\(/) { + WARN("USE_LOCKDEP", + "Where possible, use lockdep_assert_held instead of assertions based on spin_is_locked\n" . $herecurr); + } + +# check for deprecated apis + if ($line =~ /\b($deprecated_apis_search)\b\s*\(/) { + my $deprecated_api = $1; + my $new_api = $deprecated_apis{$deprecated_api}; + WARN("DEPRECATED_API", + "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr); + } + # check for various structs that are normally const (ops, kgdb, device_tree) # and avoid what seem like struct definitions 'struct foo {' if ($line !~ /\bconst\b/ && @@ -6298,12 +6501,18 @@ sub process { } # likely/unlikely comparisons similar to "(likely(foo) > 0)" - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && $line =~ /\b((?:un)?likely)\s*\(\s*$FuncArg\s*\)\s*$Compare/) { WARN("LIKELY_MISUSE", "Using $1 should generally have parentheses around the comparison\n" . $herecurr); } +# nested likely/unlikely calls + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) { + WARN("LIKELY_MISUSE", + "nested (un)?likely() calls, $1 already uses unlikely() internally\n" . $herecurr); + } + # whine mightly about in_atomic if ($line =~ /\bin_atomic\s*\(/) { if ($realfile =~ m@^drivers/@) { @@ -6341,7 +6550,7 @@ sub process { # check for DEVICE_ATTR uses that could be DEVICE_ATTR_ # and whether or not function naming is typical and if # DEVICE_ATTR permissions uses are unusual too - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $stat =~ /\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?\s*(\s*(?:${multi_mode_perms_string_search}|0[0-7]{3,3})\s*)\s*\)?\s*,\s*(\w+)\s*,\s*(\w+)\s*\)/) { my $var = $1; @@ -6401,7 +6610,7 @@ sub process { # specific definition of not visible in sysfs. # o Ignore proc_create*(...) uses with a decimal 0 permission as that means # use the default permissions - if ($^V && $^V ge 5.10.0 && + if ($perl_version_ok && defined $stat && $line =~ /$mode_perms_search/) { foreach my $entry (@mode_permission_funcs) { @@ -6463,6 +6672,12 @@ sub process { "unknown module license " . $extracted_string . "\n" . $herecurr); } } + +# check for sysctl duplicate constants + if ($line =~ /\.extra[12]\s*=\s*&(zero|one|int_max)\b/) { + WARN("DUPLICATED_SYSCTL_CONST", + "duplicated sysctl range checking value '$1', consider using the shared one in include/linux/sysctl.h\n" . $herecurr); + } } # If we have no input at all, then there is nothing to report on @@ -6487,9 +6702,14 @@ sub process { ERROR("NOT_UNIFIED_DIFF", "Does not appear to be a unified-diff format patch\n"); } - if ($is_patch && $has_commit_log && $chk_signoff && $signoff == 0) { - ERROR("MISSING_SIGN_OFF", - "Missing Signed-off-by: line(s)\n"); + if ($is_patch && $has_commit_log && $chk_signoff) { + if ($signoff == 0) { + ERROR("MISSING_SIGN_OFF", + "Missing Signed-off-by: line(s)\n"); + } elsif (!$authorsignoff) { + WARN("NO_AUTHOR_SIGN_OFF", + "Missing Signed-off-by: line by nominal patch author '$author'\n"); + } } print report_dump(); From 232ba7623171635e971227696edefddc84d47927 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Wed, 16 Oct 2019 12:59:49 +0200 Subject: [PATCH 18/39] virtio: pci: use correct type in virtio_pci_bind() For printing as %u we should use an unsigned int. Signed-off-by: Heinrich Schuchardt Reviewed-by: Bin Meng --- drivers/virtio/virtio_pci_legacy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c index 08764ee6f2..202e5ab1d3 100644 --- a/drivers/virtio/virtio_pci_legacy.c +++ b/drivers/virtio/virtio_pci_legacy.c @@ -277,7 +277,7 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq) static int virtio_pci_bind(struct udevice *udev) { - static int num_devs; + static unsigned int num_devs; char name[20]; /* Create a unique device name for PCI type devices */ From 8c403402ca691c967516481b6bc2c879d683a73d Mon Sep 17 00:00:00 2001 From: Patrick Wildt Date: Wed, 16 Oct 2019 23:22:50 +0200 Subject: [PATCH 19/39] nvme: flush dcache on both r/w, and the prp list It's possible that the data cache for the buffer still holds data to be flushed to memory, since the buffer was probably used as stack before. Thus we need to make sure to flush it also on reads, since it's possible that the cache is automatically flused to memory after the NVMe DMA transfer happened, thus overwriting the NVMe transfer's data. Also add a missing dcache flush for the prp list. Signed-off-by: Patrick Wildt Reviewed-by: Bin Meng --- drivers/nvme/nvme.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index ee6b581d9e..53ff6e89aa 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -123,6 +123,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2, } *prp2 = (ulong)dev->prp_pool; + flush_dcache_range((ulong)dev->prp_pool, (ulong)dev->prp_pool + + dev->prp_entry_num * sizeof(u64)); + return 0; } @@ -705,9 +708,8 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); u64 total_lbas = blkcnt; - if (!read) - flush_dcache_range((unsigned long)buffer, - (unsigned long)buffer + total_len); + flush_dcache_range((unsigned long)buffer, + (unsigned long)buffer + total_len); c.rw.opcode = read ? nvme_cmd_read : nvme_cmd_write; c.rw.flags = 0; From 2f83481dff9c4f253a6ac341911d78d4984ca07b Mon Sep 17 00:00:00 2001 From: Patrick Wildt Date: Wed, 16 Oct 2019 08:42:04 +0200 Subject: [PATCH 20/39] nvme: use page-aligned buffer for identify command Change the stack-allocated buffer for the identification command to explicitly allocate page-aligned buffers. Even though the spec seems to allow having admin queue commands on non page-aligned buffers, it seems to not be possible on my i.MX8MQ board with a a Silicon Power P34A80. Since all of the NVMe drivers I have seen always do admin commands on a page-aligned buffer, which does work on my system, it makes sense for us to do that as well. Signed-off-by: Patrick Wildt Reviewed-by: Bin Meng --- drivers/nvme/nvme.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 53ff6e89aa..f915817aaa 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -583,14 +583,19 @@ static int nvme_setup_io_queues(struct nvme_dev *dev) static int nvme_get_info_from_identify(struct nvme_dev *dev) { - ALLOC_CACHE_ALIGN_BUFFER(char, buf, sizeof(struct nvme_id_ctrl)); - struct nvme_id_ctrl *ctrl = (struct nvme_id_ctrl *)buf; + struct nvme_id_ctrl *ctrl; int ret; int shift = NVME_CAP_MPSMIN(dev->cap) + 12; + ctrl = memalign(dev->page_size, sizeof(struct nvme_id_ctrl)); + if (!ctrl) + return -ENOMEM; + ret = nvme_identify(dev, 0, 1, (dma_addr_t)(long)ctrl); - if (ret) + if (ret) { + free(ctrl); return -EIO; + } dev->nn = le32_to_cpu(ctrl->nn); dev->vwc = ctrl->vwc; @@ -621,6 +626,7 @@ static int nvme_get_info_from_identify(struct nvme_dev *dev) dev->max_transfer_shift = 20; } + free(ctrl); return 0; } @@ -661,16 +667,21 @@ static int nvme_blk_probe(struct udevice *udev) struct blk_desc *desc = dev_get_uclass_platdata(udev); struct nvme_ns *ns = dev_get_priv(udev); u8 flbas; - ALLOC_CACHE_ALIGN_BUFFER(char, buf, sizeof(struct nvme_id_ns)); - struct nvme_id_ns *id = (struct nvme_id_ns *)buf; struct pci_child_platdata *pplat; + struct nvme_id_ns *id; + + id = memalign(ndev->page_size, sizeof(struct nvme_id_ns)); + if (!id) + return -ENOMEM; memset(ns, 0, sizeof(*ns)); ns->dev = ndev; /* extract the namespace id from the block device name */ ns->ns_id = trailing_strtol(udev->name) + 1; - if (nvme_identify(ndev, ns->ns_id, 0, (dma_addr_t)(long)id)) + if (nvme_identify(ndev, ns->ns_id, 0, (dma_addr_t)(long)id)) { + free(id); return -EIO; + } memcpy(&ns->eui64, &id->eui64, sizeof(id->eui64)); flbas = id->flbas & NVME_NS_FLBAS_LBA_MASK; @@ -689,6 +700,7 @@ static int nvme_blk_probe(struct udevice *udev) memcpy(desc->product, ndev->serial, sizeof(ndev->serial)); memcpy(desc->revision, ndev->firmware_rev, sizeof(ndev->firmware_rev)); + free(id); return 0; } From 52e1d93c14d0a56651367eb00f8d6b6fe80a2708 Mon Sep 17 00:00:00 2001 From: Patrick Wildt Date: Thu, 3 Oct 2019 11:10:57 +0200 Subject: [PATCH 21/39] NVMe: do PCI enumerate before nvme scan Make sure that the PCI busses are enumerated before trying to find a NVMe device. Signed-off-by: Patrick Wildt Reviewed-by: Bin Meng --- include/config_distro_bootcmd.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 3570a32dff..fc0935fa21 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -189,6 +189,7 @@ "fi\0" \ \ "nvme_boot=" \ + BOOTENV_RUN_PCI_ENUM \ BOOTENV_RUN_NVME_INIT \ BOOTENV_SHARED_BLKDEV_BODY(nvme) #define BOOTENV_DEV_NVME BOOTENV_DEV_BLKDEV From 80e7e7c2aba5793a1e39592cd53de9e5aca96f0b Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Tue, 15 Oct 2019 22:43:41 +0200 Subject: [PATCH 22/39] lib: time: Add microsecond timer Add get_timer_us(), which is useful e.g. when we need higher precision timestamps. Signed-off-by: Marek Vasut Cc: Tom Rini Cc: Simon Glass [trini: Fixup arch/arm/mach-bcm283x/include/mach/timer.h] Signed-off-by: Tom Rini --- arch/arm/mach-bcm283x/include/mach/timer.h | 3 --- include/time.h | 1 + lib/time.c | 14 ++++++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/arch/arm/mach-bcm283x/include/mach/timer.h b/arch/arm/mach-bcm283x/include/mach/timer.h index 014355e759..61beb1aba1 100644 --- a/arch/arm/mach-bcm283x/include/mach/timer.h +++ b/arch/arm/mach-bcm283x/include/mach/timer.h @@ -25,9 +25,6 @@ struct bcm2835_timer_regs { u32 c2; u32 c3; }; - -extern ulong get_timer_us(ulong base); - #endif #endif diff --git a/include/time.h b/include/time.h index 1e9b369be7..a1149522ed 100644 --- a/include/time.h +++ b/include/time.h @@ -13,6 +13,7 @@ unsigned long get_timer(unsigned long base); * Granularity may be larger than 1us if hardware does not support this. */ unsigned long timer_get_us(void); +uint64_t get_timer_us(uint64_t base); /* * timer_test_add_offset() diff --git a/lib/time.c b/lib/time.c index f5751ab162..f30fc05804 100644 --- a/lib/time.c +++ b/lib/time.c @@ -134,6 +134,20 @@ ulong __weak get_timer(ulong base) return tick_to_time(get_ticks()) - base; } +static uint64_t notrace tick_to_time_us(uint64_t tick) +{ + ulong div = get_tbclk() / 1000; + + tick *= CONFIG_SYS_HZ; + do_div(tick, div); + return tick; +} + +uint64_t __weak get_timer_us(uint64_t base) +{ + return tick_to_time_us(get_ticks()) - base; +} + unsigned long __weak notrace timer_get_us(void) { return tick_to_time(get_ticks() * 1000); From 02e8a8241bb5fff315f3e721992220428f288b6d Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 15 Oct 2019 21:46:03 +0200 Subject: [PATCH 23/39] lib: errno: check for unsupported error number If errno_str() is called with an unsupported error number, do not return a random pointer but a reasonable text. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- lib/errno_str.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/errno_str.c b/lib/errno_str.c index 0ba950e970..bb8f9fbeb3 100644 --- a/lib/errno_str.c +++ b/lib/errno_str.c @@ -136,6 +136,8 @@ static const char * const errno_message[] = { ERRNO_MSG(EDQUOT, "Quota exceeded"), ERRNO_MSG(ENOMEDIUM, "No medium found"), ERRNO_MSG(EMEDIUMTYPE, "Wrong medium type"), + /* Message for unsupported error numbers */ + ERRNO_MSG(0, "Unknown error"), }; const char *errno_str(int errno) @@ -143,5 +145,9 @@ const char *errno_str(int errno) if (errno >= 0) return errno_message[0]; - return errno_message[abs(errno)]; + errno = -errno; + if (errno >= ARRAY_SIZE(errno_message)) + errno = ARRAY_SIZE(errno_message) - 1; + + return errno_message[errno]; } From 79c84de4689b1c22be3ccac8914307daa38b0bd8 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 15 Oct 2019 21:46:04 +0200 Subject: [PATCH 24/39] test: provide test for errno_str() Provide a unit test for errno_str(). Test that known and unknown error numbers are handled correctly. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- test/lib/Makefile | 1 + test/lib/test_errno_str.c | 46 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 test/lib/test_errno_str.c diff --git a/test/lib/Makefile b/test/lib/Makefile index 308c61708e..b13aaca7ce 100644 --- a/test/lib/Makefile +++ b/test/lib/Makefile @@ -6,3 +6,4 @@ obj-y += cmd_ut_lib.o obj-y += hexdump.o obj-y += lmb.o obj-y += string.o +obj-$(CONFIG_ERRNO_STR) += test_errno_str.o diff --git a/test/lib/test_errno_str.c b/test/lib/test_errno_str.c new file mode 100644 index 0000000000..8a9f1fd980 --- /dev/null +++ b/test/lib/test_errno_str.c @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2019 Heinrich Schuchardt + * + * Unit tests for memory functions + * + * The architecture dependent implementations run through different lines of + * code depending on the alignment and length of memory regions copied or set. + * This has to be considered in testing. + */ + +#include +#include +#include +#include +#include +#include + +/** + * lib_errno_str() - unit test for errno_str() + * + * Test errno_str() with varied alignment and length of the copied buffer. + * + * @uts: unit test state + * Return: 0 = success, 1 = failure + */ +static int lib_errno_str(struct unit_test_state *uts) +{ + const char *msg; + + msg = errno_str(1); + ut_asserteq_str("Success", msg); + + msg = errno_str(0); + ut_asserteq_str("Success", msg); + + msg = errno_str(-ENOMEM); + ut_asserteq_str("Out of memory", msg); + + msg = errno_str(-99999); + ut_asserteq_str("Unknown error", msg); + + return 0; +} + +LIB_TEST(lib_errno_str, 0); From 34856b0f1c4e56ad63879b9cdef71302225423cd Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 15 Oct 2019 20:43:42 +0200 Subject: [PATCH 25/39] disk: part_dos: correctly detect DOS PBR The signature 0x55 0xAA in bytes 510 and 511 of the first sector can either indicate a DOS partition table of the first sector of a FAT file system. The current code tries to check if the partition table is valid by looking at the boot indicator of the partition entries. But first of all it does not count from 0 to 3 but only from 0 to 2. And second it misses to increment the pointer for the partition entry. If it is a FAT file system can be discovered by looking for the text 'FAT' at offset 0x36 or 'FAT32' at offset 0x52. In a DOS PBR there are no partition entries, so those bytes are undefined. Don't require the byte at offset 0x1BE to differ from 0x00 and 0x80. With the patch the logic is changed as follows: If the partition table has either an invalid boot flag for any partition or has no partition at all, check if the first sector is a DOS PBR by looking at the FAT* signature. Signed-off-by: Heinrich Schuchardt --- disk/part_dos.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/disk/part_dos.c b/disk/part_dos.c index 8ddc13b50c..83ff40d310 100644 --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -67,28 +67,39 @@ static int test_block_type(unsigned char *buffer) { int slot; struct dos_partition *p; + int part_count = 0; if((buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) || (buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) ) { return (-1); } /* no DOS Signature at all */ p = (struct dos_partition *)&buffer[DOS_PART_TBL_OFFSET]; - for (slot = 0; slot < 3; slot++) { - if (p->boot_ind != 0 && p->boot_ind != 0x80) { - if (!slot && - (strncmp((char *)&buffer[DOS_PBR_FSTYPE_OFFSET], - "FAT", 3) == 0 || - strncmp((char *)&buffer[DOS_PBR32_FSTYPE_OFFSET], - "FAT32", 5) == 0)) { - return DOS_PBR; /* is PBR */ - } else { - return -1; - } - } - } - return DOS_MBR; /* Is MBR */ -} + /* Check that the boot indicators are valid and count the partitions. */ + for (slot = 0; slot < 4; ++slot, ++p) { + if (p->boot_ind != 0 && p->boot_ind != 0x80) + break; + if (p->sys_ind) + ++part_count; + } + + /* + * If the partition table is invalid or empty, + * check if this is a DOS PBR + */ + if (slot != 4 || !part_count) { + if (!strncmp((char *)&buffer[DOS_PBR_FSTYPE_OFFSET], + "FAT", 3) || + !strncmp((char *)&buffer[DOS_PBR32_FSTYPE_OFFSET], + "FAT32", 5)) + return DOS_PBR; /* This is a DOS PBR and not an MBR */ + } + if (slot == 4) + return DOS_MBR; /* This is an DOS MBR */ + + /* This is neither a DOS MBR nor a DOS PBR */ + return -1; +} static int part_test_dos(struct blk_desc *dev_desc) { From 6e7325533ba4edb916a1be158c43f7eee506c6c9 Mon Sep 17 00:00:00 2001 From: Roman Kapl Date: Mon, 14 Oct 2019 11:21:09 +0200 Subject: [PATCH 26/39] ata: ahci allow 64-bit DMA for SATA Allow 64-bit DMA on AHCI. If not supported by the host controller, at least print a message and fail. Signed-off-by: Roman Kapl --- drivers/ata/ahci.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 21a89eba5a..d10f9f0bf8 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -50,6 +50,8 @@ struct ahci_uc_priv *probe_ent = NULL; #define WAIT_MS_FLUSH 5000 #define WAIT_MS_LINKUP 200 +#define AHCI_CAP_S64A BIT(31) + __weak void __iomem *ahci_port_base(void __iomem *base, u32 port) { return base + 0x100 + (port * 0x80); @@ -503,9 +505,15 @@ static int ahci_fill_sg(struct ahci_uc_priv *uc_priv, u8 port, } for (i = 0; i < sg_count; i++) { - ahci_sg->addr = - cpu_to_le32((unsigned long) buf + i * MAX_DATA_BYTE_COUNT); - ahci_sg->addr_hi = 0; + /* We assume virt=phys */ + phys_addr_t pa = (unsigned long)buf + i * MAX_DATA_BYTE_COUNT; + + ahci_sg->addr = cpu_to_le32(lower_32_bits(pa)); + ahci_sg->addr_hi = cpu_to_le32(upper_32_bits(pa)); + if (ahci_sg->addr_hi && !(uc_priv->cap & AHCI_CAP_S64A)) { + printf("Error: DMA address too high\n"); + return -1; + } ahci_sg->flags_size = cpu_to_le32(0x3fffff & (buf_len < MAX_DATA_BYTE_COUNT ? (buf_len - 1) From 574e38618b1de30311e17186759486f217cda39a Mon Sep 17 00:00:00 2001 From: Baruch Siach Date: Mon, 7 Oct 2019 15:05:47 +0300 Subject: [PATCH 27/39] dts: fix MULTI_DTB_FIT compression choice prompt This choice is not about SPL for which we have a separate choice. Fixes: 95f4bbd581 ("lib: fdt: Allow LZO and GZIP DT compression in U-Boot") Cc: Marek Vasut Signed-off-by: Baruch Siach --- dts/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dts/Kconfig b/dts/Kconfig index 424193dfad..2bd959a7dc 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -131,7 +131,7 @@ config OF_LIST separated by . choice - prompt "SPL OF LIST compression" + prompt "OF LIST compression" depends on MULTI_DTB_FIT default MULTI_DTB_FIT_NO_COMPRESSION From eb5b63f3699fd8068f79cf8ffe5524f98ec00ba6 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sun, 6 Oct 2019 13:22:21 +0200 Subject: [PATCH 28/39] lib: errno: sync error codes Macro ERRNO_MSG() ignores the error number but we should still use the same constants as in include/linux/errno.h. Signed-off-by: Heinrich Schuchardt --- lib/errno_str.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/errno_str.c b/lib/errno_str.c index bb8f9fbeb3..2e5f4a887d 100644 --- a/lib/errno_str.c +++ b/lib/errno_str.c @@ -13,7 +13,7 @@ static const char * const errno_message[] = { ERRNO_MSG(0, "Success"), ERRNO_MSG(EPERM, "Operation not permitted"), - ERRNO_MSG(ENOEN, "No such file or directory"), + ERRNO_MSG(ENOENT, "No such file or directory"), ERRNO_MSG(ESRCH, "No such process"), ERRNO_MSG(EINTR, "Interrupted system call"), ERRNO_MSG(EIO, "I/O error"), @@ -26,7 +26,7 @@ static const char * const errno_message[] = { ERRNO_MSG(ENOMEM, "Out of memory"), ERRNO_MSG(EACCES, "Permission denied"), ERRNO_MSG(EFAULT, "Bad address"), - ERRNO_MSG(ENOTBL, "Block device required"), + ERRNO_MSG(ENOTBLK, "Block device required"), ERRNO_MSG(EBUSY, "Device or resource busy"), ERRNO_MSG(EEXIST, "File exists"), ERRNO_MSG(EXDEV, "Cross-device link"), From f279e1d9167fc0dcc7172c8f845a253511ac6001 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sun, 6 Oct 2019 13:58:57 +0200 Subject: [PATCH 29/39] lib: errno: avoid error format-overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In cmd/regulator.c an error occurs with GCC 9.2.1 if CONFIG_ERRNO_STR is not defined: cmd/regulator.c: In function ‘failure’: cmd/regulator.c:20:2: error: ‘%s’ directive argument is null [-Werror=format-overflow=] 20 | printf("Error: %d (%s)\n", ret, errno_str(ret)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In function ‘constraint’, inlined from ‘constraint’ at cmd/regulator.c:111:12: cmd/regulator.c:115:3: error: ‘%s’ directive argument is null [-Werror=format-overflow=] 115 | printf(" %s (err: %d)\n", errno_str(val), val); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ errno_str() should return a valid string instead of NULL if CONFIG_ERRNO_STR is not defined. Signed-off-by: Heinrich Schuchardt --- include/errno.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/include/errno.h b/include/errno.h index ccb7869e17..3af539b9e9 100644 --- a/include/errno.h +++ b/include/errno.h @@ -12,12 +12,21 @@ extern int errno; #define __set_errno(val) do { errno = val; } while (0) +/** + * errno_str() - get description for error number + * + * @errno: error number (negative in case of error) + * Return: string describing the error. If CONFIG_ERRNO_STR is not + * defined an empty string is returned. + */ #ifdef CONFIG_ERRNO_STR const char *errno_str(int errno); #else +static const char error_message[] = ""; + static inline const char *errno_str(int errno) { - return 0; + return error_message; } #endif #endif /* _ERRNO_H */ From 54b6abae3ae9dd181044ddd52429b4df5505872f Mon Sep 17 00:00:00 2001 From: Simon South Date: Wed, 2 Oct 2019 10:55:06 -0400 Subject: [PATCH 30/39] common: Kconfig: Fix typo in TPL_LOG_CONSOLE description Signed-off-by: Simon South --- common/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/Kconfig b/common/Kconfig index 28d5e9a0cc..d9ecf79e0a 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -764,7 +764,7 @@ config SPL_LOG_CONSOLE line number are omitted. config TPL_LOG_CONSOLE - bool "Allow log output to the console in SPL" + bool "Allow log output to the console in TPL" depends on TPL_LOG default y help From 9b3fbb2b439a717dda0eaa34cb455a9ce38532ef Mon Sep 17 00:00:00 2001 From: Simon South Date: Wed, 2 Oct 2019 10:55:07 -0400 Subject: [PATCH 31/39] tiny-printf: Support vsnprintf() Add a simple implementation of this function, to allow logging to be enabled in the SPL or TPL for systems that rely on the tiny printf() implementation. To keep the code size small, - The function is built only when logging is enabled, as it (currently) is not needed otherwise; and - Like the existing implementation of snprintf(), its buffer-size parameter is ignored. Signed-off-by: Simon South --- lib/tiny-printf.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index ebef92fc9f..62e6381961 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -366,6 +366,22 @@ int sprintf(char *buf, const char *fmt, ...) return ret; } +#if CONFIG_IS_ENABLED(LOG) +/* Note that size is ignored */ +int vsnprintf(char *buf, size_t size, const char *fmt, va_list va) +{ + struct printf_info info; + int ret; + + info.outstr = buf; + info.putc = putc_outstr; + ret = _vprintf(&info, fmt, va); + *info.outstr = '\0'; + + return ret; +} +#endif + /* Note that size is ignored */ int snprintf(char *buf, size_t size, const char *fmt, ...) { From 2ad98ab8f68c71e2db140c6d5f1020aa6fbacb9e Mon Sep 17 00:00:00 2001 From: Marek Szyprowski Date: Wed, 2 Oct 2019 14:37:20 +0200 Subject: [PATCH 32/39] linux_compat: fix potential NULL pointer access malloc_cache_aligned() might return zero, so fix potential NULL pointer access if __GFP_ZERO flag is set. Signed-off-by: Marek Szyprowski Reviewed-by: Ralph Siemsen --- lib/linux_compat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/linux_compat.c b/lib/linux_compat.c index 6373b4451e..81ea8fb126 100644 --- a/lib/linux_compat.c +++ b/lib/linux_compat.c @@ -20,7 +20,7 @@ void *kmalloc(size_t size, int flags) void *p; p = malloc_cache_aligned(size); - if (flags & __GFP_ZERO) + if (p && flags & __GFP_ZERO) memset(p, 0, size); return p; From 4e1842988364446ba0cf2171d1eebb53c15bc44e Mon Sep 17 00:00:00 2001 From: Jean-Jacques Hiblot Date: Tue, 1 Oct 2019 14:03:26 +0200 Subject: [PATCH 33/39] drivers: phy: Handle gracefully NULL pointers For some controllers PHYs can be optional. Handling NULL pointers without crashing nor failing, makes it easy to handle optional PHYs. Signed-off-by: Jean-Jacques Hiblot --- drivers/phy/phy-uclass.c | 30 +++++++++++++++++++++++++----- include/generic-phy.h | 2 +- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c index a0ac30aa71..e201a90c8c 100644 --- a/drivers/phy/phy-uclass.c +++ b/drivers/phy/phy-uclass.c @@ -108,35 +108,55 @@ int generic_phy_get_by_name(struct udevice *dev, const char *phy_name, int generic_phy_init(struct phy *phy) { - struct phy_ops const *ops = phy_dev_ops(phy->dev); + struct phy_ops const *ops; + + if (!phy) + return 0; + ops = phy_dev_ops(phy->dev); return ops->init ? ops->init(phy) : 0; } int generic_phy_reset(struct phy *phy) { - struct phy_ops const *ops = phy_dev_ops(phy->dev); + struct phy_ops const *ops; + + if (!phy) + return 0; + ops = phy_dev_ops(phy->dev); return ops->reset ? ops->reset(phy) : 0; } int generic_phy_exit(struct phy *phy) { - struct phy_ops const *ops = phy_dev_ops(phy->dev); + struct phy_ops const *ops; + + if (!phy) + return 0; + ops = phy_dev_ops(phy->dev); return ops->exit ? ops->exit(phy) : 0; } int generic_phy_power_on(struct phy *phy) { - struct phy_ops const *ops = phy_dev_ops(phy->dev); + struct phy_ops const *ops; + + if (!phy) + return 0; + ops = phy_dev_ops(phy->dev); return ops->power_on ? ops->power_on(phy) : 0; } int generic_phy_power_off(struct phy *phy) { - struct phy_ops const *ops = phy_dev_ops(phy->dev); + struct phy_ops const *ops; + + if (!phy) + return 0; + ops = phy_dev_ops(phy->dev); return ops->power_off ? ops->power_off(phy) : 0; } diff --git a/include/generic-phy.h b/include/generic-phy.h index 947c582f68..95caf58341 100644 --- a/include/generic-phy.h +++ b/include/generic-phy.h @@ -270,7 +270,7 @@ static inline int generic_phy_get_by_name(struct udevice *user, const char *phy_ */ static inline bool generic_phy_valid(struct phy *phy) { - return phy->dev != NULL; + return phy && phy->dev; } #endif /*__GENERIC_PHY_H */ From 12e288a8ba7f034eb6430133c6b3d9937b195ba8 Mon Sep 17 00:00:00 2001 From: Michal Sojka Date: Fri, 13 Sep 2019 12:43:12 +0200 Subject: [PATCH 34/39] mkimage: Set correct FDT type and ramdisk architecture in FIT auto mode When running the following command mkimage -f auto -A arm -O linux -T kernel -C none -a 0x8000 -e 0x8000 \ -d zImage -b zynq-microzed.dtb -i initramfs.cpio image.ub the type of fdt subimage is the same as of the main kernel image and the architecture of the initramfs image is not set. Such an image is refused by U-Boot when booting. This commits sets the mentioned attributes, allowing to use the "-f auto" mode in this case instead of writing full .its file. Following is the diff of mkimage output without and with this commit: FIT description: Kernel Image image with one or more FDT blobs Created: Thu Sep 12 23:23:16 2019 Image 0 (kernel-1) Description: Created: Thu Sep 12 23:23:16 2019 Type: Kernel Image Compression: uncompressed Data Size: 4192744 Bytes = 4094.48 KiB = 4.00 MiB Architecture: ARM OS: Linux Load Address: 0x00008000 Entry Point: 0x00008000 Image 1 (fdt-1) Description: zynq-microzed Created: Thu Sep 12 23:23:16 2019 - Type: Kernel Image + Type: Flat Device Tree Compression: uncompressed Data Size: 9398 Bytes = 9.18 KiB = 0.01 MiB Architecture: ARM - OS: Unknown OS - Load Address: unavailable - Entry Point: unavailable Image 2 (ramdisk-1) Description: unavailable Created: Thu Sep 12 23:23:16 2019 Type: RAMDisk Image Compression: Unknown Compression Data Size: 760672 Bytes = 742.84 KiB = 0.73 MiB - Architecture: Unknown Architecture + Architecture: ARM OS: Linux Load Address: unavailable Entry Point: unavailable Default Configuration: 'conf-1' Configuration 0 (conf-1) Description: zynq-microzed Kernel: kernel-1 Init Ramdisk: ramdisk-1 FDT: fdt-1 Loadables: kernel-1 Signed-off-by: Michal Sojka --- tools/fit_image.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/fit_image.c b/tools/fit_image.c index 5aca634b5e..0201cc44d8 100644 --- a/tools/fit_image.c +++ b/tools/fit_image.c @@ -229,6 +229,7 @@ static int fit_write_images(struct image_tool_params *params, char *fdt) for (cont = params->content_head; cont; cont = cont->next) { if (cont->type != IH_TYPE_FLATDT) continue; + typename = genimg_get_type_short_name(cont->type); snprintf(str, sizeof(str), "%s-%d", FIT_FDT_PROP, ++upto); fdt_begin_node(fdt, str); @@ -253,6 +254,8 @@ static int fit_write_images(struct image_tool_params *params, char *fdt) fdt_property_string(fdt, FIT_TYPE_PROP, FIT_RAMDISK_PROP); fdt_property_string(fdt, FIT_OS_PROP, genimg_get_os_short_name(params->os)); + fdt_property_string(fdt, FIT_ARCH_PROP, + genimg_get_arch_short_name(params->arch)); ret = fdt_property_file(params, fdt, FIT_DATA_PROP, params->fit_ramdisk); From 4d579a4394d665b95c5289b5b7c9ce344e07bf18 Mon Sep 17 00:00:00 2001 From: Sam Protsenko Date: Thu, 15 Aug 2019 23:04:02 +0300 Subject: [PATCH 35/39] libavb: Update libavb to current AOSP master Update libavb to commit 5fbb42a189aa in AOSP/master, because new version has support for super partition [1], which we need for implementing Android dynamic partitions. All changes from previous patches for libavb in U-Boot are accounted for in this commit: - commit ecc6f6bea6a2 ("libavb: Handle wrong hashtree_error_mode in avb_append_options()") - commit 897a1d947e7e ("libavb: Update SPDX tag style") - commit d8f9d2af96b3 ("avb2.0: add Android Verified Boot 2.0 library") Tested on X15: ## Android Verified Boot 2.0 version 1.1.0 read_is_device_unlocked not supported yet read_rollback_index not supported yet read_is_device_unlocked not supported yet Verification passed successfully AVB verification OK. Unit test passes: $ ./test/py/test.py --bd sandbox --build -k test_avb test/py/tests/test_android/test_avb.py ss..s. [1] https://android.googlesource.com/platform/external/avb/+/49936b4c0109411fdd38bd4ba3a32a01c40439a9 Signed-off-by: Sam Protsenko Reviewed-by: Eugeniu Rosca Acked-by: Igor Opaniuk --- lib/libavb/avb_cmdline.c | 52 ++- lib/libavb/avb_cmdline.h | 4 +- lib/libavb/avb_descriptor.c | 11 +- lib/libavb/avb_ops.h | 41 +- lib/libavb/avb_sha.h | 12 +- lib/libavb/avb_sha256.c | 36 +- lib/libavb/avb_sha512.c | 22 +- lib/libavb/avb_slot_verify.c | 658 +++++++++++++++++++++++++-------- lib/libavb/avb_slot_verify.h | 59 ++- lib/libavb/avb_sysdeps.h | 8 + lib/libavb/avb_sysdeps_posix.c | 16 +- lib/libavb/avb_vbmeta_image.c | 11 +- 12 files changed, 713 insertions(+), 217 deletions(-) diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c index d246699272..cb5b98e423 100644 --- a/lib/libavb/avb_cmdline.c +++ b/lib/libavb/avb_cmdline.c @@ -39,6 +39,14 @@ char* avb_sub_cmdline(AvbOps* ops, char part_name[AVB_PART_NAME_MAX_SIZE]; char guid_buf[37]; + /* Don't attempt to query the partition guid unless its search string is + * present in the command line. Note: the original cmdline is used here, + * not the replaced one. See b/116010959. + */ + if (avb_strstr(cmdline, replace_str[n]) == NULL) { + continue; + } + if (!avb_str_concat(part_name, sizeof part_name, part_name_str[n], @@ -70,7 +78,15 @@ char* avb_sub_cmdline(AvbOps* ops, } } - avb_assert(ret != NULL); + /* It's possible there is no _PARTUUID for replacement above. + * Duplicate cmdline to ret for additional substitutions below. + */ + if (ret == NULL) { + ret = avb_strdup(cmdline); + if (ret == NULL) { + goto fail; + } + } /* Replace any additional substitutions. */ if (additional_substitutions != NULL) { @@ -198,21 +214,27 @@ static int cmdline_append_hex(AvbSlotVerifyData* slot_data, AvbSlotVerifyResult avb_append_options( AvbOps* ops, + AvbSlotVerifyFlags flags, AvbSlotVerifyData* slot_data, AvbVBMetaImageHeader* toplevel_vbmeta, AvbAlgorithmType algorithm_type, - AvbHashtreeErrorMode hashtree_error_mode) { + AvbHashtreeErrorMode hashtree_error_mode, + AvbHashtreeErrorMode resolved_hashtree_error_mode) { AvbSlotVerifyResult ret; const char* verity_mode; bool is_device_unlocked; AvbIOResult io_ret; - /* Add androidboot.vbmeta.device option. */ - if (!cmdline_append_option(slot_data, - "androidboot.vbmeta.device", - "PARTUUID=$(ANDROID_VBMETA_PARTUUID)")) { - ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; - goto out; + /* Add androidboot.vbmeta.device option... except if not using a vbmeta + * partition since it doesn't make sense in that case. + */ + if (!(flags & AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION)) { + if (!cmdline_append_option(slot_data, + "androidboot.vbmeta.device", + "PARTUUID=$(ANDROID_VBMETA_PARTUUID)")) { + ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; + goto out; + } } /* Add androidboot.vbmeta.avb_version option. */ @@ -304,7 +326,7 @@ AvbSlotVerifyResult avb_append_options( const char* dm_verity_mode; char* new_ret; - switch (hashtree_error_mode) { + switch (resolved_hashtree_error_mode) { case AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE: if (!cmdline_append_option( slot_data, "androidboot.vbmeta.invalidate_on_error", "yes")) { @@ -331,6 +353,11 @@ AvbSlotVerifyResult avb_append_options( verity_mode = "logging"; dm_verity_mode = "ignore_corruption"; break; + case AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO: + // Should never get here because MANAGED_RESTART_AND_EIO is + // remapped by avb_manage_hashtree_error_mode(). + avb_assert_not_reached(); + break; default: ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; goto out; @@ -349,6 +376,13 @@ AvbSlotVerifyResult avb_append_options( ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; goto out; } + if (hashtree_error_mode == AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO) { + if (!cmdline_append_option( + slot_data, "androidboot.veritymode.managed", "yes")) { + ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; + goto out; + } + } ret = AVB_SLOT_VERIFY_RESULT_OK; diff --git a/lib/libavb/avb_cmdline.h b/lib/libavb/avb_cmdline.h index 9af3a99994..96539d84bd 100644 --- a/lib/libavb/avb_cmdline.h +++ b/lib/libavb/avb_cmdline.h @@ -43,10 +43,12 @@ char* avb_sub_cmdline(AvbOps* ops, AvbSlotVerifyResult avb_append_options( AvbOps* ops, + AvbSlotVerifyFlags flags, AvbSlotVerifyData* slot_data, AvbVBMetaImageHeader* toplevel_vbmeta, AvbAlgorithmType algorithm_type, - AvbHashtreeErrorMode hashtree_error_mode); + AvbHashtreeErrorMode hashtree_error_mode, + AvbHashtreeErrorMode resolved_hashtree_error_mode); /* Allocates and initializes a new command line substitution list. Free with * |avb_free_cmdline_subst_list|. diff --git a/lib/libavb/avb_descriptor.c b/lib/libavb/avb_descriptor.c index fb0b305f2c..9f03b9777a 100644 --- a/lib/libavb/avb_descriptor.c +++ b/lib/libavb/avb_descriptor.c @@ -72,7 +72,11 @@ bool avb_descriptor_foreach(const uint8_t* image_data, const AvbDescriptor* dh = (const AvbDescriptor*)p; avb_assert_aligned(dh); uint64_t nb_following = avb_be64toh(dh->num_bytes_following); - uint64_t nb_total = sizeof(AvbDescriptor) + nb_following; + uint64_t nb_total = 0; + if (!avb_safe_add(&nb_total, sizeof(AvbDescriptor), nb_following)) { + avb_error("Invalid descriptor length.\n"); + goto out; + } if ((nb_total & 7) != 0) { avb_error("Invalid descriptor length.\n"); @@ -88,7 +92,10 @@ bool avb_descriptor_foreach(const uint8_t* image_data, goto out; } - p += nb_total; + if (!avb_safe_add_to((uint64_t*)(&p), nb_total)) { + avb_error("Invalid descriptor length.\n"); + goto out; + } } ret = true; diff --git a/lib/libavb/avb_ops.h b/lib/libavb/avb_ops.h index 8bbdc7c31b..6a5c589da8 100644 --- a/lib/libavb/avb_ops.h +++ b/lib/libavb/avb_ops.h @@ -18,6 +18,7 @@ extern "C" { /* Well-known names of named persistent values. */ #define AVB_NPV_PERSISTENT_DIGEST_PREFIX "avb.persistent_digest." +#define AVB_NPV_MANAGED_VERITY_MODE "avb.managed_verity_mode" /* Return codes used for I/O operations. * @@ -171,6 +172,10 @@ struct AvbOps { * * If AVB_IO_RESULT_OK is returned then |out_is_trusted| is set - * true if trusted or false if untrusted. + * + * NOTE: If AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION is passed to + * avb_slot_verify() then this operation is never used. Instead, the + * validate_public_key_for_partition() operation is used */ AvbIOResult (*validate_vbmeta_public_key)(AvbOps* ops, const uint8_t* public_key_data, @@ -231,6 +236,9 @@ struct AvbOps { * (NUL-terminated UTF-8 string). Returns the value in * |out_size_num_bytes|. * + * If the partition doesn't exist the AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION + * error code should be returned. + * * Returns AVB_IO_RESULT_OK on success, otherwise an error code. */ AvbIOResult (*get_size_of_partition)(AvbOps* ops, @@ -253,9 +261,10 @@ struct AvbOps { * AVB_IO_RESULT_ERROR_NO_SUCH_VALUE. If |buffer_size| is smaller than the * size of the stored value, returns AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE. * - * This operation is currently only used to support persistent digests. If a - * device does not use persistent digests this function pointer can be set to - * NULL. + * This operation is currently only used to support persistent digests or the + * AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO hashtree error mode. If a + * device does not use one of these features this function pointer can be set + * to NULL. */ AvbIOResult (*read_persistent_value)(AvbOps* ops, const char* name, @@ -275,14 +284,34 @@ struct AvbOps { * AVB_IO_RESULT_ERROR_NO_SUCH_VALUE. If the |value_size| is not supported, * returns AVB_IO_RESULT_ERROR_INVALID_VALUE_SIZE. * - * This operation is currently only used to support persistent digests. If a - * device does not use persistent digests this function pointer can be set to - * NULL. + * This operation is currently only used to support persistent digests or the + * AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO hashtree error mode. If a + * device does not use one of these features this function pointer can be set + * to NULL. */ AvbIOResult (*write_persistent_value)(AvbOps* ops, const char* name, size_t value_size, const uint8_t* value); + + /* Like validate_vbmeta_public_key() but for when the flag + * AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION is being used. The name of the + * partition to get the public key for is passed in |partition_name|. + * + * Also returns the rollback index location to use for the partition, in + * |out_rollback_index_location|. + * + * Returns AVB_IO_RESULT_OK on success, otherwise an error code. + */ + AvbIOResult (*validate_public_key_for_partition)( + AvbOps* ops, + const char* partition, + const uint8_t* public_key_data, + size_t public_key_length, + const uint8_t* public_key_metadata, + size_t public_key_metadata_length, + bool* out_is_trusted, + uint32_t* out_rollback_index_location); }; #ifdef __cplusplus diff --git a/lib/libavb/avb_sha.h b/lib/libavb/avb_sha.h index 365aaadc2f..f5d02e09f2 100644 --- a/lib/libavb/avb_sha.h +++ b/lib/libavb/avb_sha.h @@ -31,8 +31,8 @@ extern "C" { /* Data structure used for SHA-256. */ typedef struct { uint32_t h[8]; - uint32_t tot_len; - uint32_t len; + uint64_t tot_len; + size_t len; uint8_t block[2 * AVB_SHA256_BLOCK_SIZE]; uint8_t buf[AVB_SHA256_DIGEST_SIZE]; /* Used for storing the final digest. */ } AvbSHA256Ctx; @@ -40,8 +40,8 @@ typedef struct { /* Data structure used for SHA-512. */ typedef struct { uint64_t h[8]; - uint32_t tot_len; - uint32_t len; + uint64_t tot_len; + size_t len; uint8_t block[2 * AVB_SHA512_BLOCK_SIZE]; uint8_t buf[AVB_SHA512_DIGEST_SIZE]; /* Used for storing the final digest. */ } AvbSHA512Ctx; @@ -50,7 +50,7 @@ typedef struct { void avb_sha256_init(AvbSHA256Ctx* ctx); /* Updates the SHA-256 context with |len| bytes from |data|. */ -void avb_sha256_update(AvbSHA256Ctx* ctx, const uint8_t* data, uint32_t len); +void avb_sha256_update(AvbSHA256Ctx* ctx, const uint8_t* data, size_t len); /* Returns the SHA-256 digest. */ uint8_t* avb_sha256_final(AvbSHA256Ctx* ctx) AVB_ATTR_WARN_UNUSED_RESULT; @@ -59,7 +59,7 @@ uint8_t* avb_sha256_final(AvbSHA256Ctx* ctx) AVB_ATTR_WARN_UNUSED_RESULT; void avb_sha512_init(AvbSHA512Ctx* ctx); /* Updates the SHA-512 context with |len| bytes from |data|. */ -void avb_sha512_update(AvbSHA512Ctx* ctx, const uint8_t* data, uint32_t len); +void avb_sha512_update(AvbSHA512Ctx* ctx, const uint8_t* data, size_t len); /* Returns the SHA-512 digest. */ uint8_t* avb_sha512_final(AvbSHA512Ctx* ctx) AVB_ATTR_WARN_UNUSED_RESULT; diff --git a/lib/libavb/avb_sha256.c b/lib/libavb/avb_sha256.c index d24c7015f6..86ecca57b7 100644 --- a/lib/libavb/avb_sha256.c +++ b/lib/libavb/avb_sha256.c @@ -29,6 +29,18 @@ *((str) + 0) = (uint8_t)((x) >> 24); \ } +#define UNPACK64(x, str) \ + { \ + *((str) + 7) = (uint8_t)x; \ + *((str) + 6) = (uint8_t)((uint64_t)x >> 8); \ + *((str) + 5) = (uint8_t)((uint64_t)x >> 16); \ + *((str) + 4) = (uint8_t)((uint64_t)x >> 24); \ + *((str) + 3) = (uint8_t)((uint64_t)x >> 32); \ + *((str) + 2) = (uint8_t)((uint64_t)x >> 40); \ + *((str) + 1) = (uint8_t)((uint64_t)x >> 48); \ + *((str) + 0) = (uint8_t)((uint64_t)x >> 56); \ + } + #define PACK32(str, x) \ { \ *(x) = ((uint32_t) * ((str) + 3)) | ((uint32_t) * ((str) + 2) << 8) | \ @@ -96,18 +108,18 @@ void avb_sha256_init(AvbSHA256Ctx* ctx) { static void SHA256_transform(AvbSHA256Ctx* ctx, const uint8_t* message, - unsigned int block_nb) { + size_t block_nb) { uint32_t w[64]; uint32_t wv[8]; uint32_t t1, t2; const unsigned char* sub_block; - int i; + size_t i; #ifndef UNROLL_LOOPS - int j; + size_t j; #endif - for (i = 0; i < (int)block_nb; i++) { + for (i = 0; i < block_nb; i++) { sub_block = message + (i << 6); #ifndef UNROLL_LOOPS @@ -293,9 +305,9 @@ static void SHA256_transform(AvbSHA256Ctx* ctx, } } -void avb_sha256_update(AvbSHA256Ctx* ctx, const uint8_t* data, uint32_t len) { - unsigned int block_nb; - unsigned int new_len, rem_len, tmp_len; +void avb_sha256_update(AvbSHA256Ctx* ctx, const uint8_t* data, size_t len) { + size_t block_nb; + size_t new_len, rem_len, tmp_len; const uint8_t* shifted_data; tmp_len = AVB_SHA256_BLOCK_SIZE - ctx->len; @@ -325,11 +337,11 @@ void avb_sha256_update(AvbSHA256Ctx* ctx, const uint8_t* data, uint32_t len) { } uint8_t* avb_sha256_final(AvbSHA256Ctx* ctx) { - unsigned int block_nb; - unsigned int pm_len; - unsigned int len_b; + size_t block_nb; + size_t pm_len; + uint64_t len_b; #ifndef UNROLL_LOOPS - int i; + size_t i; #endif block_nb = @@ -340,7 +352,7 @@ uint8_t* avb_sha256_final(AvbSHA256Ctx* ctx) { avb_memset(ctx->block + ctx->len, 0, pm_len - ctx->len); ctx->block[ctx->len] = 0x80; - UNPACK32(len_b, ctx->block + pm_len - 4); + UNPACK64(len_b, ctx->block + pm_len - 8); SHA256_transform(ctx, ctx->block, block_nb); diff --git a/lib/libavb/avb_sha512.c b/lib/libavb/avb_sha512.c index a5e7297aa7..b19054fc74 100644 --- a/lib/libavb/avb_sha512.c +++ b/lib/libavb/avb_sha512.c @@ -127,14 +127,14 @@ void avb_sha512_init(AvbSHA512Ctx* ctx) { static void SHA512_transform(AvbSHA512Ctx* ctx, const uint8_t* message, - unsigned int block_nb) { + size_t block_nb) { uint64_t w[80]; uint64_t wv[8]; uint64_t t1, t2; const uint8_t* sub_block; - int i, j; + size_t i, j; - for (i = 0; i < (int)block_nb; i++) { + for (i = 0; i < block_nb; i++) { sub_block = message + (i << 7); #ifdef UNROLL_LOOPS_SHA512 @@ -291,9 +291,9 @@ static void SHA512_transform(AvbSHA512Ctx* ctx, } } -void avb_sha512_update(AvbSHA512Ctx* ctx, const uint8_t* data, uint32_t len) { - unsigned int block_nb; - unsigned int new_len, rem_len, tmp_len; +void avb_sha512_update(AvbSHA512Ctx* ctx, const uint8_t* data, size_t len) { + size_t block_nb; + size_t new_len, rem_len, tmp_len; const uint8_t* shifted_data; tmp_len = AVB_SHA512_BLOCK_SIZE - ctx->len; @@ -323,12 +323,12 @@ void avb_sha512_update(AvbSHA512Ctx* ctx, const uint8_t* data, uint32_t len) { } uint8_t* avb_sha512_final(AvbSHA512Ctx* ctx) { - unsigned int block_nb; - unsigned int pm_len; - unsigned int len_b; + size_t block_nb; + size_t pm_len; + uint64_t len_b; #ifndef UNROLL_LOOPS_SHA512 - int i; + size_t i; #endif block_nb = @@ -339,7 +339,7 @@ uint8_t* avb_sha512_final(AvbSHA512Ctx* ctx) { avb_memset(ctx->block + ctx->len, 0, pm_len - ctx->len); ctx->block[ctx->len] = 0x80; - UNPACK32(len_b, ctx->block + pm_len - 4); + UNPACK64(len_b, ctx->block + pm_len - 8); SHA512_transform(ctx, ctx->block, block_nb); diff --git a/lib/libavb/avb_slot_verify.c b/lib/libavb/avb_slot_verify.c index a941850d93..5d400b38aa 100644 --- a/lib/libavb/avb_slot_verify.c +++ b/lib/libavb/avb_slot_verify.c @@ -24,6 +24,14 @@ /* Maximum size of a vbmeta image - 64 KiB. */ #define VBMETA_MAX_SIZE (64 * 1024) +static AvbSlotVerifyResult initialize_persistent_digest( + AvbOps* ops, + const char* part_name, + const char* persistent_value_name, + size_t digest_size, + const uint8_t* initial_digest, + uint8_t* out_digest); + /* Helper function to see if we should continue with verification in * allow_verification_error=true mode if something goes wrong. See the * comments for the avb_slot_verify() function for more information. @@ -114,9 +122,26 @@ static AvbSlotVerifyResult load_full_partition(AvbOps* ops, return AVB_SLOT_VERIFY_RESULT_OK; } +/* Reads a persistent digest stored as a named persistent value corresponding to + * the given |part_name|. The value is returned in |out_digest| which must point + * to |expected_digest_size| bytes. If there is no digest stored for |part_name| + * it can be initialized by providing a non-NULL |initial_digest| of length + * |expected_digest_size|. This automatic initialization will only occur if the + * device is currently locked. The |initial_digest| may be NULL. + * + * Returns AVB_SLOT_VERIFY_RESULT_OK on success, otherwise returns an + * AVB_SLOT_VERIFY_RESULT_ERROR_* error code. + * + * If the value does not exist, is not supported, or is not populated, and + * |initial_digest| is NULL, returns + * AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA. If |expected_digest_size| does + * not match the stored digest size, also returns + * AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA. + */ static AvbSlotVerifyResult read_persistent_digest(AvbOps* ops, const char* part_name, size_t expected_digest_size, + const uint8_t* initial_digest, uint8_t* out_digest) { char* persistent_value_name = NULL; AvbIOResult io_ret = AVB_IO_RESULT_OK; @@ -131,30 +156,106 @@ static AvbSlotVerifyResult read_persistent_digest(AvbOps* ops, if (persistent_value_name == NULL) { return AVB_SLOT_VERIFY_RESULT_ERROR_OOM; } + io_ret = ops->read_persistent_value(ops, persistent_value_name, expected_digest_size, out_digest, &stored_digest_size); + + // If no such named persistent value exists and an initial digest value was + // given, initialize the named persistent value with the given digest. If + // initialized successfully, this will recurse into this function but with a + // NULL initial_digest. + if (io_ret == AVB_IO_RESULT_ERROR_NO_SUCH_VALUE && initial_digest) { + AvbSlotVerifyResult ret = + initialize_persistent_digest(ops, + part_name, + persistent_value_name, + expected_digest_size, + initial_digest, + out_digest); + avb_free(persistent_value_name); + return ret; + } avb_free(persistent_value_name); + if (io_ret == AVB_IO_RESULT_ERROR_OOM) { return AVB_SLOT_VERIFY_RESULT_ERROR_OOM; } else if (io_ret == AVB_IO_RESULT_ERROR_NO_SUCH_VALUE) { + // Treat a missing persistent value as a verification error, which is + // ignoreable, rather than a metadata error which is not. avb_errorv(part_name, ": Persistent digest does not exist.\n", NULL); - return AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA; + return AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION; } else if (io_ret == AVB_IO_RESULT_ERROR_INVALID_VALUE_SIZE || - io_ret == AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE || - expected_digest_size != stored_digest_size) { + io_ret == AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE) { avb_errorv( part_name, ": Persistent digest is not of expected size.\n", NULL); return AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA; } else if (io_ret != AVB_IO_RESULT_OK) { avb_errorv(part_name, ": Error reading persistent digest.\n", NULL); return AVB_SLOT_VERIFY_RESULT_ERROR_IO; + } else if (expected_digest_size != stored_digest_size) { + avb_errorv( + part_name, ": Persistent digest is not of expected size.\n", NULL); + return AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA; } return AVB_SLOT_VERIFY_RESULT_OK; } +static AvbSlotVerifyResult initialize_persistent_digest( + AvbOps* ops, + const char* part_name, + const char* persistent_value_name, + size_t digest_size, + const uint8_t* initial_digest, + uint8_t* out_digest) { + AvbSlotVerifyResult ret; + AvbIOResult io_ret = AVB_IO_RESULT_OK; + bool is_device_unlocked = true; + + io_ret = ops->read_is_device_unlocked(ops, &is_device_unlocked); + if (io_ret == AVB_IO_RESULT_ERROR_OOM) { + return AVB_SLOT_VERIFY_RESULT_ERROR_OOM; + } else if (io_ret != AVB_IO_RESULT_OK) { + avb_error("Error getting device lock state.\n"); + return AVB_SLOT_VERIFY_RESULT_ERROR_IO; + } + + if (is_device_unlocked) { + avb_debugv(part_name, + ": Digest does not exist, device unlocked so not initializing " + "digest.\n", + NULL); + return AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION; + } + + // Device locked; initialize digest with given initial value. + avb_debugv(part_name, + ": Digest does not exist, initializing persistent digest.\n", + NULL); + io_ret = ops->write_persistent_value( + ops, persistent_value_name, digest_size, initial_digest); + if (io_ret == AVB_IO_RESULT_ERROR_OOM) { + return AVB_SLOT_VERIFY_RESULT_ERROR_OOM; + } else if (io_ret != AVB_IO_RESULT_OK) { + avb_errorv(part_name, ": Error initializing persistent digest.\n", NULL); + return AVB_SLOT_VERIFY_RESULT_ERROR_IO; + } + + // To ensure that the digest value was written successfully - and avoid a + // scenario where the digest is simply 'initialized' on every verify - recurse + // into read_persistent_digest to read back the written value. The NULL + // initial_digest ensures that this will not recurse again. + ret = read_persistent_digest(ops, part_name, digest_size, NULL, out_digest); + if (ret != AVB_SLOT_VERIFY_RESULT_OK) { + avb_errorv(part_name, + ": Reading back initialized persistent digest failed!\n", + NULL); + } + return ret; +} + static AvbSlotVerifyResult load_and_verify_hash_partition( AvbOps* ops, const char* const* requested_partitions, @@ -248,24 +349,16 @@ static AvbSlotVerifyResult load_and_verify_hash_partition( */ image_size = hash_desc.image_size; if (allow_verification_error) { - if (ops->get_size_of_partition == NULL) { - avb_errorv(part_name, - ": The get_size_of_partition() operation is " - "not implemented so we may not load the entire partition. " - "Please implement.", - NULL); - } else { - io_ret = ops->get_size_of_partition(ops, part_name, &image_size); - if (io_ret == AVB_IO_RESULT_ERROR_OOM) { - ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; - goto out; - } else if (io_ret != AVB_IO_RESULT_OK) { - avb_errorv(part_name, ": Error determining partition size.\n", NULL); - ret = AVB_SLOT_VERIFY_RESULT_ERROR_IO; - goto out; - } - avb_debugv(part_name, ": Loading entire partition.\n", NULL); + io_ret = ops->get_size_of_partition(ops, part_name, &image_size); + if (io_ret == AVB_IO_RESULT_ERROR_OOM) { + ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; + goto out; + } else if (io_ret != AVB_IO_RESULT_OK) { + avb_errorv(part_name, ": Error determining partition size.\n", NULL); + ret = AVB_SLOT_VERIFY_RESULT_ERROR_IO; + goto out; } + avb_debugv(part_name, ": Loading entire partition.\n", NULL); } ret = load_full_partition( @@ -273,19 +366,27 @@ static AvbSlotVerifyResult load_and_verify_hash_partition( if (ret != AVB_SLOT_VERIFY_RESULT_OK) { goto out; } - + // Although only one of the type might be used, we have to defined the + // structure here so that they would live outside the 'if/else' scope to be + // used later. + AvbSHA256Ctx sha256_ctx; + AvbSHA512Ctx sha512_ctx; + size_t image_size_to_hash = hash_desc.image_size; + // If we allow verification error and the whole partition is smaller than + // image size in hash descriptor, we just hash the whole partition. + if (image_size_to_hash > image_size) { + image_size_to_hash = image_size; + } if (avb_strcmp((const char*)hash_desc.hash_algorithm, "sha256") == 0) { - AvbSHA256Ctx sha256_ctx; avb_sha256_init(&sha256_ctx); avb_sha256_update(&sha256_ctx, desc_salt, hash_desc.salt_len); - avb_sha256_update(&sha256_ctx, image_buf, hash_desc.image_size); + avb_sha256_update(&sha256_ctx, image_buf, image_size_to_hash); digest = avb_sha256_final(&sha256_ctx); digest_len = AVB_SHA256_DIGEST_SIZE; } else if (avb_strcmp((const char*)hash_desc.hash_algorithm, "sha512") == 0) { - AvbSHA512Ctx sha512_ctx; avb_sha512_init(&sha512_ctx); avb_sha512_update(&sha512_ctx, desc_salt, hash_desc.salt_len); - avb_sha512_update(&sha512_ctx, image_buf, hash_desc.image_size); + avb_sha512_update(&sha512_ctx, image_buf, image_size_to_hash); digest = avb_sha512_final(&sha512_ctx); digest_len = AVB_SHA512_DIGEST_SIZE; } else { @@ -295,18 +396,21 @@ static AvbSlotVerifyResult load_and_verify_hash_partition( } if (hash_desc.digest_len == 0) { - // Expect a match to a persistent digest. + /* Expect a match to a persistent digest. */ avb_debugv(part_name, ": No digest, using persistent digest.\n", NULL); expected_digest_len = digest_len; expected_digest = expected_digest_buf; avb_assert(expected_digest_len <= sizeof(expected_digest_buf)); - ret = - read_persistent_digest(ops, part_name, digest_len, expected_digest_buf); + /* Pass |digest| as the |initial_digest| so devices not yet initialized get + * initialized to the current partition digest. + */ + ret = read_persistent_digest( + ops, part_name, digest_len, digest, expected_digest_buf); if (ret != AVB_SLOT_VERIFY_RESULT_OK) { goto out; } } else { - // Expect a match to the digest in the descriptor. + /* Expect a match to the digest in the descriptor. */ expected_digest_len = hash_desc.digest_len; expected_digest = desc_digest; } @@ -365,12 +469,6 @@ static AvbSlotVerifyResult load_requested_partitions( bool image_preloaded = false; size_t n; - if (ops->get_size_of_partition == NULL) { - avb_error("get_size_of_partition() not implemented.\n"); - ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; - goto out; - } - for (n = 0; requested_partitions[n] != NULL; n++) { char part_name[AVB_PART_NAME_MAX_SIZE]; AvbIOResult io_ret; @@ -441,6 +539,7 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( AvbOps* ops, const char* const* requested_partitions, const char* ab_suffix, + AvbSlotVerifyFlags flags, bool allow_verification_error, AvbVBMetaImageFlags toplevel_vbmeta_flags, int rollback_index_location, @@ -467,7 +566,7 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( size_t num_descriptors; size_t n; bool is_main_vbmeta; - bool is_vbmeta_partition; + bool look_for_vbmeta_footer; AvbVBMetaData* vbmeta_image_data = NULL; ret = AVB_SLOT_VERIFY_RESULT_OK; @@ -478,8 +577,20 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( * rollback_index_location to determine whether we're the main * vbmeta struct. */ - is_main_vbmeta = (rollback_index_location == 0); - is_vbmeta_partition = (avb_strcmp(partition_name, "vbmeta") == 0); + is_main_vbmeta = false; + if (rollback_index_location == 0) { + if ((flags & AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION) == 0) { + is_main_vbmeta = true; + } + } + + /* Don't use footers for vbmeta partitions ('vbmeta' or + * 'vbmeta_'). + */ + look_for_vbmeta_footer = true; + if (avb_strncmp(partition_name, "vbmeta", avb_strlen("vbmeta")) == 0) { + look_for_vbmeta_footer = false; + } if (!avb_validate_utf8((const uint8_t*)partition_name, partition_name_len)) { avb_error("Partition name is not valid UTF-8.\n"); @@ -487,7 +598,7 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( goto out; } - /* Construct full partition name. */ + /* Construct full partition name e.g. system_a. */ if (!avb_str_concat(full_partition_name, sizeof full_partition_name, partition_name, @@ -499,19 +610,15 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( goto out; } - avb_debugv("Loading vbmeta struct from partition '", - full_partition_name, - "'.\n", - NULL); - - /* If we're loading from the main vbmeta partition, the vbmeta - * struct is in the beginning. Otherwise we have to locate it via a - * footer. + /* If we're loading from the main vbmeta partition, the vbmeta struct is in + * the beginning. Otherwise we may have to locate it via a footer... if no + * footer is found, we look in the beginning to support e.g. vbmeta_ + * partitions holding data for e.g. super partitions (b/80195851 for + * rationale). */ - if (is_vbmeta_partition) { - vbmeta_offset = 0; - vbmeta_size = VBMETA_MAX_SIZE; - } else { + vbmeta_offset = 0; + vbmeta_size = VBMETA_MAX_SIZE; + if (look_for_vbmeta_footer) { uint8_t footer_buf[AVB_FOOTER_SIZE]; size_t footer_num_read; AvbFooter footer; @@ -534,21 +641,17 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( if (!avb_footer_validate_and_byteswap((const AvbFooter*)footer_buf, &footer)) { - avb_errorv(full_partition_name, ": Error validating footer.\n", NULL); - ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA; - goto out; + avb_debugv(full_partition_name, ": No footer detected.\n", NULL); + } else { + /* Basic footer sanity check since the data is untrusted. */ + if (footer.vbmeta_size > VBMETA_MAX_SIZE) { + avb_errorv( + full_partition_name, ": Invalid vbmeta size in footer.\n", NULL); + } else { + vbmeta_offset = footer.vbmeta_offset; + vbmeta_size = footer.vbmeta_size; + } } - - /* Basic footer sanity check since the data is untrusted. */ - if (footer.vbmeta_size > VBMETA_MAX_SIZE) { - avb_errorv( - full_partition_name, ": Invalid vbmeta size in footer.\n", NULL); - ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA; - goto out; - } - - vbmeta_offset = footer.vbmeta_offset; - vbmeta_size = footer.vbmeta_size; } vbmeta_buf = avb_malloc(vbmeta_size); @@ -557,6 +660,18 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( goto out; } + if (vbmeta_offset != 0) { + avb_debugv("Loading vbmeta struct in footer from partition '", + full_partition_name, + "'.\n", + NULL); + } else { + avb_debugv("Loading vbmeta struct from partition '", + full_partition_name, + "'.\n", + NULL); + } + io_ret = ops->read_from_partition(ops, full_partition_name, vbmeta_offset, @@ -571,13 +686,14 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( * go try to get it from the boot partition instead. */ if (is_main_vbmeta && io_ret == AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION && - is_vbmeta_partition) { + !look_for_vbmeta_footer) { avb_debugv(full_partition_name, ": No such partition. Trying 'boot' instead.\n", NULL); ret = load_and_verify_vbmeta(ops, requested_partitions, ab_suffix, + flags, allow_verification_error, 0 /* toplevel_vbmeta_flags */, 0 /* rollback_index_location */, @@ -655,6 +771,8 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( } } + uint32_t rollback_index_location_to_use = rollback_index_location; + /* Check if key used to make signature matches what is expected. */ if (pk_data != NULL) { if (expected_public_key != NULL) { @@ -682,9 +800,27 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( pk_metadata_len = vbmeta_header.public_key_metadata_size; } - avb_assert(is_main_vbmeta); - io_ret = ops->validate_vbmeta_public_key( - ops, pk_data, pk_len, pk_metadata, pk_metadata_len, &key_is_trusted); + // If we're not using a vbmeta partition, need to use another AvbOps... + if (flags & AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION) { + io_ret = ops->validate_public_key_for_partition( + ops, + full_partition_name, + pk_data, + pk_len, + pk_metadata, + pk_metadata_len, + &key_is_trusted, + &rollback_index_location_to_use); + } else { + avb_assert(is_main_vbmeta); + io_ret = ops->validate_vbmeta_public_key(ops, + pk_data, + pk_len, + pk_metadata, + pk_metadata_len, + &key_is_trusted); + } + if (io_ret == AVB_IO_RESULT_ERROR_OOM) { ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; goto out; @@ -709,7 +845,7 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( /* Check rollback index. */ io_ret = ops->read_rollback_index( - ops, rollback_index_location, &stored_rollback_index); + ops, rollback_index_location_to_use, &stored_rollback_index); if (io_ret == AVB_IO_RESULT_ERROR_OOM) { ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; goto out; @@ -735,7 +871,9 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( if (is_main_vbmeta) { avb_assert(slot_data->num_vbmeta_images == 0); } else { - avb_assert(slot_data->num_vbmeta_images > 0); + if (!(flags & AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION)) { + avb_assert(slot_data->num_vbmeta_images > 0); + } } if (slot_data->num_vbmeta_images == MAX_NUMBER_OF_VBMETA_IMAGES) { avb_errorv(full_partition_name, ": Too many vbmeta images.\n", NULL); @@ -859,6 +997,7 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( load_and_verify_vbmeta(ops, requested_partitions, ab_suffix, + flags, allow_verification_error, toplevel_vbmeta_flags, chain_desc.rollback_index_location, @@ -1019,7 +1158,11 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( goto out; } - ret = read_persistent_digest(ops, part_name, digest_len, digest_buf); + ret = read_persistent_digest(ops, + part_name, + digest_len, + NULL /* initial_digest */, + digest_buf); if (ret != AVB_SLOT_VERIFY_RESULT_OK) { goto out; } @@ -1043,7 +1186,8 @@ static AvbSlotVerifyResult load_and_verify_vbmeta( } } - if (rollback_index_location >= AVB_MAX_NUMBER_OF_ROLLBACK_INDEX_LOCATIONS) { + if (rollback_index_location < 0 || + rollback_index_location >= AVB_MAX_NUMBER_OF_ROLLBACK_INDEX_LOCATIONS) { avb_errorv( full_partition_name, ": Invalid rollback_index_location.\n", NULL); ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA; @@ -1072,6 +1216,130 @@ out: return ret; } +static AvbIOResult avb_manage_hashtree_error_mode( + AvbOps* ops, + AvbSlotVerifyFlags flags, + AvbSlotVerifyData* data, + AvbHashtreeErrorMode* out_hashtree_error_mode) { + AvbHashtreeErrorMode ret = AVB_HASHTREE_ERROR_MODE_RESTART; + AvbIOResult io_ret = AVB_IO_RESULT_OK; + uint8_t vbmeta_digest_sha256[AVB_SHA256_DIGEST_SIZE]; + uint8_t stored_vbmeta_digest_sha256[AVB_SHA256_DIGEST_SIZE]; + size_t num_bytes_read; + + avb_assert(out_hashtree_error_mode != NULL); + avb_assert(ops->read_persistent_value != NULL); + avb_assert(ops->write_persistent_value != NULL); + + // If we're rebooting because of dm-verity corruption, make a note of + // the vbmeta hash so we can stay in 'eio' mode until things change. + if (flags & AVB_SLOT_VERIFY_FLAGS_RESTART_CAUSED_BY_HASHTREE_CORRUPTION) { + avb_debug( + "Rebooting because of dm-verity corruption - " + "recording OS instance and using 'eio' mode.\n"); + avb_slot_verify_data_calculate_vbmeta_digest( + data, AVB_DIGEST_TYPE_SHA256, vbmeta_digest_sha256); + io_ret = ops->write_persistent_value(ops, + AVB_NPV_MANAGED_VERITY_MODE, + AVB_SHA256_DIGEST_SIZE, + vbmeta_digest_sha256); + if (io_ret != AVB_IO_RESULT_OK) { + avb_error("Error writing to " AVB_NPV_MANAGED_VERITY_MODE ".\n"); + goto out; + } + ret = AVB_HASHTREE_ERROR_MODE_EIO; + io_ret = AVB_IO_RESULT_OK; + goto out; + } + + // See if we're in 'eio' mode. + io_ret = ops->read_persistent_value(ops, + AVB_NPV_MANAGED_VERITY_MODE, + AVB_SHA256_DIGEST_SIZE, + stored_vbmeta_digest_sha256, + &num_bytes_read); + if (io_ret == AVB_IO_RESULT_ERROR_NO_SUCH_VALUE || + (io_ret == AVB_IO_RESULT_OK && num_bytes_read == 0)) { + // This is the usual case ('eio' mode not set). + avb_debug("No dm-verity corruption - using in 'restart' mode.\n"); + ret = AVB_HASHTREE_ERROR_MODE_RESTART; + io_ret = AVB_IO_RESULT_OK; + goto out; + } else if (io_ret != AVB_IO_RESULT_OK) { + avb_error("Error reading from " AVB_NPV_MANAGED_VERITY_MODE ".\n"); + goto out; + } + if (num_bytes_read != AVB_SHA256_DIGEST_SIZE) { + avb_error( + "Unexpected number of bytes read from " AVB_NPV_MANAGED_VERITY_MODE + ".\n"); + io_ret = AVB_IO_RESULT_ERROR_IO; + goto out; + } + + // OK, so we're currently in 'eio' mode and the vbmeta digest of the OS + // that caused this is in |stored_vbmeta_digest_sha256| ... now see if + // the OS we're dealing with now is the same. + avb_slot_verify_data_calculate_vbmeta_digest( + data, AVB_DIGEST_TYPE_SHA256, vbmeta_digest_sha256); + if (avb_memcmp(vbmeta_digest_sha256, + stored_vbmeta_digest_sha256, + AVB_SHA256_DIGEST_SIZE) == 0) { + // It's the same so we're still in 'eio' mode. + avb_debug("Same OS instance detected - staying in 'eio' mode.\n"); + ret = AVB_HASHTREE_ERROR_MODE_EIO; + io_ret = AVB_IO_RESULT_OK; + } else { + // It did change! + avb_debug( + "New OS instance detected - changing from 'eio' to 'restart' mode.\n"); + io_ret = + ops->write_persistent_value(ops, + AVB_NPV_MANAGED_VERITY_MODE, + 0, // This clears the persistent property. + vbmeta_digest_sha256); + if (io_ret != AVB_IO_RESULT_OK) { + avb_error("Error clearing " AVB_NPV_MANAGED_VERITY_MODE ".\n"); + goto out; + } + ret = AVB_HASHTREE_ERROR_MODE_RESTART; + io_ret = AVB_IO_RESULT_OK; + } + +out: + *out_hashtree_error_mode = ret; + return io_ret; +} + +static bool has_system_partition(AvbOps* ops, const char* ab_suffix) { + char part_name[AVB_PART_NAME_MAX_SIZE]; + char* system_part_name = "system"; + char guid_buf[37]; + AvbIOResult io_ret; + + if (!avb_str_concat(part_name, + sizeof part_name, + system_part_name, + avb_strlen(system_part_name), + ab_suffix, + avb_strlen(ab_suffix))) { + avb_error("System partition name and suffix does not fit.\n"); + return false; + } + + io_ret = ops->get_unique_guid_for_partition( + ops, part_name, guid_buf, sizeof guid_buf); + if (io_ret == AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION) { + avb_debug("No system partition.\n"); + return false; + } else if (io_ret != AVB_IO_RESULT_OK) { + avb_error("Error getting unique GUID for system partition.\n"); + return false; + } + + return true; +} + AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, const char* const* requested_partitions, const char* ab_suffix, @@ -1087,14 +1355,10 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, (flags & AVB_SLOT_VERIFY_FLAGS_ALLOW_VERIFICATION_ERROR); AvbCmdlineSubstList* additional_cmdline_subst = NULL; - /* Fail early if we're missing the AvbOps needed for slot verification. - * - * For now, handle get_size_of_partition() not being implemented. In - * a later release we may change that. - */ + /* Fail early if we're missing the AvbOps needed for slot verification. */ avb_assert(ops->read_is_device_unlocked != NULL); avb_assert(ops->read_from_partition != NULL); - avb_assert(ops->validate_vbmeta_public_key != NULL); + avb_assert(ops->get_size_of_partition != NULL); avb_assert(ops->read_rollback_index != NULL); avb_assert(ops->get_unique_guid_for_partition != NULL); @@ -1112,6 +1376,36 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, goto fail; } + /* Make sure passed-in AvbOps support persistent values if + * asking for libavb to manage verity state. + */ + if (hashtree_error_mode == AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO) { + if (ops->read_persistent_value == NULL || + ops->write_persistent_value == NULL) { + avb_error( + "Persistent values required for " + "AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO " + "but are not implemented in given AvbOps.\n"); + ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; + goto fail; + } + } + + /* Make sure passed-in AvbOps support verifying public keys and getting + * rollback index location if not using a vbmeta partition. + */ + if (flags & AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION) { + if (ops->validate_public_key_for_partition == NULL) { + avb_error( + "AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION was passed but the " + "validate_public_key_for_partition() operation isn't implemented.\n"); + ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; + goto fail; + } + } else { + avb_assert(ops->validate_vbmeta_public_key != NULL); + } + slot_data = avb_calloc(sizeof(AvbSlotVerifyData)); if (slot_data == NULL) { ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; @@ -1136,97 +1430,161 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, goto fail; } - ret = load_and_verify_vbmeta(ops, - requested_partitions, - ab_suffix, - allow_verification_error, - 0 /* toplevel_vbmeta_flags */, - 0 /* rollback_index_location */, - "vbmeta", - avb_strlen("vbmeta"), - NULL /* expected_public_key */, - 0 /* expected_public_key_length */, - slot_data, - &algorithm_type, - additional_cmdline_subst); - if (!allow_verification_error && ret != AVB_SLOT_VERIFY_RESULT_OK) { + if (flags & AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION) { + if (requested_partitions == NULL || requested_partitions[0] == NULL) { + avb_fatal( + "Requested partitions cannot be empty when using " + "AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION"); + ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; + goto fail; + } + + /* No vbmeta partition, go through each of the requested partitions... */ + for (size_t n = 0; requested_partitions[n] != NULL; n++) { + ret = load_and_verify_vbmeta(ops, + requested_partitions, + ab_suffix, + flags, + allow_verification_error, + 0 /* toplevel_vbmeta_flags */, + 0 /* rollback_index_location */, + requested_partitions[n], + avb_strlen(requested_partitions[n]), + NULL /* expected_public_key */, + 0 /* expected_public_key_length */, + slot_data, + &algorithm_type, + additional_cmdline_subst); + if (!allow_verification_error && ret != AVB_SLOT_VERIFY_RESULT_OK) { + goto fail; + } + } + + } else { + /* Usual path, load "vbmeta"... */ + ret = load_and_verify_vbmeta(ops, + requested_partitions, + ab_suffix, + flags, + allow_verification_error, + 0 /* toplevel_vbmeta_flags */, + 0 /* rollback_index_location */, + "vbmeta", + avb_strlen("vbmeta"), + NULL /* expected_public_key */, + 0 /* expected_public_key_length */, + slot_data, + &algorithm_type, + additional_cmdline_subst); + if (!allow_verification_error && ret != AVB_SLOT_VERIFY_RESULT_OK) { + goto fail; + } + } + + if (!result_should_continue(ret)) { goto fail; } /* If things check out, mangle the kernel command-line as needed. */ - if (result_should_continue(ret)) { + if (!(flags & AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION)) { if (avb_strcmp(slot_data->vbmeta_images[0].partition_name, "vbmeta") != 0) { avb_assert( avb_strcmp(slot_data->vbmeta_images[0].partition_name, "boot") == 0); using_boot_for_vbmeta = true; } + } - /* Byteswap top-level vbmeta header since we'll need it below. */ - avb_vbmeta_image_header_to_host_byte_order( - (const AvbVBMetaImageHeader*)slot_data->vbmeta_images[0].vbmeta_data, - &toplevel_vbmeta); + /* Byteswap top-level vbmeta header since we'll need it below. */ + avb_vbmeta_image_header_to_host_byte_order( + (const AvbVBMetaImageHeader*)slot_data->vbmeta_images[0].vbmeta_data, + &toplevel_vbmeta); - /* Fill in |ab_suffix| field. */ - slot_data->ab_suffix = avb_strdup(ab_suffix); - if (slot_data->ab_suffix == NULL) { + /* Fill in |ab_suffix| field. */ + slot_data->ab_suffix = avb_strdup(ab_suffix); + if (slot_data->ab_suffix == NULL) { + ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; + goto fail; + } + + /* If verification is disabled, we are done ... we specifically + * don't want to add any androidboot.* options since verification + * is disabled. + */ + if (toplevel_vbmeta.flags & AVB_VBMETA_IMAGE_FLAGS_VERIFICATION_DISABLED) { + /* Since verification is disabled we didn't process any + * descriptors and thus there's no cmdline... so set root= such + * that the system partition is mounted. + */ + avb_assert(slot_data->cmdline == NULL); + // Devices with dynamic partitions won't have system partition. + // Instead, it has a large super partition to accommodate *.img files. + // See b/119551429 for details. + if (has_system_partition(ops, ab_suffix)) { + slot_data->cmdline = + avb_strdup("root=PARTUUID=$(ANDROID_SYSTEM_PARTUUID)"); + } else { + // The |cmdline| field should be a NUL-terminated string. + slot_data->cmdline = avb_strdup(""); + } + if (slot_data->cmdline == NULL) { ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; goto fail; } + } else { + /* If requested, manage dm-verity mode... */ + AvbHashtreeErrorMode resolved_hashtree_error_mode = hashtree_error_mode; + if (hashtree_error_mode == + AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO) { + AvbIOResult io_ret; + io_ret = avb_manage_hashtree_error_mode( + ops, flags, slot_data, &resolved_hashtree_error_mode); + if (io_ret != AVB_IO_RESULT_OK) { + ret = AVB_SLOT_VERIFY_RESULT_ERROR_IO; + if (io_ret == AVB_IO_RESULT_ERROR_OOM) { + ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; + } + goto fail; + } + } + slot_data->resolved_hashtree_error_mode = resolved_hashtree_error_mode; - /* If verification is disabled, we are done ... we specifically - * don't want to add any androidboot.* options since verification - * is disabled. - */ - if (toplevel_vbmeta.flags & AVB_VBMETA_IMAGE_FLAGS_VERIFICATION_DISABLED) { - /* Since verification is disabled we didn't process any - * descriptors and thus there's no cmdline... so set root= such - * that the system partition is mounted. - */ - avb_assert(slot_data->cmdline == NULL); - slot_data->cmdline = - avb_strdup("root=PARTUUID=$(ANDROID_SYSTEM_PARTUUID)"); - if (slot_data->cmdline == NULL) { + /* Add options... */ + AvbSlotVerifyResult sub_ret; + sub_ret = avb_append_options(ops, + flags, + slot_data, + &toplevel_vbmeta, + algorithm_type, + hashtree_error_mode, + resolved_hashtree_error_mode); + if (sub_ret != AVB_SLOT_VERIFY_RESULT_OK) { + ret = sub_ret; + goto fail; + } + } + + /* Substitute $(ANDROID_SYSTEM_PARTUUID) and friends. */ + if (slot_data->cmdline != NULL && avb_strlen(slot_data->cmdline) != 0) { + char* new_cmdline; + new_cmdline = avb_sub_cmdline(ops, + slot_data->cmdline, + ab_suffix, + using_boot_for_vbmeta, + additional_cmdline_subst); + if (new_cmdline != slot_data->cmdline) { + if (new_cmdline == NULL) { ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; goto fail; } - } else { - /* Add options - any failure in avb_append_options() is either an - * I/O or OOM error. - */ - AvbSlotVerifyResult sub_ret = avb_append_options(ops, - slot_data, - &toplevel_vbmeta, - algorithm_type, - hashtree_error_mode); - if (sub_ret != AVB_SLOT_VERIFY_RESULT_OK) { - ret = sub_ret; - goto fail; - } + avb_free(slot_data->cmdline); + slot_data->cmdline = new_cmdline; } + } - /* Substitute $(ANDROID_SYSTEM_PARTUUID) and friends. */ - if (slot_data->cmdline != NULL) { - char* new_cmdline; - new_cmdline = avb_sub_cmdline(ops, - slot_data->cmdline, - ab_suffix, - using_boot_for_vbmeta, - additional_cmdline_subst); - if (new_cmdline != slot_data->cmdline) { - if (new_cmdline == NULL) { - ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM; - goto fail; - } - avb_free(slot_data->cmdline); - slot_data->cmdline = new_cmdline; - } - } - - if (out_data != NULL) { - *out_data = slot_data; - } else { - avb_slot_verify_data_free(slot_data); - } + if (out_data != NULL) { + *out_data = slot_data; + } else { + avb_slot_verify_data_free(slot_data); } avb_free_cmdline_subst_list(additional_cmdline_subst); diff --git a/lib/libavb/avb_slot_verify.h b/lib/libavb/avb_slot_verify.h index 73fd70d4ce..8d0fa53693 100644 --- a/lib/libavb/avb_slot_verify.h +++ b/lib/libavb/avb_slot_verify.h @@ -51,12 +51,25 @@ typedef enum { * be used ONLY for diagnostics and debugging. It cannot be used * unless AVB_SLOT_VERIFY_FLAGS_ALLOW_VERIFICATION_ERROR is also * used. + * + * AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO means that either + * AVB_HASHTREE_ERROR_MODE_RESTART or AVB_HASHTREE_ERROR_MODE_EIO is used + * depending on state. This mode implements a state machine whereby + * AVB_HASHTREE_ERROR_MODE_RESTART is used by default and when + * AVB_SLOT_VERIFY_FLAGS_RESTART_CAUSED_BY_HASHTREE_CORRUPTION is passed the + * mode transitions to AVB_HASHTREE_ERROR_MODE_EIO. When a new OS has been + * detected the device transitions back to the AVB_HASHTREE_ERROR_MODE_RESTART + * mode. To do this persistent storage is needed - specifically this means that + * the passed in AvbOps will need to have the read_persistent_value() and + * write_persistent_value() operations implemented. The name of the persistent + * value used is "avb.managed_verity_mode" and 32 bytes of storage is needed. */ typedef enum { AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE, AVB_HASHTREE_ERROR_MODE_RESTART, AVB_HASHTREE_ERROR_MODE_EIO, - AVB_HASHTREE_ERROR_MODE_LOGGING + AVB_HASHTREE_ERROR_MODE_LOGGING, + AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO } AvbHashtreeErrorMode; /* Flags that influence how avb_slot_verify() works. @@ -80,10 +93,26 @@ typedef enum { * contents loaded from |requested_partition| will be the contents of * the entire partition instead of just the size specified in the hash * descriptor. + * + * The AVB_SLOT_VERIFY_FLAGS_RESTART_CAUSED_BY_HASHTREE_CORRUPTION flag + * should be set if using AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO + * and the reason the boot loader is running is because the device + * was restarted by the dm-verity driver. + * + * If the AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION flag is set then + * data won't be loaded from the "vbmeta" partition and the + * |validate_vbmeta_public_key| operation is never called. Instead, the + * vbmeta structs in |requested_partitions| are loaded and processed and the + * |validate_public_key_for_partition| operation is called for each of these + * vbmeta structs. This flag is useful when booting into recovery on a device + * not using A/B - see section "Booting into recovery" in README.md for + * more information. */ typedef enum { AVB_SLOT_VERIFY_FLAGS_NONE = 0, - AVB_SLOT_VERIFY_FLAGS_ALLOW_VERIFICATION_ERROR = (1 << 0) + AVB_SLOT_VERIFY_FLAGS_ALLOW_VERIFICATION_ERROR = (1 << 0), + AVB_SLOT_VERIFY_FLAGS_RESTART_CAUSED_BY_HASHTREE_CORRUPTION = (1 << 1), + AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION = (1 << 2), } AvbSlotVerifyFlags; /* Get a textual representation of |result|. */ @@ -188,6 +217,10 @@ typedef struct { * set to AVB_HASHTREE_ERROR_MODE_EIO, and 'logging' if it's set to * AVB_HASHTREE_ERROR_MODE_LOGGING. * + * androidboot.veritymode.managed: This is set to 'yes' only + * if hashtree validation isn't disabled and the passed-in hashtree + * error mode is AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO. + * * androidboot.vbmeta.invalidate_on_error: This is set to 'yes' only * if hashtree validation isn't disabled and the passed-in hashtree * error mode is AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE. @@ -203,7 +236,9 @@ typedef struct { * PARTUUID=$(ANDROID_VBMETA_PARTUUID) before substitution so it * will end up pointing to the vbmeta partition for the verified * slot. If there is no vbmeta partition it will point to the boot - * partition of the verified slot. + * partition of the verified slot. If the flag + * AVB_SLOT_VERIFY_FLAGS_NO_VBMETA_PARTITION is used, this is not + * set. * * androidboot.vbmeta.avb_version: This is set to the decimal value * of AVB_VERSION_MAJOR followed by a dot followed by the decimal @@ -228,6 +263,15 @@ typedef struct { * appropriate system partition is substituted in. Note that none of * the androidboot.* options mentioned above will be set. * + * The |resolved_hashtree_error_mode| is the the value of the passed + * avb_slot_verify()'s |hashtree_error_mode| parameter except that it never has + * the value AVB_HASHTREE_ERROR_MODE_MANAGED_RESTART_AND_EIO. If this value was + * passed in, then the restart/eio state machine is used resulting in + * |resolved_hashtree_error_mode| being set to either + * AVB_HASHTREE_ERROR_MODE_RESTART or AVB_HASHTREE_ERROR_MODE_EIO. If set to + * AVB_HASHTREE_ERROR_MODE_EIO the boot loader should present a RED warning + * screen for the user to click through before continuing to boot. + * * This struct may grow in the future without it being considered an * ABI break. */ @@ -239,6 +283,7 @@ typedef struct { size_t num_loaded_partitions; char* cmdline; uint64_t rollback_indexes[AVB_MAX_NUMBER_OF_ROLLBACK_INDEX_LOCATIONS]; + AvbHashtreeErrorMode resolved_hashtree_error_mode; } AvbSlotVerifyData; /* Calculates a digest of all vbmeta images in |data| using @@ -282,12 +327,8 @@ void avb_slot_verify_data_free(AvbSlotVerifyData* data); * ignore verification errors which is something needed in the * UNLOCKED state. See the AvbSlotVerifyFlags enumeration for details. * - * The |hashtree_error_mode| parameter should be set to the desired - * error handling mode when hashtree validation fails inside the - * HLOS. This value isn't used by libavb per se - it is forwarded to - * the HLOS through the androidboot.veritymode and - * androidboot.vbmeta.invalidate_on_error cmdline parameters. See the - * AvbHashtreeErrorMode enumeration for details. + * The |hashtree_error_mode| parameter should be set to the desired error + * handling mode. See the AvbHashtreeErrorMode enumeration for details. * * Also note that |out_data| is never set if * AVB_SLOT_VERIFY_RESULT_ERROR_OOM, AVB_SLOT_VERIFY_RESULT_ERROR_IO, diff --git a/lib/libavb/avb_sysdeps.h b/lib/libavb/avb_sysdeps.h index f032de4a2e..f52428cc62 100644 --- a/lib/libavb/avb_sysdeps.h +++ b/lib/libavb/avb_sysdeps.h @@ -53,6 +53,14 @@ int avb_memcmp(const void* src1, */ int avb_strcmp(const char* s1, const char* s2); +/* Compare |n| bytes in two strings. + * + * Return an integer less than, equal to, or greater than zero if the + * first |n| bytes of |s1| is found, respectively, to be less than, + * to match, or be greater than the first |n| bytes of |s2|. + */ +int avb_strncmp(const char* s1, const char* s2, size_t n); + /* Copy |n| bytes from |src| to |dest|. */ void* avb_memcpy(void* dest, const void* src, size_t n); diff --git a/lib/libavb/avb_sysdeps_posix.c b/lib/libavb/avb_sysdeps_posix.c index e9addc1c87..4ccf41e428 100644 --- a/lib/libavb/avb_sysdeps_posix.c +++ b/lib/libavb/avb_sysdeps_posix.c @@ -24,14 +24,12 @@ int avb_strcmp(const char* s1, const char* s2) { return strcmp(s1, s2); } -size_t avb_strlen(const char* str) { - return strlen(str); +int avb_strncmp(const char* s1, const char* s2, size_t n) { + return strncmp(s1, s2, n); } -uint32_t avb_div_by_10(uint64_t* dividend) { - uint32_t rem = (uint32_t)(*dividend % 10); - *dividend /= 10; - return rem; +size_t avb_strlen(const char* str) { + return strlen(str); } void avb_abort(void) { @@ -60,3 +58,9 @@ void* avb_malloc_(size_t size) { void avb_free(void* ptr) { free(ptr); } + +uint32_t avb_div_by_10(uint64_t* dividend) { + uint32_t rem = (uint32_t)(*dividend % 10); + *dividend /= 10; + return rem; +} diff --git a/lib/libavb/avb_vbmeta_image.c b/lib/libavb/avb_vbmeta_image.c index a7e2322b9e..384f5ac19e 100644 --- a/lib/libavb/avb_vbmeta_image.c +++ b/lib/libavb/avb_vbmeta_image.c @@ -35,17 +35,18 @@ AvbVBMetaVerifyResult avb_vbmeta_image_verify( *out_public_key_length = 0; } + /* Before we byteswap or compare Magic, ensure length is long enough. */ + if (length < sizeof(AvbVBMetaImageHeader)) { + avb_error("Length is smaller than header.\n"); + goto out; + } + /* Ensure magic is correct. */ if (avb_safe_memcmp(data, AVB_MAGIC, AVB_MAGIC_LEN) != 0) { avb_error("Magic is incorrect.\n"); goto out; } - /* Before we byteswap, ensure length is long enough. */ - if (length < sizeof(AvbVBMetaImageHeader)) { - avb_error("Length is smaller than header.\n"); - goto out; - } avb_vbmeta_image_header_to_host_byte_order((const AvbVBMetaImageHeader*)data, &h); From 8f684b5a8b8351bf2d464609071ccd2de35c8773 Mon Sep 17 00:00:00 2001 From: Sam Protsenko Date: Thu, 15 Aug 2019 23:04:03 +0300 Subject: [PATCH 36/39] libavb: Fix build warnings after updating the lib After updating libavb to most recent version from AOSP/master, two new warnings appear: Warning #1: lib/libavb/avb_cmdline.c: In function 'avb_append_options': lib/libavb/avb_cmdline.c:365:15: warning: 'dm_verity_mode' may be used uninitialized in this function [-Wmaybe-uninitialized] new_ret = avb_replace( ^~~~~~~~~~~~ slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/libavb/avb_cmdline.c:374:8: warning: 'verity_mode' may be used uninitialized in this function [-Wmaybe-uninitialized] if (!cmdline_append_option( ^~~~~~~~~~~~~~~~~~~~~~ slot_data, "androidboot.veritymode", verity_mode)) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Warning #2: lib/libavb/avb_slot_verify.c: In function 'avb_slot_verify': lib/libavb/avb_slot_verify.c:1349:23: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] AvbSlotVerifyResult ret; ^~~ Fix those by providing default return values to affected functions. Signed-off-by: Sam Protsenko --- lib/libavb/avb_cmdline.c | 3 ++- lib/libavb/avb_slot_verify.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c index cb5b98e423..684c512bb9 100644 --- a/lib/libavb/avb_cmdline.c +++ b/lib/libavb/avb_cmdline.c @@ -357,7 +357,8 @@ AvbSlotVerifyResult avb_append_options( // Should never get here because MANAGED_RESTART_AND_EIO is // remapped by avb_manage_hashtree_error_mode(). avb_assert_not_reached(); - break; + ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; + goto out; default: ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; goto out; diff --git a/lib/libavb/avb_slot_verify.c b/lib/libavb/avb_slot_verify.c index 5d400b38aa..c0defdf9c9 100644 --- a/lib/libavb/avb_slot_verify.c +++ b/lib/libavb/avb_slot_verify.c @@ -1346,7 +1346,7 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, AvbSlotVerifyFlags flags, AvbHashtreeErrorMode hashtree_error_mode, AvbSlotVerifyData** out_data) { - AvbSlotVerifyResult ret; + AvbSlotVerifyResult ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; AvbSlotVerifyData* slot_data = NULL; AvbAlgorithmType algorithm_type = AVB_ALGORITHM_TYPE_NONE; bool using_boot_for_vbmeta = false; From bb43c2784704b79eaa383fe38255b2ecf73b46ff Mon Sep 17 00:00:00 2001 From: Sam Protsenko Date: Thu, 15 Aug 2019 20:49:47 +0300 Subject: [PATCH 37/39] cmd: avb: Fix requested partitions list The requested_partitions[] array should contain only boot partitions. Usually it's only 'boot' partition, as can be seen in [1]. Also, seems like the requested_partitions[] are only used when there is no 'vbmeta' partition [2], which is not a regular use-case. Make requested_partitions[] contain only 'boot' partition as it was supposed to be, and also make that array to be a local in do_avb_verify_part() function, as nobody else needs that. [1] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/test/avb_slot_verify_unittest.cc#108 [2] https://android.googlesource.com/platform/external/avb/+/5fbb42a189aa/libavb/avb_slot_verify.c#1461 Signed-off-by: Sam Protsenko Reviewed-by: Igor Opaniuk --- cmd/avb.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/cmd/avb.c b/cmd/avb.c index 5bc158252b..d4e8884328 100644 --- a/cmd/avb.c +++ b/cmd/avb.c @@ -15,11 +15,6 @@ #define AVB_BOOTARGS "avb_bootargs" static struct AvbOps *avb_ops; -static const char * const requested_partitions[] = {"boot", - "system", - "vendor", - NULL}; - int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { unsigned long mmc_dev; @@ -232,6 +227,7 @@ int do_avb_get_uuid(cmd_tbl_t *cmdtp, int flag, int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) { + const char * const requested_partitions[] = {"boot", NULL}; AvbSlotVerifyResult slot_result; AvbSlotVerifyData *out_data; char *cmdline; From 965ec3caa8fcaafe3d18ad7c3810a13b09ee8abe Mon Sep 17 00:00:00 2001 From: Sam Protsenko Date: Mon, 21 Oct 2019 13:55:16 +0300 Subject: [PATCH 38/39] cmd: avb: Support A/B slots Add optional parameter to 'avb verify' sub-command, so that user is able to specify which slot to use, in case when user's partitions are slotted. If that parameter is omitted, the behavior of 'avb verify' will be the same as before, so user API is content. Signed-off-by: Sam Protsenko Reviewed-by: Igor Opaniuk Acked-by: Igor Opaniuk --- cmd/avb.c | 13 +++++++++---- doc/android/avb2.txt | 4 ++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/cmd/avb.c b/cmd/avb.c index d4e8884328..a4de5c40a2 100644 --- a/cmd/avb.c +++ b/cmd/avb.c @@ -232,6 +232,7 @@ int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag, AvbSlotVerifyData *out_data; char *cmdline; char *extra_args; + char *slot_suffix = ""; bool unlocked = false; int res = CMD_RET_FAILURE; @@ -241,9 +242,12 @@ int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag, return CMD_RET_FAILURE; } - if (argc != 1) + if (argc < 1 || argc > 2) return CMD_RET_USAGE; + if (argc == 2) + slot_suffix = argv[1]; + printf("## Android Verified Boot 2.0 version %s\n", avb_version_string()); @@ -256,7 +260,7 @@ int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag, slot_result = avb_slot_verify(avb_ops, requested_partitions, - "", + slot_suffix, unlocked, AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE, &out_data); @@ -416,7 +420,7 @@ static cmd_tbl_t cmd_avb[] = { U_BOOT_CMD_MKENT(read_part, 5, 0, do_avb_read_part, "", ""), U_BOOT_CMD_MKENT(read_part_hex, 4, 0, do_avb_read_part_hex, "", ""), U_BOOT_CMD_MKENT(write_part, 5, 0, do_avb_write_part, "", ""), - U_BOOT_CMD_MKENT(verify, 1, 0, do_avb_verify_part, "", ""), + U_BOOT_CMD_MKENT(verify, 2, 0, do_avb_verify_part, "", ""), #ifdef CONFIG_OPTEE_TA_AVB U_BOOT_CMD_MKENT(read_pvalue, 3, 0, do_avb_read_pvalue, "", ""), U_BOOT_CMD_MKENT(write_pvalue, 3, 0, do_avb_write_pvalue, "", ""), @@ -459,6 +463,7 @@ U_BOOT_CMD( "avb read_pvalue - read a persistent value \n" "avb write_pvalue - write a persistent value \n" #endif - "avb verify - run verification process using hash data\n" + "avb verify [slot_suffix] - run verification process using hash data\n" " from vbmeta structure\n" + " [slot_suffix] - _a, _b, etc (if vbmeta partition is slotted)\n" ); diff --git a/doc/android/avb2.txt b/doc/android/avb2.txt index a29cee1b6f..48e9297c75 100644 --- a/doc/android/avb2.txt +++ b/doc/android/avb2.txt @@ -95,6 +95,10 @@ e.g.: mmc read ${loadaddr} ${boot_start} ${boot_size}; \ bootm $loadaddr $loadaddr $fdtaddr; \ +If partitions you want to verify are slotted (have A/B suffixes), then current +slot suffix should be passed to 'avb verify' sub-command, e.g.: + +=> avb verify _a To switch on automatic generation of vbmeta partition in AOSP build, add these lines to device configuration mk file: From 5d80a1a93d42c8325d65516cc654ff6a9ceec58a Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Thu, 31 Oct 2019 10:45:03 -0400 Subject: [PATCH 39/39] azure: Update for python3 and current pytest Similar to the rework for GitLab-CI and Travis-CI, rework the Azure Pipeline to use python3 and requirements.txt to install the necessary modules. Signed-off-by: Tom Rini --- .azure-pipelines.yml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index d476d8d0e9..11d5a6175b 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -1,7 +1,7 @@ variables: windows_vm: vs2015-win2012r2 ubuntu_vm: ubuntu-18.04 - ci_runner_image: trini/u-boot-gitlab-ci-runner:bionic-20190912.1-03Oct2019 + ci_runner_image: trini/u-boot-gitlab-ci-runner:bionic-20191010-20Oct2019 # Add '-u 0' options for Azure pipelines, otherwise we get "permission # denied" error when it tries to "useradd -m -u 1001 vsts_azpcontainer", # since our $(ci_runner_image) user is not root. @@ -245,11 +245,6 @@ jobs: git clone --depth=1 git://github.com/swarren/uboot-test-hooks.git /tmp/uboot-test-hooks ln -s travis-ci /tmp/uboot-test-hooks/bin/`hostname` ln -s travis-ci /tmp/uboot-test-hooks/py/`hostname` - virtualenv /tmp/venv - . /tmp/venv/bin/activate - pip install pytest==2.8.7 - pip install python-subunit - pip install coverage grub-mkimage --prefix=\"\" -o ~/grub_x86.efi -O i386-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd grub-mkimage --prefix=\"\" -o ~/grub_x64.efi -O x86_64-efi normal echo lsefimmap lsefi lsefisystab efinet tftp minicmd mkdir ~/grub2-arm @@ -266,6 +261,9 @@ jobs: exit $ret; fi; fi + virtualenv -p /usr/bin/python3 /tmp/venv + . /tmp/venv/bin/activate + pip install -r test/py/requirements.txt export UBOOT_TRAVIS_BUILD_DIR=/tmp/.bm-work/${TEST_PY_BD}; export PATH=/opt/qemu/bin:/tmp/uboot-test-hooks/bin:/usr/bin:/bin; export PYTHONPATH=/tmp/uboot-test-hooks/py/travis-ci;