Re: [kvm-unit-tests PATCH v2 4/6] x86: efi_main: Self-relocate ELF .dynamic addresses

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

On 8/19/21 4:33 AM, Varad Gautam wrote:
> EFI expects a relocatable PE, and the loader will patch in the
> relocations from the COFF.
>
> Since we are wrapping an ELF into a PE here, the EFI loader will
> not handle ELF relocations, and we need to patch the ELF .dynamic
> section manually on early boot.
>
> Signed-off-by: Varad Gautam<varad.gautam@suse.com>
> ---
>   x86/efi_main.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
>
> diff --git a/x86/efi_main.c b/x86/efi_main.c
> index 237d4e7..be3f9ab 100644
> --- a/x86/efi_main.c
> +++ b/x86/efi_main.c
> @@ -1,9 +1,13 @@
>   #include <alloc_phys.h>
>   #include <linux/uefi.h>
> +#include <elf.h>
>   
>   unsigned long __efiapi efi_main(efi_handle_t handle, efi_system_table_t *sys_tab);
>   efi_system_table_t *efi_system_table = NULL;
>   
> +extern char ImageBase;
> +extern char _DYNAMIC;
> +
>   static void efi_free_pool(void *ptr)
>   {
>   	efi_bs_call(free_pool, ptr);
> @@ -93,11 +97,70 @@ static efi_status_t exit_efi(void *handle)
>   	return EFI_SUCCESS;
>   }
>   
> +static efi_status_t elf_reloc(unsigned long image_base, unsigned long dynamic)


Since this function is only relocating the dynamic section, we should 
probably name it something like elf_reloc_dyn().

> +{
> +	long relsz = 0, relent = 0;
> +	Elf64_Rel *rel = 0;
> +	Elf64_Dyn *dyn = (Elf64_Dyn *) dynamic;
> +	unsigned long *addr;
> +	int i;
> +
> +	for (i = 0; dyn[i].d_tag != DT_NULL; i++) {
> +		switch (dyn[i].d_tag) {
> +		case DT_RELA:
> +			rel = (Elf64_Rel *)
> +				((unsigned long) dyn[i].d_un.d_ptr + image_base);
> +			break;
> +		case DT_RELASZ:
> +			relsz = dyn[i].d_un.d_val;
> +			break;
> +		case DT_RELAENT:
> +			relent = dyn[i].d_un.d_val;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (!rel && relent == 0)
> +		return EFI_SUCCESS;
> +
> +	if (!rel || relent == 0)
> +		return EFI_LOAD_ERROR;
> +
> +	while (relsz > 0) {
> +		/* apply the relocs */
> +		switch (ELF64_R_TYPE (rel->r_info)) {
> +		case R_X86_64_NONE:
> +			break;
> +		case R_X86_64_RELATIVE:
> +			addr = (unsigned long *) (image_base + rel->r_offset);
> +			*addr += image_base;
> +			break;
> +		default:
> +			break;
> +		}
> +		rel = (Elf64_Rel *) ((char *) rel + relent);
> +		relsz -= relent;
> +	}
> +	return EFI_SUCCESS;
> +}
> +
>   unsigned long __efiapi efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
>   {
> +	unsigned long image_base, dyn;
>   	efi_system_table = sys_tab;
>   
>   	exit_efi(handle);
>   
> +	image_base = (unsigned long) &ImageBase;
> +	dyn = image_base + (unsigned long) &_DYNAMIC;
> +
> +	/* The EFI loader does not handle ELF relocations, so fixup
> +	 * .dynamic addresses before proceeding any further. */
> +	elf_reloc(image_base, dyn);
> +
> +	start64();


Should this call to start64() be moved to your next patch because the 
function needs to be fixed and you are fixing it in there ?

> +
>   	return 0;
>   }