[PATCH] Revert "x86/module: Detect and skip invalid relocations"

[PATCH] Revert "x86/module: Detect and skip invalid relocations"

[Cheng Jian]

This reverts commit eda9cec4c9a12208a6f69fbe68f72a6311d50032.

Since commit (eda9cec4c9a1 'x86/module: Detect and skip invalid
relocations') add some sanity check in apply_relocate_add, borke
re-insmod a kernel module which has been patched before,

The relocation informations of the livepatch module have been
overwritten since first patched, so if we rmmod and insmod the
kernel module, these values are not zero anymore, when
klp_module_coming doing, and that commit marks them as invalid
invalid_relocation.

Then the following error occurs:

	module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc (____ptrval____), val ffffffffc000236c
	livepatch: failed to initialize patch 'livepatch_0001_test' for module 'test' (-8)
	livepatch: patch 'livepatch_0001_test' failed for module 'test', refusing to load module 'test'

To fix this, just revert that commit.

Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
---
 arch/x86/kernel/module.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index d5c72cb..3eb23a8 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -160,28 +160,20 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 		case R_X86_64_NONE:
 			break;
 		case R_X86_64_64:
-			if (*(u64 *)loc != 0)
-				goto invalid_relocation;
 			*(u64 *)loc = val;
 			break;
 		case R_X86_64_32:
-			if (*(u32 *)loc != 0)
-				goto invalid_relocation;
 			*(u32 *)loc = val;
 			if (val != *(u32 *)loc)
 				goto overflow;
 			break;
 		case R_X86_64_32S:
-			if (*(s32 *)loc != 0)
-				goto invalid_relocation;
 			*(s32 *)loc = val;
 			if ((s64)val != *(s32 *)loc)
 				goto overflow;
 			break;
 		case R_X86_64_PC32:
 		case R_X86_64_PLT32:
-			if (*(u32 *)loc != 0)
-				goto invalid_relocation;
 			val -= (u64)loc;
 			*(u32 *)loc = val;
 #if 0
@@ -190,8 +182,6 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 #endif
 			break;
 		case R_X86_64_PC64:
-			if (*(u64 *)loc != 0)
-				goto invalid_relocation;
 			val -= (u64)loc;
 			*(u64 *)loc = val;
 			break;
@@ -203,11 +193,6 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	}
 	return 0;
 
-invalid_relocation:
-	pr_err("x86/modules: Skipping invalid relocation target, existing value is nonzero for type %d, loc %p, val %Lx\n",
-	       (int)ELF64_R_TYPE(rel[i].r_info), loc, val);
-	return -ENOEXEC;
-
 overflow:
 	pr_err("overflow in relocation type %d val %Lx\n",
 	       (int)ELF64_R_TYPE(rel[i].r_info), val);
-- 
2.7.4

Re: [PATCH] Revert "x86/module: Detect and skip invalid relocations"

[Miroslav Benes]

On Thu, 20 Jun 2019, Cheng Jian wrote:

