Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.pau@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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.