From: Ard Biesheuvel <ardb@kernel.org>

The PVH entrypoint is entered in 32-bit mode, and is documented as being
a 32-bit field. Linux happens to widen the field in the ELF note to 64
bits so treating it as a 64-bit field works for booting the kernel.

However, Xen documents the ELF note with the following example

  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long, xen_start32)

and uses .long in the code as well, and so reading more than 32 bits
here is risky. And dereferencing a size_t* in portable code is just
bizarre, so let's use a uint32_t specifically in all cases here.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 hw/i386/x86-common.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index 992ea1f25e..5b1971c13b 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -539,7 +539,7 @@ DeviceState *ioapic_init_secondary(GSIState *gsi_state)
  */
 static uint64_t read_pvh_start_addr(void *arg1, void *arg2, bool is64)
 {
-    size_t *elf_note_data_addr;
+    void *elf_note_data_addr;
 
     /* Check if ELF Note header passed in is valid */
     if (arg1 == NULL) {
@@ -555,8 +555,6 @@ static uint64_t read_pvh_start_addr(void *arg1, void *arg2, bool is64)
         elf_note_data_addr =
             ((void *)nhdr64) + nhdr_size64 +
             QEMU_ALIGN_UP(nhdr_namesz, phdr_align);
-
-        pvh_start_addr = *elf_note_data_addr;
     } else {
         struct elf32_note *nhdr32 = (struct elf32_note *)arg1;
         uint32_t nhdr_size32 = sizeof(struct elf32_note);
@@ -566,10 +564,10 @@ static uint64_t read_pvh_start_addr(void *arg1, void *arg2, bool is64)
         elf_note_data_addr =
             ((void *)nhdr32) + nhdr_size32 +
             QEMU_ALIGN_UP(nhdr_namesz, phdr_align);
-
-        pvh_start_addr = *(uint32_t *)elf_note_data_addr;
     }
 
+    pvh_start_addr = *(uint32_t *)elf_note_data_addr;
+
     return pvh_start_addr;
 }
 
-- 
2.46.1.824.gd892dcdcdd-goog

On 9/27/24 09:19, Ard Biesheuvel wrote:
> -
> -        pvh_start_addr = *(uint32_t *)elf_note_data_addr;
>       }
>   
> +    pvh_start_addr = *(uint32_t *)elf_note_data_addr;

I think we even want ldl_le_p(elf_note_data_addr) here?  It makes no 
sense to read big-endian data.

Paolo

On Fri, 27 Sept 2024 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 9/27/24 09:19, Ard Biesheuvel wrote:
> > -
> > -        pvh_start_addr = *(uint32_t *)elf_note_data_addr;
> >       }
> >
> > +    pvh_start_addr = *(uint32_t *)elf_note_data_addr;
>
> I think we even want ldl_le_p(elf_note_data_addr) here?  It makes no
> sense to read big-endian data.
>

Yeah good point.