> This reverts commit eda9cec4c9a12208a6f69fbe68f72a6311d50032.
> 
> Since commit (eda9cec4c9a1 'x86/module: Detect and skip invalid
> relocations') add some sanity check in apply_relocate_add, borke
> re-insmod a kernel module which has been patched before,
> 
> The relocation informations of the livepatch module have been
> overwritten since first patched, so if we rmmod and insmod the
> kernel module, these values are not zero anymore, when
> klp_module_coming doing, and that commit marks them as invalid
> invalid_relocation.
> 
> Then the following error occurs:
> 
> 	module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc (____ptrval____), val ffffffffc000236c
> 	livepatch: failed to initialize patch 'livepatch_0001_test' for module 'test' (-8)
> 	livepatch: patch 'livepatch_0001_test' failed for module 'test', refusing to load module 'test'

Oh yeah. First reported here 20180602161151.apuhs2dygsexmcg2@treble (LP ML 
only and there is no archive on lore.kernel.org yet. Sorry about that.). I 
posted v1 here 
https://lore.kernel.org/lkml/20180607092949.1706-1-mbenes@suse.cz/ and 
even started to work on v2 in March with arch-specific nullifying, but 
then I got sidetracked again. I'll move it up my todo list a bit.

Miroslav

Re: [PATCH] Revert "x86/module: Detect and skip invalid relocations"

[Thomas Gleixner]

Miroslav,

On Thu, 20 Jun 2019, Miroslav Benes wrote:
> On Thu, 20 Jun 2019, Cheng Jian wrote:
> 
> > This reverts commit eda9cec4c9a12208a6f69fbe68f72a6311d50032.
> > 
> > Since commit (eda9cec4c9a1 'x86/module: Detect and skip invalid
> > relocations') add some sanity check in apply_relocate_add, borke
> > re-insmod a kernel module which has been patched before,
> > 
> > The relocation informations of the livepatch module have been
> > overwritten since first patched, so if we rmmod and insmod the
> > kernel module, these values are not zero anymore, when
> > klp_module_coming doing, and that commit marks them as invalid
> > invalid_relocation.
> > 
> > Then the following error occurs:
> > 
> > 	module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc (____ptrval____), val ffffffffc000236c
> > 	livepatch: failed to initialize patch 'livepatch_0001_test' for module 'test' (-8)
> > 	livepatch: patch 'livepatch_0001_test' failed for module 'test', refusing to load module 'test'
> 
> Oh yeah. First reported here 20180602161151.apuhs2dygsexmcg2@treble (LP ML 
> only and there is no archive on lore.kernel.org yet. Sorry about that.). I 
> posted v1 here 
> https://lore.kernel.org/lkml/20180607092949.1706-1-mbenes@suse.cz/ and 
> even started to work on v2 in March with arch-specific nullifying, but 
> then I got sidetracked again. I'll move it up my todo list a bit.

so we need to revert it for now, right?

Thanks,

	tglx

Re: [PATCH] Revert "x86/module: Detect and skip invalid relocations"

[Miroslav Benes]

On Sat, 22 Jun 2019, Thomas Gleixner wrote:

> Miroslav,
> 
> On Thu, 20 Jun 2019, Miroslav Benes wrote:
> > On Thu, 20 Jun 2019, Cheng Jian wrote:
> > 
> > > This reverts commit eda9cec4c9a12208a6f69fbe68f72a6311d50032.
> > > 
> > > Since commit (eda9cec4c9a1 'x86/module: Detect and skip invalid
> > > relocations') add some sanity check in apply_relocate_add, borke
> > > re-insmod a kernel module which has been patched before,
> > > 
> > > The relocation informations of the livepatch module have been
> > > overwritten since first patched, so if we rmmod and insmod the
> > > kernel module, these values are not zero anymore, when
> > > klp_module_coming doing, and that commit marks them as invalid
> > > invalid_relocation.
> > > 
> > > Then the following error occurs:
> > > 
> > > 	module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc (____ptrval____), val ffffffffc000236c
> > > 	livepatch: failed to initialize patch 'livepatch_0001_test' for module 'test' (-8)
> > > 	livepatch: patch 'livepatch_0001_test' failed for module 'test', refusing to load module 'test'
> > 
> > Oh yeah. First reported here 20180602161151.apuhs2dygsexmcg2@treble (LP ML 
> > only and there is no archive on lore.kernel.org yet. Sorry about that.). I 
> > posted v1 here 
> > https://lore.kernel.org/lkml/20180607092949.1706-1-mbenes@suse.cz/ and 
> > even started to work on v2 in March with arch-specific nullifying, but 
> > then I got sidetracked again. I'll move it up my todo list a bit.
> 
> so we need to revert it for now, right?

Not necessarily.

Quoting Josh from the original bug report:
"Possible ways to fix it:

1) Remove the error check in apply_relocate_add().  I don't think we
   should do this, because the error is actually useful for detecting
   corrupt modules.  And also, powerpc has the similar error so this
   wouldn't be a universal solution.

2) In klp_unpatch_object(), call an arch-specific arch_unpatch_object()
   which reverses any arch-specific patching: on x86, clearing all
   relocation targets to zero; on powerpc, converting the instructions
   after relative link branches to nops.  I don't think we should do
   this because it's not a global solution and requires fidgety
   arch-specific patching code.

3) Don't allow patched modules to be removed.  I think this makes the
   most sense.  Nobody needs this functionality anyway (right?).
"

1 would be the revert. We decided against it. The scenario (rmmod a 
module) is (supposedly) not that common in practice. Even the current bug 
report was triggered just in testing if I am not mistaken. Moreover, you 
need kpatch-build to properly set up relocation records. Upstream 
livepatch does not offer it as of now. That's why (I think) Josh thought 
the benefits of the check outweighed the disadvantage.

