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.

Fixes: f7f42accbbbb ('x86/efi: Use generic PE/COFF structures')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/efi/mkreloc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
index 375cb79d6959..1a6cfc845cba 100644
--- 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.
@@ -73,11 +71,12 @@ static unsigned int load(const char *name, int *handle,
     {
     case PE_OPT_MAGIC_PE32:
         *width = 32;
-        *image_base = base;
+        *image_base = pe32_opt_hdr.image_base;
         break;
     case PE_OPT_MAGIC_PE32PLUS:
         *width = 64;
-        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
+        *image_base = ((uint64_t)pe32_opt_hdr.image_base << 32) |
+                      pe32_opt_hdr.data_base;
         break;
     default:
         fprintf(stderr, "%s: Wrong PE file format\n", name);
-- 
2.48.1

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

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@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@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.

--- 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;
+        }
+        /* 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 )
+    {
+        fprintf(stderr,
+                "%s: section %.8s @ %08"PRIx32" beyond image size %08"PRIxFAST32"\n",
+                name, sec->name, sec->rva, image_size);
+        exit(6);
+    }
+
+    if ( (uint_fast64_t)sec->rva + sec->virtual_size > image_size )
+    {
+        fprintf(stderr,
+                "%s: section %.8s @ [%09"PRIx32",%09"PRIxFAST64") extends beyond image size %09"PRIxFAST32"\n",
+                name, sec->name, sec->rva,
+                (uint_fast64_t)sec->rva + sec->virtual_size, image_size);
+        exit(6);
+    }
+
     if ( !page_size )
         page_size = sysconf(_SC_PAGESIZE);
     offs = sec->data_addr & (page_size - 1);
@@ -233,7 +261,7 @@ int main(int argc, char *argv[])
     int in1, in2;
     unsigned int i, nsec, width1, width2;
     uint_fast64_t base1, base2;
-    uint32_t size1, size2;
+    uint_fast32_t size1, size2;
     struct section_header *sec1, *sec2;
 
     if ( argc == 1 ||
@@ -308,8 +336,8 @@ int main(int argc, char *argv[])
             sec1[i].raw_data_size = sec1[i].virtual_size;
             sec2[i].raw_data_size = sec2[i].virtual_size;
         }
-        ptr1 = map_section(sec1 + i, in1, argv[1]);
-        ptr2 = map_section(sec2 + i, in2, argv[2]);
+        ptr1 = map_section(sec1 + i, in1, argv[1], size1);
+        ptr2 = map_section(sec2 + i, in2, argv[2], size1);
 
         diff_sections(ptr1, ptr2, sec1 + i, base2 - base1, width1,
                       base1, base1 + size1);

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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@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.

On 08.04.2025 13:21, Roger Pau Monné wrote:
> On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
>> 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@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@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.

This is precisely why I said "more cumbersome".

>> @@ -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?

Except that the switch() itself uses not only pe_hdr, but also
pe32_opt_hdr. That could be re-arranged, but I'm a little reluctant to
do so.

>> @@ -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?

Yes and no. No in so far as this would be wrong for zero-size sections.
Yet see also the first of the two post-commit-message remarks.

Jan

On Tue, Apr 08, 2025 at 02:34:48PM +0200, Jan Beulich wrote:
> On 08.04.2025 13:21, Roger Pau Monné wrote:
> > On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
> >> 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@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@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.
> 
> This is precisely why I said "more cumbersome".
> 
> >> @@ -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?
> 
> Except that the switch() itself uses not only pe_hdr, but also
> pe32_opt_hdr. That could be re-arranged, but I'm a little reluctant to
> do so.

Hm, I see, the magic field checked here is in the extended header, so
we would need to fetch it ahead of the switch in any case.  How
unhelpful.

One thing that I find weird about this code is the obfuscation of the
switch condition, won't it be easier to read as:

if ( pe_hdr.magic != PE_MAGIC ||
     pe_hdr.opt_hdr_size < sizeof(pe32_opt_hdr) )
    fprintf(stderr,
            "%s: Wrong PE magic or missing optional header\n", name);
    exit(3);
}

switch ( pe32_opt_hdr.magic )
{
...

I would assume the current arrangement is done as to reuse the
`default` error label, but IMO that switch condition is too hard to
parse.

> >> @@ -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?
> 
> Yes and no. No in so far as this would be wrong for zero-size sections.
> Yet see also the first of the two post-commit-message remarks.

Hm, yes, don't have a strong opinion really, just leave it like that
I guess.

Thanks, Roger.

On 10.04.2025 09:20, Roger Pau Monné wrote:
> On Tue, Apr 08, 2025 at 02:34:48PM +0200, Jan Beulich wrote:
>> On 08.04.2025 13:21, Roger Pau Monné wrote:
>>> On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
>>>> @@ -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?
>>
>> Except that the switch() itself uses not only pe_hdr, but also
>> pe32_opt_hdr. That could be re-arranged, but I'm a little reluctant to
>> do so.
> 
> Hm, I see, the magic field checked here is in the extended header, so
> we would need to fetch it ahead of the switch in any case.  How
> unhelpful.
> 
> One thing that I find weird about this code is the obfuscation of the
> switch condition, won't it be easier to read as:
> 
> if ( pe_hdr.magic != PE_MAGIC ||
>      pe_hdr.opt_hdr_size < sizeof(pe32_opt_hdr) )
>     fprintf(stderr,
>             "%s: Wrong PE magic or missing optional header\n", name);
>     exit(3);
> }
> 
> switch ( pe32_opt_hdr.magic )
> {
> ...
> 
> I would assume the current arrangement is done as to reuse the
> `default` error label, but IMO that switch condition is too hard to
> parse.

Well, yes, I have a tendency to code things like this to re-use code
where possible, but I (meanwhile) understand many people don't like
the result. Doing this differently would be a separate patch though, I
think. Anyway - to catch the maintainers' attention I guess I'll (re-)
submit the patch outside of this thread.

Jan

On 01/04/2025 2:08 pm, 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

This is slightly awkward grammar. "Prior to commit" is the more normal
phrasing.

> 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.
>
> Fixes: f7f42accbbbb ('x86/efi: Use generic PE/COFF structures')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>