Build an external-data FIT, then point one image's data-offset far past the end of the file and, separately, make its data-size run a few bytes past the end, re-processing the result with mkimage -F each time. Both must be rejected with a diagnostic naming that image. Previously the first crashed mkimage and the second silently imported the bytes beyond the end of the file as image data.
Also check that a FIT whose images are all empty, which mkimage -E -p writes with data-position past the end of the file, still re-processes, since an empty region has nothing to read. Finally, check that a data-offset or data-size that is not exactly one cell is skipped like a missing property, so that mkimage finds no data for the image and fails. fdtdec_get_int() already ignored an empty property, so that case covers the length check the import now relies on in the accessors. A property of two cells used to be imported from its first cell, so that <0 0> made the image use another image's data or left it empty. While at it, correct the comment in test_fit_import_data_missing_offset(), which said that the hashing stage reports the skipped image. mkimage stops before that stage, in the cipher pass, which runs first and fetches the data of every image. Signed-off-by: Aristo Chen <[email protected]> --- v2: also cover a data-offset and data-size of two cells, rename the test to test_fit_import_data_prop_len and reword its comment and the last paragraph of the commit message, as suggested by Simon. Neither names the hashing stage any more, since mkimage stops in the cipher pass before reaching it, and the comment of the existing test_fit_import_data_missing_offset() is corrected too. test/py/tests/test_fit_import_data.py | 96 ++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/test/py/tests/test_fit_import_data.py b/test/py/tests/test_fit_import_data.py index 95f7114243e..7ce59291953 100644 --- a/test/py/tests/test_fit_import_data.py +++ b/test/py/tests/test_fit_import_data.py @@ -3,7 +3,7 @@ # # Test mkimage import of external data in fit_import_data() -"""Regression test for stale per-image state in fit_import_data(). +"""Regression tests for fit_import_data(). The import loop used to keep the data pointer and external property name of the previous image, so an image node carrying data-size but neither @@ -11,6 +11,11 @@ data-offset nor data-position imported the previous image's data and then made mkimage abort without printing any diagnostic. Such a node must be skipped by the import and reported by the later processing stages instead. + +The import also read each image's external data at the offset and size +the FIT declares without checking them against the file, so a region +reaching past the end of the file crashed mkimage or imported unrelated +memory as image data. Such a region must be rejected before it is read. """ import os @@ -107,9 +112,96 @@ def test_fit_import_data_missing_offset(ubman): fdtput_keep_data(itb, '/images/kernel-2', 'data-offset', opts=['-d']) # Re-processing must skip the malformed image in the import, so that - # the hashing stage reports it; previously the stale pointer made the + # a later stage reports it; previously the stale pointer made the # import write kernel-1's data into kernel-2 and abort silently result = subprocess.run([mkimage, '-F', itb], capture_output=True, text=True) assert result.returncode != 0 assert "Can't get image data/size" in result.stderr + + [email protected]('sandbox') [email protected]('dtc') [email protected]('fdtput') +def test_fit_import_data_out_of_range(ubman): + """External data outside the file must be rejected, not read""" + mkimage = os.path.join(ubman.config.build_dir, 'tools/mkimage') + params = { + 'kernel1': fit_util.make_kernel(ubman, 'oob-kernel1.bin', 'first'), + 'kernel2': fit_util.make_kernel(ubman, 'oob-kernel2.bin', 'second'), + } + its = fit_util.make_its(ubman, BASE_ITS, params, 'oob.its') + itb = fit_util.make_fname(ubman, 'oob.itb') + + result = subprocess.run([mkimage, '-E', '-f', its, itb], + capture_output=True, text=True) + assert result.returncode == 0, result.stderr + + # Point kernel-2's data far past the end of the file + fdtput_keep_data(itb, '/images/kernel-2', 'data-offset', '0x40000000', + opts=['-t', 'i']) + result = subprocess.run([mkimage, '-F', itb], + capture_output=True, text=True) + assert result.returncode != 0 + assert "image 'kernel-2': external data" in result.stderr + assert 'outside the FIT file' in result.stderr + + # A size running just past the end of the file must be rejected too. + # kernel-2's data comes last, padded to 4 bytes, so 4 bytes more than + # its real size always reach past the end. + result = subprocess.run([mkimage, '-E', '-f', its, itb], + capture_output=True, text=True) + assert result.returncode == 0, result.stderr + size = os.path.getsize(params['kernel2']) + 4 + fdtput_keep_data(itb, '/images/kernel-2', 'data-size', f'{size:#x}', + opts=['-t', 'i']) + result = subprocess.run([mkimage, '-F', itb], + capture_output=True, text=True) + assert result.returncode != 0 + assert "image 'kernel-2': external data" in result.stderr + + # An empty region has nothing to read, so it is accepted even past the + # end of the file, which is where mkimage -E -p places it when every + # image is empty + for name in ('kernel1', 'kernel2'): + params[name] = fit_util.make_fname(ubman, f'oob-empty-{name}.bin') + with open(params[name], 'wb'): + pass + its = fit_util.make_its(ubman, BASE_ITS, params, 'oob-empty.its') + result = subprocess.run([mkimage, '-E', '-p', '0x1000', '-f', its, itb], + capture_output=True, text=True) + assert result.returncode == 0, result.stderr + assert os.path.getsize(itb) < 0x1000 + result = subprocess.run([mkimage, '-F', itb], + capture_output=True, text=True) + assert result.returncode == 0, result.stderr + + [email protected]('sandbox') [email protected]('dtc') [email protected]('fdtput') +def test_fit_import_data_prop_len(ubman): + """An external data property that is not exactly one cell is not read""" + mkimage = os.path.join(ubman.config.build_dir, 'tools/mkimage') + params = { + 'kernel1': fit_util.make_kernel(ubman, 'len-kernel1.bin', 'first'), + 'kernel2': fit_util.make_kernel(ubman, 'len-kernel2.bin', 'second'), + } + its = fit_util.make_its(ubman, BASE_ITS, params, 'len.its') + itb = fit_util.make_fname(ubman, 'len.itb') + + # A property that is not exactly one cell must be skipped like a + # missing one, so that mkimage finds no data for the image and fails. + # An empty one must not be read from the bytes that follow it, and one + # of two cells must not be taken from its first cell. + for prop in ('data-offset', 'data-size'): + for cells in ([], ['0', '0']): + result = subprocess.run([mkimage, '-E', '-f', its, itb], + capture_output=True, text=True) + assert result.returncode == 0, result.stderr + fdtput_keep_data(itb, '/images/kernel-2', prop, *cells, + opts=['-t', 'i']) + result = subprocess.run([mkimage, '-F', itb], + capture_output=True, text=True) + assert result.returncode != 0 + assert "Can't get image data/size" in result.stderr -- 2.43.0
