Hi Simon, Thanks for the review. I have posted v2 as a two-patch series addressing your comments:
https://lore.kernel.org/u-boot/[email protected]/ 1. image: Bound legacy script parsing by the image size 2. bootstd: Add a bootmeth for scripts in raw partitions > Rejecting external data is unexpected behaviour, but I suppose it is > fair enough, since scripts are small and we don't expect any other > images in the FIT. I have retained this restriction and documented that the script FIT must be self-contained, so its metadata and embedded data can be validated within the loaded partition image. > Philosophically speaking, it would be nice to extern the existing > bootmeth_script - did you consider that. Yes, I considered extending the existing script bootmeth. For now, I have kept a separate method so raw partition-label lookup and filesystem script lookup can be selected independently in the bootmeth order. Both methods share script verification and execution. Would this approach be acceptable to you? > Please can you add the bounds checks to image_locate_script() instead, > in a separate patch? Done in patch 1. The shared legacy parser now checks known buffer sizes before CRC verification, bounds the length-table walk by the declared payload size, and checks the script length against the remaining data. The existing script bootmeth and source command benefit from the same checks, and the raw bootmeth no longer duplicates the parsing. I retained genimg_get_format() and added a known-buffer header-size check before format detection. The regression test is named image_legacy_script_bounds to make its coverage explicit. > Please drop these, so it is clearer which checks matter. Done. I removed the redundant legacy data-size and header-CRC checks, and the FIT totalsize equality check from the raw image validator. > This only applies to FIT but also runs for legacy images. Please move > it into the FIT branch above. Done. The raw FIT minimum-size check is now in the FIT branch. > Please use log_msg_ret() for error returns here and on the 'goto out' > paths, as other bootmeths do. Done. The rejection paths now have log_msg_ret() diagnostics identifying the failed operation or check. > The underline is one character short, so Sphinx warns and 'make > htmldocs' fails in CI. Fixed. The strict HTML documentation build now passes without warnings. > No defconfig enables CONFIG_BOOTMETH_SCRIPT_RAW, so CI always skips > this test. Please can you enable it for sandbox? Check that the > existing bootflow/bootmeth tests still pass, since they count the > auto-bound bootmeths. The test also needs > @pytest.mark.requiredtool('sgdisk'), as in test_ab.py. Done. CONFIG_BOOTMETH_SCRIPT_RAW is enabled in sandbox_defconfig, and the image setup test requires sgdisk. The existing bootflow and bootmeth tests have been updated for the additional method. The iterator test retains BOOTFLOWIF_SKIP_GLOBAL coverage. > bootmeths should be tested in C, in test/boot/bootflow.c, with a disk > image created by test/py/img/ Done. The execution tests are now in test/boot/bootflow.c, using a GPT image generated by test/py/img/script_raw.py. The C test covers legacy and FIT execution and the invalid-image rejection cases. The sandbox build and 40 selected tests passed. After renaming the legacy parser test, I rebuilt sandbox and reran that test successfully. Both patches pass checkpatch, and the strict documentation build passes. v2 was also successfully tested on LCKFB Taishan Pi RK3566 using lckfb-tspi-rk3566_defconfig with CONFIG_BOOTMETH_SCRIPT_RAW enabled. Regards, He Yong
