Hi Heinrich, On 10/30/26 13:14, Heinrich Schuchardt wrote: > The PECOFF specification does not require any table. Six are needed if > you have a .reloc section which some non-compliant software assumes. > Aren't you describing a bug in barebox?
No, that sentence is wrong, sorry. barebox's optional header struct does not include the data directories, so its size check accepts fewer than 16 as well. I'll drop the sentence in v2. > read_header() in > https://github.com/rhboot/shim/blob/e794a714.../pe-relocate.c#L376 > has rather strict tests for PE files. It might be worthwhile to compare > their checks with ours. I went through it. shim has one check the series lacks, and EDK2's PeCoffLoaderGetPeHeader() has it too: NumberOfRvaAndSizes must be at most 16, and SizeOfOptionalHeader must hold exactly that many entries. v2 checks that instead of only checking that the certificate table entry fits. The rest is covered by the series, is loader policy (stripped relocations, NX), concerns SizeOfImage, which U-Boot doesn't use, or, like requiring e_lfanew to point past the DOS header, rejects odd images that do no harm here. > FileAlignment and SectionAlignment should be checked once before the > loop calling this function instead of checking the same value multiple > times. OK. efi_image_parse() only uses FileAlignment. SectionAlignment is only used in efi_load_pe(), where efi_alloc_aligned_pages() already refuses one that is not a power of two. > We are already checking for overflows in pe_range_ok(). Why use > __builtin_add_overflow()? pe_range_ok() checks SizeOfRawData before it is rounded up, and the rounding could still wrap where size_t is 32 bits. > This whole function can be replaced by using the ALIGN() macro. Yes. v2 drops pe_align_size() and rounds up with ALIGN() on a u64, which can't wrap. > Please, avoid this increment by starting at 4. Will do. > The PECOFF specification defines a default of 512 for the FileAlignment. > That is the value you should use if FileAlignment == 0. Will do, with a test. > Here you are closing a memory leak. But this issue is not described in > the commit message. There was no leak before: the old code had no error exit while sorted was allocated. The free is needed for the error exits this patch adds to the section loop. I'll say so in the commit message. Thanks, Ana
