linux-kernel - Re: [PATCH] x86/kvm: fix FASTOP_SIZE when return thunks are enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Jul 2022 11:52:49 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Thadeu Lima de Souza Cascardo <cascardo@...onical.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org,
        stable@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Naresh Kamboju <naresh.kamboju@...aro.org>,
        Borislav Petkov <bp@...e.de>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Linux Kernel Functional Testing <lkft@...aro.org>
Subject: Re: [PATCH] x86/kvm: fix FASTOP_SIZE when return thunks are enabled

On Wed, Jul 13, 2022 at 02:12:41PM -0300, Thadeu Lima de Souza Cascardo wrote:
> The return thunk call makes the fastop functions larger, just like IBT
> does. Consider a 16-byte FASTOP_SIZE when CONFIG_RETHUNK is enabled.
> 
> Otherwise, functions will be incorrectly aligned and when computing their
> position for differently sized operators, they will executed in the middle
> or end of a function, which may as well be an int3, leading to a crash
> like:

Bah.. I did the SETcc stuff, but then forgot about the FASTOP :/

  af2e140f3420 ("x86/kvm: Fix SETcc emulation for return thunks")

> Fixes: aa3d480315ba ("x86: Use return-thunk in asm code")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@...onical.com>
> Cc: Peter Zijlstra (Intel) <peterz@...radead.org>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Josh Poimboeuf <jpoimboe@...nel.org>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Reported-by: Linux Kernel Functional Testing <lkft@...aro.org>
> ---
>  arch/x86/kvm/emulate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index db96bf7d1122..d779eea1052e 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -190,7 +190,7 @@
>  #define X16(x...) X8(x), X8(x)
>  
>  #define NR_FASTOP (ilog2(sizeof(ulong)) + 1)
> -#define FASTOP_SIZE (8 * (1 + HAS_KERNEL_IBT))
> +#define FASTOP_SIZE (8 * (1 + (HAS_KERNEL_IBT | IS_ENABLED(CONFIG_RETHUNK))))

Would it make sense to do something like this instead?

---
 arch/x86/kvm/emulate.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index db96bf7d1122..b4305d2dcc51 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -189,8 +189,12 @@
 #define X8(x...) X4(x), X4(x)
 #define X16(x...) X8(x), X8(x)
 
-#define NR_FASTOP (ilog2(sizeof(ulong)) + 1)
-#define FASTOP_SIZE (8 * (1 + HAS_KERNEL_IBT))
+#define NR_FASTOP	(ilog2(sizeof(ulong)) + 1)
+#define RET_LENGTH	(1 + (4 * IS_ENABLED(CONFIG_RETHUNK)) + \
+			 IS_ENABLED(CONFIG_SLS))
+#define FASTOP_LENGTH	(7 + ENDBR_INSN_SIZE + RET_LENGTH)
+#define FASTOP_SIZE	(8 << ((FASTOP_LENGTH > 8) & 1) << ((FASTOP_LENGTH > 16) & 1))
+static_assert(FASTOP_LENGTH <= FASTOP_SIZE);
 
 struct opcode {
 	u64 flags;
@@ -442,8 +446,6 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
  * RET | JMP __x86_return_thunk	[1,5 bytes; CONFIG_RETHUNK]
  * INT3				[1 byte; CONFIG_SLS]
  */
-#define RET_LENGTH	(1 + (4 * IS_ENABLED(CONFIG_RETHUNK)) + \
-			 IS_ENABLED(CONFIG_SLS))
 #define SETCC_LENGTH	(ENDBR_INSN_SIZE + 3 + RET_LENGTH)
 #define SETCC_ALIGN	(4 << ((SETCC_LENGTH > 4) & 1) << ((SETCC_LENGTH > 8) & 1))
 static_assert(SETCC_LENGTH <= SETCC_ALIGN);