On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
> On 01.04.2025 16:17, Jan Beulich wrote:
> > On 01.04.2025 15:08, Roger Pau Monne wrote:
> >> The base address is in the pe32_opt_hdr, not after it.
>
> Which is a result of pe.h munging both the optional and the NT header into
> a single structure.
>
> >> 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?
>
> It simply didn't. We got away only because apparently no-one tried a build
> with a linker old enough for this tool to come into play.
>
> I'd like to suggest the replacement patch below, though.
>
> Jan
>
> x86/EFI: correct mkreloc header (field) reading
>
> With us now reading the full combined optional and NT headers, the
> subsequent reading of (and seeking to) NT header fields is wrong. Since
> PE32 and PE32+ NT headers are different anyway (beyond the image base
> oddity extending across both headers), switch to using a union. This
> allows to fetch the image base more directly then.
>
> Additionally add checking to map_section(), which would have caught at
> least the wrong (zero) image size that we previously used.
>
> Fixes: f7f42accbbbb ("x86/efi: Use generic PE/COFF structures")
> Reported-by: Roger Pau Monné <roger....@citrix.com>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> ---
> Of the two checks added to map_section(), the 1st ends up being largely
> redundant with the 2nd one. Should we use just the latter?
>
> Also sanity checking the image base would be possible, but more
> cumbersome if we wanted to check moret than just "is in high half of
> address space). Therefore I've left out doing so.
We could likely check that image_base >= XEN_VIRT_START? However I'm
not sure how easy it is to make XEN_VIRT_START available to mkreloc.
> --- a/xen/arch/x86/efi/mkreloc.c
> +++ b/xen/arch/x86/efi/mkreloc.c
> @@ -28,14 +28,16 @@ static void usage(const char *cmd, int r
> static unsigned int load(const char *name, int *handle,
> struct section_header **sections,
> uint_fast64_t *image_base,
> - uint32_t *image_size,
> + uint_fast32_t *image_size,
> unsigned int *width)
> {
> int in = open(name, O_RDONLY);
> struct mz_hdr mz_hdr;
> struct pe_hdr pe_hdr;
> - struct pe32_opt_hdr pe32_opt_hdr;
> - uint32_t base;
> + union {
> + struct pe32_opt_hdr pe;
> + struct pe32plus_opt_hdr pep;
> + } pe32_opt_hdr;
>
> if ( in < 0 ||
> read(in, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) )
> @@ -54,31 +56,40 @@ static unsigned int load(const char *nam
>
> 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.
> - */
> - lseek(in, 24, SEEK_CUR) < 0 ||
> - read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) )
> + (read(in, &pe32_opt_hdr.pe, sizeof(pe32_opt_hdr.pe)) !=
> + sizeof(pe32_opt_hdr.pe)) )
> {
> perror(name);
> exit(3);
> }
>
> switch ( (pe_hdr.magic == PE_MAGIC &&
> - pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr)) *
> - pe32_opt_hdr.magic )
> + pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pe)) *
> + pe32_opt_hdr.pe.magic )
> {
> case PE_OPT_MAGIC_PE32:
> *width = 32;
> - *image_base = base;
> + *image_base = pe32_opt_hdr.pe.image_base;
> + *image_size = pe32_opt_hdr.pe.image_size;
> break;
> case PE_OPT_MAGIC_PE32PLUS:
> - *width = 64;
> - *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
> - break;
> + if ( pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pep) )
> + {
> + if ( read(in,
> + &pe32_opt_hdr.pe + 1,
> + sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe)) !=
> + sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe) )
> + {
> + perror(name);
> + exit(3);
> + }
> +
> + *width = 64;
> + *image_base = pe32_opt_hdr.pep.image_base;
> + *image_size = pe32_opt_hdr.pep.image_size;
> + break;
> + }
Since you are already refactoring much of this code, won't it be
clearer to fetch the header inside of the switch cases. So that
there's a single read call for each header type?
> + /* Fall through. */
> default:
> fprintf(stderr, "%s: Wrong PE file format\n", name);
> exit(3);
> @@ -108,11 +119,28 @@ static unsigned int load(const char *nam
> static long page_size;
>
> static const void *map_section(const struct section_header *sec, int in,
> - const char *name)
> + const char *name, uint_fast32_t image_size)
> {
> const char *ptr;
> unsigned long offs;
>
> + if ( sec->rva > image_size )
Strictly, should this be >=, as rva is a position, and image_size is a
size, so the last allowed bit would be image_size - 1?
Thanks, Roger.
