Hi Simon, On Mon, Oct 12, 2026 at 10:11 PM Simon Glass <[email protected]> wrote: > > Hi Aristo, > > On 2026-10-11T15:21:13, Aristo Chen <[email protected]> wrote: > > test: py: cover invalid external data properties in fit_import_data() > > > > 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, now that the import reads the properties through the FIT > > accessors, check that an empty data-offset or data-size is skipped like > > a missing property, so that the hashing stage reports the image instead > > of the import reading it from the bytes that follow it. > > I don't think this is quite right. Before patch 4 the import used > fdtdec_get_int(), which already returns the default for a property > shorter than an int (see the 'len >= sizeof(int)' check in > lib/fdtdec_common.c). So the import never read an empty property, and > patch 4's commit message says as much ('as it already did for a > shorter one'). What this test actually guards is patch 2. Once the > import skips the image, the hashing stage in tools/image-host.c calls > fit_image_get_data(), which without patch 2 would read the following > FDT tag as the offset or size and carry on. Please can you reword this > paragraph to say so?
Agreed that the import never read an empty property before patch 4. The empty cases only test patch 2 together with patch 4, though: they also pass before the series and with patch 2 alone, and fail only with patch 4 but not patch 2, where the import takes the next FDT tag as the value (an empty data-size imports 3 bytes) and mkimage succeeds. I checked all three with mkimage built at each point. The skipped image is not reported by the hashing stage either, and my commit message was wrong there too. fit_cipher_data() runs before the hashing stage and calls fit_image_cipher_data() for every image, which fetches the embedded data with fit_image_get_emb_data() before it looks for a cipher node. That is where mkimage stops, with "Can't get image data/size". Neither stage calls fit_image_get_data() or reads data-offset or data-size. I will reword the paragraph for v2. > > > > > Signed-off-by: Aristo Chen <[email protected]> > > > > test/py/tests/test_fit_import_data.py | 91 > > ++++++++++++++++++++++++++++++++++- > > 1 file changed, 90 insertions(+), 1 deletion(-) > > > diff --git a/test/py/tests/test_fit_import_data.py > > b/test/py/tests/test_fit_import_data.py > > @@ -113,3 +118,87 @@ def test_fit_import_data_missing_offset(ubman): > > + # An empty property must be skipped like a missing one, so that the > > + # hashing stage reports the image, rather than read from the bytes > > + # that follow it, which would import a few bytes or the wrong data > > + for prop in ('data-offset', 'data-size'): > > Same here: 'import a few bytes' was never possible for an empty property. Right, only patch 4 without patch 2 does that, and no commit in the series has that combination. I will reword the comment as well. > > The case that patch 4 does change in fit_import_data() is a property > longer than one cell. fdtdec_get_int() accepts it and uses the first > cell, but the accessors now reject it. Please can you add a two-cell > data-offset and data-size (e.g. fdtput -t i ... 0 0) so that the > import change itself is covered? The test name and docstring could > then be widened from 'shorter than one cell' to 'not exactly one > cell'. Good idea. Before this series, data-offset = <0 0> imports the first kernel's data into the second one and data-size = <0 0> leaves the second one empty, both without an error. I will add both cases for v2 > > Regards, > Simon Thanks for the review. Best regards, Aristo
