Lemme send a proper patch now...
---
From: Borislav Petkov <[email protected]>
Date: Sun, 17 Jun 2018 13:57:42 +0200
Subject: [PATCH] x86/crypto: Add missing RETs
Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
otherwise they run into INT3 padding due to
51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
leading to spurious debug exceptions.
Mike Galbraith <[email protected]> took care of all the remaining callsites.
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/crypto/aegis128-aesni-asm.S | 1 +
arch/x86/crypto/aegis128l-aesni-asm.S | 1 +
arch/x86/crypto/aegis256-aesni-asm.S | 1 +
arch/x86/crypto/morus1280-avx2-asm.S | 1 +
arch/x86/crypto/morus1280-sse2-asm.S | 1 +
arch/x86/crypto/morus640-sse2-asm.S | 1 +
6 files changed, 6 insertions(+)
diff --git a/arch/x86/crypto/aegis128-aesni-asm.S b/arch/x86/crypto/aegis128-aesni-asm.S
index 9254e0b6cc06..717bf0776421 100644
--- a/arch/x86/crypto/aegis128-aesni-asm.S
+++ b/arch/x86/crypto/aegis128-aesni-asm.S
@@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail)
movdqu STATE3, 0x40(STATEP)
FRAME_END
+ ret
ENDPROC(crypto_aegis128_aesni_enc_tail)
.macro decrypt_block a s0 s1 s2 s3 s4 i
diff --git a/arch/x86/crypto/aegis128l-aesni-asm.S b/arch/x86/crypto/aegis128l-aesni-asm.S
index 9263c344f2c7..4eda2b8db9e1 100644
--- a/arch/x86/crypto/aegis128l-aesni-asm.S
+++ b/arch/x86/crypto/aegis128l-aesni-asm.S
@@ -645,6 +645,7 @@ ENTRY(crypto_aegis128l_aesni_enc_tail)
state_store0
FRAME_END
+ ret
ENDPROC(crypto_aegis128l_aesni_enc_tail)
/*
diff --git a/arch/x86/crypto/aegis256-aesni-asm.S b/arch/x86/crypto/aegis256-aesni-asm.S
index 1d977d515bf9..32aae8397268 100644
--- a/arch/x86/crypto/aegis256-aesni-asm.S
+++ b/arch/x86/crypto/aegis256-aesni-asm.S
@@ -543,6 +543,7 @@ ENTRY(crypto_aegis256_aesni_enc_tail)
state_store0
FRAME_END
+ ret
ENDPROC(crypto_aegis256_aesni_enc_tail)
/*
diff --git a/arch/x86/crypto/morus1280-avx2-asm.S b/arch/x86/crypto/morus1280-avx2-asm.S
index 37d422e77931..07653d4582a6 100644
--- a/arch/x86/crypto/morus1280-avx2-asm.S
+++ b/arch/x86/crypto/morus1280-avx2-asm.S
@@ -453,6 +453,7 @@ ENTRY(crypto_morus1280_avx2_enc_tail)
vmovdqu STATE4, (4 * 32)(%rdi)
FRAME_END
+ ret
ENDPROC(crypto_morus1280_avx2_enc_tail)
/*
diff --git a/arch/x86/crypto/morus1280-sse2-asm.S b/arch/x86/crypto/morus1280-sse2-asm.S
index 1fe637c7be9d..bd1aa1b60869 100644
--- a/arch/x86/crypto/morus1280-sse2-asm.S
+++ b/arch/x86/crypto/morus1280-sse2-asm.S
@@ -652,6 +652,7 @@ ENTRY(crypto_morus1280_sse2_enc_tail)
movdqu STATE4_HI, (9 * 16)(%rdi)
FRAME_END
+ ret
ENDPROC(crypto_morus1280_sse2_enc_tail)
/*
diff --git a/arch/x86/crypto/morus640-sse2-asm.S b/arch/x86/crypto/morus640-sse2-asm.S
index 71c72a0a0862..efa02816d921 100644
--- a/arch/x86/crypto/morus640-sse2-asm.S
+++ b/arch/x86/crypto/morus640-sse2-asm.S
@@ -437,6 +437,7 @@ ENTRY(crypto_morus640_sse2_enc_tail)
movdqu STATE4, (4 * 16)(%rdi)
FRAME_END
+ ret
ENDPROC(crypto_morus640_sse2_enc_tail)
/*
--
2.17.0.582.gccdcbd54c
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
so 23. 6. 2018 o 12:36 Borislav Petkov <[email protected]> napísal(a):
>
> Lemme send a proper patch now...
>
> ---
> From: Borislav Petkov <[email protected]>
> Date: Sun, 17 Jun 2018 13:57:42 +0200
> Subject: [PATCH] x86/crypto: Add missing RETs
>
> Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> otherwise they run into INT3 padding due to
>
> 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
>
> leading to spurious debug exceptions.
>
> Mike Galbraith <[email protected]> took care of all the remaining callsites.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Oh, thanks for fixing that!
Acked-by: Ondrej Mosnacek <[email protected]>
Cheers,
Ondrej
> ---
> arch/x86/crypto/aegis128-aesni-asm.S | 1 +
> arch/x86/crypto/aegis128l-aesni-asm.S | 1 +
> arch/x86/crypto/aegis256-aesni-asm.S | 1 +
> arch/x86/crypto/morus1280-avx2-asm.S | 1 +
> arch/x86/crypto/morus1280-sse2-asm.S | 1 +
> arch/x86/crypto/morus640-sse2-asm.S | 1 +
> 6 files changed, 6 insertions(+)
>
> diff --git a/arch/x86/crypto/aegis128-aesni-asm.S b/arch/x86/crypto/aegis128-aesni-asm.S
> index 9254e0b6cc06..717bf0776421 100644
> --- a/arch/x86/crypto/aegis128-aesni-asm.S
> +++ b/arch/x86/crypto/aegis128-aesni-asm.S
> @@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail)
> movdqu STATE3, 0x40(STATEP)
>
> FRAME_END
> + ret
> ENDPROC(crypto_aegis128_aesni_enc_tail)
>
> .macro decrypt_block a s0 s1 s2 s3 s4 i
> diff --git a/arch/x86/crypto/aegis128l-aesni-asm.S b/arch/x86/crypto/aegis128l-aesni-asm.S
> index 9263c344f2c7..4eda2b8db9e1 100644
> --- a/arch/x86/crypto/aegis128l-aesni-asm.S
> +++ b/arch/x86/crypto/aegis128l-aesni-asm.S
> @@ -645,6 +645,7 @@ ENTRY(crypto_aegis128l_aesni_enc_tail)
> state_store0
>
> FRAME_END
> + ret
> ENDPROC(crypto_aegis128l_aesni_enc_tail)
>
> /*
> diff --git a/arch/x86/crypto/aegis256-aesni-asm.S b/arch/x86/crypto/aegis256-aesni-asm.S
> index 1d977d515bf9..32aae8397268 100644
> --- a/arch/x86/crypto/aegis256-aesni-asm.S
> +++ b/arch/x86/crypto/aegis256-aesni-asm.S
> @@ -543,6 +543,7 @@ ENTRY(crypto_aegis256_aesni_enc_tail)
> state_store0
>
> FRAME_END
> + ret
> ENDPROC(crypto_aegis256_aesni_enc_tail)
>
> /*
> diff --git a/arch/x86/crypto/morus1280-avx2-asm.S b/arch/x86/crypto/morus1280-avx2-asm.S
> index 37d422e77931..07653d4582a6 100644
> --- a/arch/x86/crypto/morus1280-avx2-asm.S
> +++ b/arch/x86/crypto/morus1280-avx2-asm.S
> @@ -453,6 +453,7 @@ ENTRY(crypto_morus1280_avx2_enc_tail)
> vmovdqu STATE4, (4 * 32)(%rdi)
>
> FRAME_END
> + ret
> ENDPROC(crypto_morus1280_avx2_enc_tail)
>
> /*
> diff --git a/arch/x86/crypto/morus1280-sse2-asm.S b/arch/x86/crypto/morus1280-sse2-asm.S
> index 1fe637c7be9d..bd1aa1b60869 100644
> --- a/arch/x86/crypto/morus1280-sse2-asm.S
> +++ b/arch/x86/crypto/morus1280-sse2-asm.S
> @@ -652,6 +652,7 @@ ENTRY(crypto_morus1280_sse2_enc_tail)
> movdqu STATE4_HI, (9 * 16)(%rdi)
>
> FRAME_END
> + ret
> ENDPROC(crypto_morus1280_sse2_enc_tail)
>
> /*
> diff --git a/arch/x86/crypto/morus640-sse2-asm.S b/arch/x86/crypto/morus640-sse2-asm.S
> index 71c72a0a0862..efa02816d921 100644
> --- a/arch/x86/crypto/morus640-sse2-asm.S
> +++ b/arch/x86/crypto/morus640-sse2-asm.S
> @@ -437,6 +437,7 @@ ENTRY(crypto_morus640_sse2_enc_tail)
> movdqu STATE4, (4 * 16)(%rdi)
>
> FRAME_END
> + ret
> ENDPROC(crypto_morus640_sse2_enc_tail)
>
> /*
> --
> 2.17.0.582.gccdcbd54c
>
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
* Borislav Petkov <[email protected]> wrote:
> Lemme send a proper patch now...
>
> ---
> From: Borislav Petkov <[email protected]>
> Date: Sun, 17 Jun 2018 13:57:42 +0200
> Subject: [PATCH] x86/crypto: Add missing RETs
>
> Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> otherwise they run into INT3 padding due to
>
> 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
>
> leading to spurious debug exceptions.
>
> Mike Galbraith <[email protected]> took care of all the remaining callsites.
Note that 51bad67ffbce has been zapped because it caused too many problems like
this, but the explicit RETs make sense nevertheless. When applying the patch
please don't include the SHA1 of the non-upstream (and probably never-upstream)
commit.
Thanks,
Ingo
On Sun, 24 Jun 2018, Ingo Molnar wrote:
> * Borislav Petkov <[email protected]> wrote:
>
> > Lemme send a proper patch now...
> >
> > ---
> > From: Borislav Petkov <[email protected]>
> > Date: Sun, 17 Jun 2018 13:57:42 +0200
> > Subject: [PATCH] x86/crypto: Add missing RETs
> >
> > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> > otherwise they run into INT3 padding due to
> >
> > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> >
> > leading to spurious debug exceptions.
> >
> > Mike Galbraith <[email protected]> took care of all the remaining callsites.
>
> Note that 51bad67ffbce has been zapped because it caused too many problems like
> this, but the explicit RETs make sense nevertheless. When applying the patch
> please don't include the SHA1 of the non-upstream (and probably never-upstream)
> commit.
We should really have something like that exactly to catch cases like this.
Thanks,
tglx
On Sun, Jun 24, 2018 at 09:12:35AM +0200, Thomas Gleixner wrote:
> We should really have something like that exactly to catch cases like this.
Sounds like a good use case for the snake language.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote:
> > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> > otherwise they run into INT3 padding due to
> >
> > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> >
> > leading to spurious debug exceptions.
> >
> > Mike Galbraith <[email protected]> took care of all the remaining callsites.
>
> Note that 51bad67ffbce has been zapped because it caused too many problems like
> this, but the explicit RETs make sense nevertheless.
So commit which found real bug(s) was zapped.
OK
* Alexey Dobriyan <[email protected]> wrote:
> On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote:
> > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> > > otherwise they run into INT3 padding due to
> > >
> > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> > >
> > > leading to spurious debug exceptions.
> > >
> > > Mike Galbraith <[email protected]> took care of all the remaining callsites.
> >
> > Note that 51bad67ffbce has been zapped because it caused too many problems like
> > this, but the explicit RETs make sense nevertheless.
>
> So commit which found real bug(s) was zapped.
>
> OK
No, what happened is that the commit was first moved into WIP.x86/debug showing
its work-in-progress status, because it was incomplete and caused bugs:
https://lore.kernel.org/lkml/[email protected]/T/#u
... and finally, after weeks of inaction I zapped it because I didn't see progress
and you didn't answer my question.
If a fixed patch with updated tooling to detect these crashes before they occur on
live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just
incomplete in the current form.
Thanks,
Ingo
On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
>
> * Alexey Dobriyan <[email protected]> wrote:
>
> > On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote:
> > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> > > > otherwise they run into INT3 padding due to
> > > >
> > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> > > >
> > > > leading to spurious debug exceptions.
> > > >
> > > > Mike Galbraith <[email protected]> took care of all the remaining callsites.
> > >
> > > Note that 51bad67ffbce has been zapped because it caused too many problems like
> > > this, but the explicit RETs make sense nevertheless.
> >
> > So commit which found real bug(s) was zapped.
> >
> > OK
>
> No, what happened is that the commit was first moved into WIP.x86/debug showing
> its work-in-progress status, because it was incomplete and caused bugs:
>
> https://lore.kernel.org/lkml/[email protected]/T/#u
>
> ... and finally, after weeks of inaction I zapped it because I didn't see progress
> and you didn't answer my question.
>
> If a fixed patch with updated tooling to detect these crashes before they occur on
> live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just
> incomplete in the current form.
Hm, what happened to the objtool patch to detect these at build time?
Did it not work?
https://lore.kernel.org/lkml/20180517134934.eog2fgoby5azq5a7@treble
--
Josh
* Josh Poimboeuf <[email protected]> wrote:
> On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
> >
> > * Alexey Dobriyan <[email protected]> wrote:
> >
> > > On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote:
> > > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> > > > > otherwise they run into INT3 padding due to
> > > > >
> > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> > > > >
> > > > > leading to spurious debug exceptions.
> > > > >
> > > > > Mike Galbraith <[email protected]> took care of all the remaining callsites.
> > > >
> > > > Note that 51bad67ffbce has been zapped because it caused too many problems like
> > > > this, but the explicit RETs make sense nevertheless.
> > >
> > > So commit which found real bug(s) was zapped.
> > >
> > > OK
> >
> > No, what happened is that the commit was first moved into WIP.x86/debug showing
> > its work-in-progress status, because it was incomplete and caused bugs:
> >
> > https://lore.kernel.org/lkml/[email protected]/T/#u
> >
> > ... and finally, after weeks of inaction I zapped it because I didn't see progress
> > and you didn't answer my question.
> >
> > If a fixed patch with updated tooling to detect these crashes before they occur on
> > live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just
> > incomplete in the current form.
>
> Hm, what happened to the objtool patch to detect these at build time?
> Did it not work?
>
> https://lore.kernel.org/lkml/20180517134934.eog2fgoby5azq5a7@treble
So that's still incomplete in that doesn't analyze the 32-bit build yet, right?
Thanks,
Ingo
On Tue, Jun 26, 2018 at 08:49:30AM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> wrote:
>
> > On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
> > >
> > > * Alexey Dobriyan <[email protected]> wrote:
> > >
> > > > On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote:
> > > > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> > > > > > otherwise they run into INT3 padding due to
> > > > > >
> > > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> > > > > >
> > > > > > leading to spurious debug exceptions.
> > > > > >
> > > > > > Mike Galbraith <[email protected]> took care of all the remaining callsites.
> > > > >
> > > > > Note that 51bad67ffbce has been zapped because it caused too many problems like
> > > > > this, but the explicit RETs make sense nevertheless.
> > > >
> > > > So commit which found real bug(s) was zapped.
> > > >
> > > > OK
> > >
> > > No, what happened is that the commit was first moved into WIP.x86/debug showing
> > > its work-in-progress status, because it was incomplete and caused bugs:
> > >
> > > https://lore.kernel.org/lkml/[email protected]/T/#u
> > >
> > > ... and finally, after weeks of inaction I zapped it because I didn't see progress
> > > and you didn't answer my question.
> > >
> > > If a fixed patch with updated tooling to detect these crashes before they occur on
> > > live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just
> > > incomplete in the current form.
> >
> > Hm, what happened to the objtool patch to detect these at build time?
> > Did it not work?
> >
> > https://lore.kernel.org/lkml/20180517134934.eog2fgoby5azq5a7@treble
>
> So that's still incomplete in that doesn't analyze the 32-bit build yet, right?
We could do INT3s on 64-bit and NOPs on 32-bit.
Or, possibly even better, we could just keep NOPs everywhere and instead
make objtool smart enough to detect function fallthroughs. That should
be pretty easy, actually. It already does it for C files.
Something like the below should work, though it's still got a few
issues:
a) objtool is currently disabled for crypto code because it doesn't
yet understand crypto stack re-alignments (which really needs
fixing anyway); and
b) it complains about the blank xen hypercalls falling through. Those
aren't actual functions anyway, so we should probably annotate
those somehow so that objtool ignores them anyway.
I'm a bit swamped at the moment but I can fix those once I get a little
more bandwidth. I at least verified that this patch caught the crypto
missing RETs.
diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile
index a450ad573dcb..a2c52eec2863 100644
--- a/arch/x86/crypto/Makefile
+++ b/arch/x86/crypto/Makefile
@@ -3,8 +3,6 @@
# Arch-specific CryptoAPI modules.
#
-OBJECT_FILES_NON_STANDARD := y
-
avx_supported := $(call as-instr,vpxor %xmm0$(comma)%xmm0$(comma)%xmm0,yes,no)
avx2_supported := $(call as-instr,vpgatherdd %ymm0$(comma)(%eax$(comma)%ymm1\
$(comma)4)$(comma)%ymm2,yes,no)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2928939b98ec..f740fd828cba 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1798,13 +1798,14 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
while (1) {
next_insn = next_insn_same_sec(file, insn);
- if (file->c_file && func && insn->func && func != insn->func->pfunc) {
+ if (func && insn->func && func != insn->func->pfunc) {
WARN("%s() falls through to next function %s()",
func->name, insn->func->name);
return 1;
}
- func = insn->func ? insn->func->pfunc : NULL;
+ if (insn->type != INSN_NOP)
+ func = insn->func ? insn->func->pfunc : NULL;
if (func && insn->ignore) {
WARN_FUNC("BUG: why am I validating an ignored function?",
On Sat, Jun 23, 2018 at 12:36:22PM +0200, Borislav Petkov wrote:
> Lemme send a proper patch now...
>
> ---
> From: Borislav Petkov <[email protected]>
> Date: Sun, 17 Jun 2018 13:57:42 +0200
> Subject: [PATCH] x86/crypto: Add missing RETs
>
> Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> otherwise they run into INT3 padding due to
>
> 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
>
> leading to spurious debug exceptions.
>
> Mike Galbraith <[email protected]> took care of all the remaining callsites.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
ne 1. 7. 2018 o 15:20 Herbert Xu <[email protected]> napísal(a):
>
> On Sat, Jun 23, 2018 at 12:36:22PM +0200, Borislav Petkov wrote:
> > Lemme send a proper patch now...
> >
> > ---
> > From: Borislav Petkov <[email protected]>
> > Date: Sun, 17 Jun 2018 13:57:42 +0200
> > Subject: [PATCH] x86/crypto: Add missing RETs
> >
> > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms
> > otherwise they run into INT3 padding due to
> >
> > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> >
> > leading to spurious debug exceptions.
> >
> > Mike Galbraith <[email protected]> took care of all the remaining callsites.
> >
> > Signed-off-by: Borislav Petkov <[email protected]>
>
> Patch applied. Thanks.
Hi Herbert,
I can see you applied this patch to your cryptodev-2.6 tree (which I
believe is for the next release). Shouldn't this go into the
crypto-2.6 tree so it gets into 4.18-rcX? I'm not sure, but it seems
to me that it qualifies as a (potentially serious?) bug fix.
Also, I think you accidentally extracted the wrong part of the e-mail
as the commit message...
Thanks,
Ondrej
On Sun, Jul 01, 2018 at 05:24:39PM +0200, Ondrej Mosnáček wrote:
>
> I can see you applied this patch to your cryptodev-2.6 tree (which I
> believe is for the next release). Shouldn't this go into the
> crypto-2.6 tree so it gets into 4.18-rcX? I'm not sure, but it seems
> to me that it qualifies as a (potentially serious?) bug fix.
>
> Also, I think you accidentally extracted the wrong part of the e-mail
> as the commit message...
Thanks, it should all be fixed now.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
* Josh Poimboeuf <[email protected]> wrote:
> > So that's still incomplete in that doesn't analyze the 32-bit build yet, right?
>
> We could do INT3s on 64-bit and NOPs on 32-bit.
>
> Or, possibly even better, we could just keep NOPs everywhere and instead
> make objtool smart enough to detect function fallthroughs. That should
> be pretty easy, actually. It already does it for C files.
>
> Something like the below should work, though it's still got a few
> issues:
>
> a) objtool is currently disabled for crypto code because it doesn't
> yet understand crypto stack re-alignments (which really needs
> fixing anyway); and
>
> b) it complains about the blank xen hypercalls falling through. Those
> aren't actual functions anyway, so we should probably annotate
> those somehow so that objtool ignores them anyway.
>
> I'm a bit swamped at the moment but I can fix those once I get a little
> more bandwidth. I at least verified that this patch caught the crypto
> missing RETs.
Great, I'd be perfectly fine with such an approach.
Also, if we have that then we could re-apply Alexey's patch and switch to INT3
(only on 64-bit kernels) without any trouble, because objtool should detect any
execution flow bugs before the INT3 could trigger, right?
I.e. any INT3 fault would show a combination of *both* an objtool bug and a
probable code flow bug - which I suspect would warrant crashing the box ...
Thanks,
Ingo
On Thu, Jul 05, 2018 at 09:58:15AM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> wrote:
>
> > > So that's still incomplete in that doesn't analyze the 32-bit build yet, right?
> >
> > We could do INT3s on 64-bit and NOPs on 32-bit.
> >
> > Or, possibly even better, we could just keep NOPs everywhere and instead
> > make objtool smart enough to detect function fallthroughs. That should
> > be pretty easy, actually. It already does it for C files.
> >
> > Something like the below should work, though it's still got a few
> > issues:
> >
> > a) objtool is currently disabled for crypto code because it doesn't
> > yet understand crypto stack re-alignments (which really needs
> > fixing anyway); and
> >
> > b) it complains about the blank xen hypercalls falling through. Those
> > aren't actual functions anyway, so we should probably annotate
> > those somehow so that objtool ignores them anyway.
> >
> > I'm a bit swamped at the moment but I can fix those once I get a little
> > more bandwidth. I at least verified that this patch caught the crypto
> > missing RETs.
>
> Great, I'd be perfectly fine with such an approach.
>
> Also, if we have that then we could re-apply Alexey's patch and switch to INT3
> (only on 64-bit kernels) without any trouble, because objtool should detect any
> execution flow bugs before the INT3 could trigger, right?
>
> I.e. any INT3 fault would show a combination of *both* an objtool bug and a
> probable code flow bug - which I suspect would warrant crashing the box ...
Sounds good to me. I can take Alexey's patch and submit a 64-bit
version of it, along with the relevant objtool changes (though it may
still be a few weeks before I get the chance).
--
Josh
* Josh Poimboeuf <[email protected]> wrote:
> > Great, I'd be perfectly fine with such an approach.
> >
> > Also, if we have that then we could re-apply Alexey's patch and switch to INT3
> > (only on 64-bit kernels) without any trouble, because objtool should detect any
> > execution flow bugs before the INT3 could trigger, right?
> >
> > I.e. any INT3 fault would show a combination of *both* an objtool bug and a
> > probable code flow bug - which I suspect would warrant crashing the box ...
>
> Sounds good to me. I can take Alexey's patch and submit a 64-bit
> version of it, along with the relevant objtool changes (though it may
> still be a few weeks before I get the chance).
Sounds good to me, thanks!
Ingo