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

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

[Philipp Rudo]

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;
 	}
 	return 0;
 }
-- 
2.31.1

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

[Heiko Carstens]

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

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

[Philipp Rudo]

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

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

[Heiko Carstens]

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

No objections at all. This sounds good!

Thanks,
Heiko