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 <[email protected]>
---
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
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/[email protected]/ 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
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/[email protected]/ 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
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/[email protected]/ 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
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/[email protected]/ 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