[PATCH] x86/crypto: Add missing RETs

[PATCH] x86/crypto: Add missing RETs

[Borislav Petkov]

Lemme send a proper patch now...

---
From: Borislav Petkov <bp@suse.de>
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 <efault@gmx.de> took care of all the remaining callsites.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 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.

Re: [PATCH] x86/crypto: Add missing RETs

[Ondrej Mosnáček]

so 23. 6. 2018 o 12:36 Borislav Petkov <bp@alien8.de> napísal(a):
>
> Lemme send a proper patch now...
>
> ---
> From: Borislav Petkov <bp@suse.de>
> 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 <efault@gmx.de> took care of all the remaining callsites.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Oh, thanks for fixing that!

Acked-by: Ondrej Mosnacek <omosnacek@gmail.com>

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.

Re: [PATCH] x86/crypto: Add missing RETs

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> Lemme send a proper patch now...
> 
> ---
> From: Borislav Petkov <bp@suse.de>
> 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 <efault@gmx.de> 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

Re: [PATCH] x86/crypto: Add missing RETs

[Thomas Gleixner]

On Sun, 24 Jun 2018, Ingo Molnar wrote:
> * Borislav Petkov <bp@alien8.de> wrote:
> 
> > Lemme send a proper patch now...
> > 
> > ---
> > From: Borislav Petkov <bp@suse.de>
> > 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 <efault@gmx.de> 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

Re: [PATCH] x86/crypto: Add missing RETs

[Borislav Petkov]

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.

Re: [PATCH] x86/crypto: Add missing RETs

[Alexey Dobriyan]

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 <efault@gmx.de> 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

Re: [PATCH] x86/crypto: Add missing RETs

[Ingo Molnar]

* Alexey Dobriyan <adobriyan@gmail.com> 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 <efault@gmx.de> 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/20180518073644.GA8593@gmail.com/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

Re: [PATCH] x86/crypto: Add missing RETs

[Josh Poimboeuf]

On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
> 
> * Alexey Dobriyan <adobriyan@gmail.com> 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 <efault@gmx.de> 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/20180518073644.GA8593@gmail.com/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

Re: [PATCH] x86/crypto: Add missing RETs

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
> > 
> > * Alexey Dobriyan <adobriyan@gmail.com> 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 <efault@gmx.de> 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/20180518073644.GA8593@gmail.com/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

Re: [PATCH] x86/crypto: Add missing RETs

[Josh Poimboeuf]

On Tue, Jun 26, 2018 at 08:49:30AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
> > > 
> > > * Alexey Dobriyan <adobriyan@gmail.com> 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 <efault@gmx.de> 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/20180518073644.GA8593@gmail.com/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?",

Re: [PATCH] x86/crypto: Add missing RETs

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> 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

Re: [PATCH] x86/crypto: Add missing RETs

[Josh Poimboeuf]

On Thu, Jul 05, 2018 at 09:58:15AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> 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

Re: [PATCH] x86/crypto: Add missing RETs

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> 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

Re: [PATCH] x86/crypto: Add missing RETs

[Herbert Xu]

On Sat, Jun 23, 2018 at 12:36:22PM +0200, Borislav Petkov wrote:
> Lemme send a proper patch now...
> 
> ---
> From: Borislav Petkov <bp@suse.de>
> 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 <efault@gmx.de> took care of all the remaining callsites.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] x86/crypto: Add missing RETs

[Ondrej Mosnáček]

ne 1. 7. 2018 o 15:20 Herbert Xu <herbert@gondor.apana.org.au> 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 <bp@suse.de>
> > 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 <efault@gmx.de> took care of all the remaining callsites.
> >
> > Signed-off-by: Borislav Petkov <bp@suse.de>
>
> 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

Re: [PATCH] x86/crypto: Add missing RETs

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt