Thank you for the feedback – all the comments have now been addressed. > Since this is a security fix, please mention in the commit message > that the OOB read is reachable during signature verification of an > untrusted FIT
We did it for all five patches, since they are all security fixes. > and add a test in test/boot/ (or extend an existing > one) that feeds a FIT with an oversized hashed-strings size and checks > that verification fails cleanly We added the test for this, as well as for the patch with the subject "image-fit: Validate external data offset and size". The other three patches have no additional tests as they are basic sanity checks with no logical validation to exercise. Thank you, Anton On Fri, May 29, 2026 at 9:45 AM Simon Glass <[email protected]> wrote: > Hi Anton, > > On 2026-05-27T11:23:38, Anton Ivanov <[email protected]> wrote: > > image-fit-sig: Validate hashed-strings region size > > > > fit_config_check_sig() reads the hashed-strings property and uses > > its size value without validation when building the region list for > > signature verification. A crafted FIT image can specify an arbitrary > > size, causing the hash calculation to read beyond the end of the FIT > > image. > > > > Validate that the declared strings region fits within the FIT > > before adding it to the region list. > > > > Signed-off-by: Anton Ivanov <[email protected]> > > > > boot/image-fit-sig.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c > > @@ -512,8 +512,18 @@ static int fit_config_check_sig(const void *fit, > int noffset, int conf_noffset, > > * The strings region offset must be a static 0x0. > > * This is set in tool/image-host.c > > */ > > - fdt_regions[count].offset = fdt_off_dt_strings(fit); > > - fdt_regions[count].size = fdt32_to_cpu(strings[1]); > > + int offset = fdt_off_dt_strings(fit); > > + int size = fdt32_to_cpu(strings[1]); > > + /* > > + * The offset should be already validated by > fdt_check_header(); > > + * validate the size here. > > + */ > > + if (size < 0 || size > fdt_totalsize(fit) - offset) { > > + *err_msgp = "Strings region is out of bounds"; > > + return -1; > > + } > > Thanks for fixing this. > > tools/image-host.c writes string_size = fdt_size_dt_strings(fit), so > the tighter bound here is fdt_size_dt_strings(fit) rather than > fdt_totalsize(fit) - offset. The latter still permits a crafted size > that overlaps the structs block while staying within the FIT. > > Also, fdt_getprop() is called with a NULL lenp on line 509, so > strings[1] is read without first checking that the property is at > least 2 * sizeof(fdt32_t) bytes long. A crafted FIT could shorten > hashed-strings to make that read go past the property - please can you > validate the property length too while you are here. > > fdt_off_dt_strings() and fdt32_to_cpu() both return uint32_t, so the > locals would be better as uint32_t (and the size < 0 check then goes > away). Please move them up with the other locals rather than declaring > them mid-block. > > Since this is a security fix, please mention in the commit message > that the OOB read is reachable during signature verification of an > untrusted FIT, and add a test in test/boot/ (or extend an existing > one) that feeds a FIT with an oversized hashed-strings size and checks > that verification fails cleanly. > > Regards, > Simon >
