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

[Alistair Francis]

On Thu, Jun 9, 2016 at 10:38 AM, Peter Maydell  wrote:
> On 25 May 2016 at 19:49, Alistair Francis  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 
>> ---
>> 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, _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.

Good point, I didn't realise it was the same code for both. I have
moved it into load_elf64/load_elf32.

Thanks,

Alistair

>
> thanks
> -- PMM
>

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  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 
> ---
> 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, _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

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

[Alistair Francis]

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 
---
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, _machine, sizeof(e_machine)) != sizeof(e_machine)) {
+goto fail;
+}
+elf_machine = e_machine;
+}
+
 lseek(fd, 0, SEEK_SET);
 if (e_ident[EI_CLASS] == ELFCLASS64) {
 ret = load_elf64(filename, fd, translate_fn, translate_opaque, 
must_swab,
-- 
2.7.4