Re: [PATCH] s390/kexec_file: fix error handling when applying relocations

[Heiko Carstens <hca@linux.ibm.com>]

On Mon, Dec 06, 2021 at 12:20:47PM +0100, Philipp Rudo wrote:
> arch_kexec_apply_relocations_add currently ignores all errors returned
> by arch_kexec_do_relocs. This means that every unknown relocation is
> silently skipped causing unpredictable behavior while the relocated code
> runs. Fix this by checking for errors and fail kexec_file_load if an
> unknown relocation type is encountered.
> 
> The problem was found after gcc changed its behavior and used
> R_390_PLT32DBL relocations for brasl instruction and relied on ld to
> resolve the relocations in the final link in case direct calls are
> possible. As the purgatory code is only linked partially (option -r)
> ld didn't resolve the relocations leaving them for arch_kexec_do_relocs.
> But arch_kexec_do_relocs doesn't know how to handle R_390_PLT32DBL
> relocations so they were silently skipped. This ultimately caused an
> endless loop in the purgatory as the brasl instructions kept branching
> to itself.
> 
> Fixes: 71406883fd35 ("s390/kexec_file: Add kexec_file_load system call")
> Reported-by: Tao Liu <ltao@redhat.com>
> Signed-off-by: Philipp Rudo <prudo@redhat.com>
> ---
>  arch/s390/kernel/machine_kexec_file.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
> index 9975ad200d74..0e1d646207dc 100644
> --- a/arch/s390/kernel/machine_kexec_file.c
> +++ b/arch/s390/kernel/machine_kexec_file.c
> @@ -292,6 +292,7 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
>  {
>  	Elf_Rela *relas;
>  	int i, r_type;
> +	int ret;
>  
>  	relas = (void *)pi->ehdr + relsec->sh_offset;
>  
> @@ -326,7 +327,9 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
>  		addr = section->sh_addr + relas[i].r_offset;
>  
>  		r_type = ELF64_R_TYPE(relas[i].r_info);
> -		arch_kexec_do_relocs(r_type, loc, val, addr);
> +		ret = arch_kexec_do_relocs(r_type, loc, val, addr);
> +		if (ret)
> +			return -EINVAL;

I'd prefer if this would return -ENOEXEC, just to be consistent with
x86. And _maybe_ it would also make sense to print an error message,
including the failing relocation type?

Thanks,
Heiko