Hi Heiko,
On Mon, 6 Dec 2021 18:13:43 +0100
Heiko Carstens <hca@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Philipp Rudo <prudo@xxxxxxxxxx>
> > ---
> > 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
