On 01.04.2025 15:08, Roger Pau Monne wrote: > The base address is in the pe32_opt_hdr, not after it. > > Previous to commit f7f42accbbbb the base was read standalone (as the first > field of pe32_opt_hdr). However with the addition of reading the full > contents of pe32_opt_hdr, such read will also fetch the base. The current > attempt to read the base after pe32_opt_hdr is bogus, and could only work > if the file cursor is repositioned using lseek(), but there's no need for > that as the data is already fetched in pe32_opt_hdr.
Yes, but: How did things work at all then with this bug? Plus ... > --- a/xen/arch/x86/efi/mkreloc.c > +++ b/xen/arch/x86/efi/mkreloc.c > @@ -35,7 +35,6 @@ static unsigned int load(const char *name, int *handle, > struct mz_hdr mz_hdr; > struct pe_hdr pe_hdr; > struct pe32_opt_hdr pe32_opt_hdr; > - uint32_t base; > > if ( in < 0 || > read(in, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) ) > @@ -55,7 +54,6 @@ static unsigned int load(const char *name, int *handle, > if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 || > read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) || > read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != > sizeof(pe32_opt_hdr) || > - read(in, &base, sizeof(base)) != sizeof(base) || > /* > * Luckily the image size field lives at the > * same offset for both formats. ... the code right below here has the same issue then, hasn't it? It's a SEEK_CUR that's being done, which I'm sure isn't going to land us at the image size field (which again we did read already). Using the full structure also renders questionable why it's (only) pe32_opt_hdr that we use here, and not (also) pe32plus_opt_hdr. I think this is a pretty clear indication that said earlier change better wouldn't have gone in without a proper R-b. Jan
