Re: [Qemu-devel] [PATCH v7 1/3] loader: Allow ELF loader to auto-detect the ELF arch

[Peter Maydell]

On 25 May 2016 at 19:49, Alistair Francis <address@hidden> wrote:
> If the caller didn't specify an architecture for the ELF machine
> the load_elf() function will auto detect it based on the ELF file.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V7:
>  - Fix typo
>
>  hw/core/loader.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 53e0e41..a8a372d 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -419,6 +419,7 @@ int load_elf(const char *filename, uint64_t 
> (*translate_fn)(void *, uint64_t),
>  {
>      int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>      uint8_t e_ident[EI_NIDENT];
> +    uint16_t e_machine;
>
>      fd = open(filename, O_RDONLY | O_BINARY);
>      if (fd < 0) {
> @@ -451,6 +452,15 @@ int load_elf(const char *filename, uint64_t 
> (*translate_fn)(void *, uint64_t),
>          goto fail;
>      }
>
> +    if (elf_machine < 1) {
> +        /* The caller didn't specify an ARCH, we can figure it out */
> +        lseek(fd, 0x12, SEEK_SET);
> +        if (read(fd, &e_machine, sizeof(e_machine)) != sizeof(e_machine)) {
> +            goto fail;
> +        }
> +        elf_machine = e_machine;
> +    }

Isn't there an endianness problem here, given that e_machine is a 16
bit field? In load_elf32()/load_elf64() we will byteswap the e_machine
field we read off the disk if must_swab is true, which will mean it won't
match the value we've read here and not byteswapped.

I think you're better off pushing the "allow architecture to be
unspecified" support down into load_elf32()/load_elf64(), where
it can just become

    if (elf_machine < 1) {
        elf_machine = ehdr.e_machine;
    }

once the load_elf code has read and byteswapped the header for you.

thanks
-- PMM