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

[Philipp Rudo <prudo@redhat.com>]

Hi Heiko,

On Mon, 6 Dec 2021 18:13:43 +0100
Heiko Carstens <hca@linux.ibm.com> wrote:

> 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?

sure, I'll update the return value to -ENOEXEC.

About the error message, I didn't add it on purpose as none of the
other error cases print one. For consistency I would add one for those
cases as well. Any objections?

Thanks
Philipp