Then I tried to implement 3, but there were problems with it too. 2 
remains to be finished and then we can decide what the best approach is.

That being said... I am not against the reverting the commit per se, but 
we lived with it or quite a long time and no one has met it so far in 
"real life". I don't think it is the classic "we broke something, we have 
to revert" scenario.

Josh, any comment? I think your opinion matters here much more than mine.

Miroslav

Re: [PATCH] Revert "x86/module: Detect and skip invalid relocations"

[Josh Poimboeuf]

On Mon, Jun 24, 2019 at 12:00:33PM +0200, Miroslav Benes wrote:
> On Sat, 22 Jun 2019, Thomas Gleixner wrote:
> 
> > Miroslav,
> > 
> > On Thu, 20 Jun 2019, Miroslav Benes wrote:
> > > On Thu, 20 Jun 2019, Cheng Jian wrote:
> > > 
> > > > This reverts commit eda9cec4c9a12208a6f69fbe68f72a6311d50032.
> > > > 
> > > > Since commit (eda9cec4c9a1 'x86/module: Detect and skip invalid
> > > > relocations') add some sanity check in apply_relocate_add, borke
> > > > re-insmod a kernel module which has been patched before,
> > > > 
> > > > The relocation informations of the livepatch module have been
> > > > overwritten since first patched, so if we rmmod and insmod the
> > > > kernel module, these values are not zero anymore, when
> > > > klp_module_coming doing, and that commit marks them as invalid
> > > > invalid_relocation.
> > > > 
> > > > Then the following error occurs:
> > > > 
> > > > 	module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc (____ptrval____), val ffffffffc000236c
> > > > 	livepatch: failed to initialize patch 'livepatch_0001_test' for module 'test' (-8)
> > > > 	livepatch: patch 'livepatch_0001_test' failed for module 'test', refusing to load module 'test'
> > > 
> > > Oh yeah. First reported here 20180602161151.apuhs2dygsexmcg2@treble (LP ML 
> > > only and there is no archive on lore.kernel.org yet. Sorry about that.). I 
> > > posted v1 here 
> > > https://lore.kernel.org/lkml/20180607092949.1706-1-mbenes@suse.cz/ and 
> > > even started to work on v2 in March with arch-specific nullifying, but 
> > > then I got sidetracked again. I'll move it up my todo list a bit.
> > 
> > so we need to revert it for now, right?
> 
> Not necessarily.
> 
> Quoting Josh from the original bug report:
> "Possible ways to fix it:
> 
> 1) Remove the error check in apply_relocate_add().  I don't think we
>    should do this, because the error is actually useful for detecting
>    corrupt modules.  And also, powerpc has the similar error so this
>    wouldn't be a universal solution.
> 
> 2) In klp_unpatch_object(), call an arch-specific arch_unpatch_object()
>    which reverses any arch-specific patching: on x86, clearing all
>    relocation targets to zero; on powerpc, converting the instructions
>    after relative link branches to nops.  I don't think we should do
>    this because it's not a global solution and requires fidgety
>    arch-specific patching code.
> 
> 3) Don't allow patched modules to be removed.  I think this makes the
>    most sense.  Nobody needs this functionality anyway (right?).
> "
> 
> 1 would be the revert. We decided against it. The scenario (rmmod a 
> module) is (supposedly) not that common in practice. Even the current bug 
> report was triggered just in testing if I am not mistaken. Moreover, you 
> need kpatch-build to properly set up relocation records. Upstream 
> livepatch does not offer it as of now. That's why (I think) Josh thought 
> the benefits of the check outweighed the disadvantage.
> 
> Then I tried to implement 3, but there were problems with it too. 2 
> remains to be finished and then we can decide what the best approach is.
> 
> That being said... I am not against the reverting the commit per se, but 
> we lived with it or quite a long time and no one has met it so far in 
> "real life". I don't think it is the classic "we broke something, we have 
> to revert" scenario.
> 
> Josh, any comment? I think your opinion matters here much more than mine.

Agreed, as far as I know the problem is purely theoretical and we
haven't seen any real-world bug reports, because people aren't reloading
patched modules in the real world.

If we were to revert the error checks in apply_relocate_add() then it
could expose us to real-world regressions (which we have actually seen
in the past).

So I would vote to leave the error checks in place, at least until it
becomes a real-world issue.  And in the meantime hopefully you can
finish implementing #2 or #3 soon :-)

-- 
Josh