ACK: [SRU][Bionic][Xenial][Trusty][Patch 1/1] x86/paravirt: Fix spectre-v2 mitigations for paravirt guests
Stefan Bader
stefan.bader at canonical.com
Mon Sep 10 16:41:39 UTC 2018
On 10.09.2018 18:35, Kleber Sacilotto de Souza wrote:
> From: Peter Zijlstra <peterz at infradead.org>
>
> CVE-2018-15594
>
> Nadav reported that on guests we're failing to rewrite the indirect
> calls to CALLEE_SAVE paravirt functions. In particular the
> pv_queued_spin_unlock() call is left unpatched and that is all over the
> place. This obviously wrecks Spectre-v2 mitigation (for paravirt
> guests) which relies on not actually having indirect calls around.
>
> The reason is an incorrect clobber test in paravirt_patch_call(); this
> function rewrites an indirect call with a direct call to the _SAME_
> function, there is no possible way the clobbers can be different
> because of this.
>
> Therefore remove this clobber check. Also put WARNs on the other patch
> failure case (not enough room for the instruction) which I've not seen
> trigger in my (limited) testing.
>
> Three live kernel image disassemblies for lock_sock_nested (as a small
> function that illustrates the problem nicely). PRE is the current
> situation for guests, POST is with this patch applied and NATIVE is with
> or without the patch for !guests.
>
> PRE:
>
> (gdb) disassemble lock_sock_nested
> Dump of assembler code for function lock_sock_nested:
> 0xffffffff817be970 <+0>: push %rbp
> 0xffffffff817be971 <+1>: mov %rdi,%rbp
> 0xffffffff817be974 <+4>: push %rbx
> 0xffffffff817be975 <+5>: lea 0x88(%rbp),%rbx
> 0xffffffff817be97c <+12>: callq 0xffffffff819f7160 <_cond_resched>
> 0xffffffff817be981 <+17>: mov %rbx,%rdi
> 0xffffffff817be984 <+20>: callq 0xffffffff819fbb00 <_raw_spin_lock_bh>
> 0xffffffff817be989 <+25>: mov 0x8c(%rbp),%eax
> 0xffffffff817be98f <+31>: test %eax,%eax
> 0xffffffff817be991 <+33>: jne 0xffffffff817be9ba <lock_sock_nested+74>
> 0xffffffff817be993 <+35>: movl $0x1,0x8c(%rbp)
> 0xffffffff817be99d <+45>: mov %rbx,%rdi
> 0xffffffff817be9a0 <+48>: callq *0xffffffff822299e8
> 0xffffffff817be9a7 <+55>: pop %rbx
> 0xffffffff817be9a8 <+56>: pop %rbp
> 0xffffffff817be9a9 <+57>: mov $0x200,%esi
> 0xffffffff817be9ae <+62>: mov $0xffffffff817be993,%rdi
> 0xffffffff817be9b5 <+69>: jmpq 0xffffffff81063ae0 <__local_bh_enable_ip>
> 0xffffffff817be9ba <+74>: mov %rbp,%rdi
> 0xffffffff817be9bd <+77>: callq 0xffffffff817be8c0 <__lock_sock>
> 0xffffffff817be9c2 <+82>: jmp 0xffffffff817be993 <lock_sock_nested+35>
> End of assembler dump.
>
> POST:
>
> (gdb) disassemble lock_sock_nested
> Dump of assembler code for function lock_sock_nested:
> 0xffffffff817be970 <+0>: push %rbp
> 0xffffffff817be971 <+1>: mov %rdi,%rbp
> 0xffffffff817be974 <+4>: push %rbx
> 0xffffffff817be975 <+5>: lea 0x88(%rbp),%rbx
> 0xffffffff817be97c <+12>: callq 0xffffffff819f7160 <_cond_resched>
> 0xffffffff817be981 <+17>: mov %rbx,%rdi
> 0xffffffff817be984 <+20>: callq 0xffffffff819fbb00 <_raw_spin_lock_bh>
> 0xffffffff817be989 <+25>: mov 0x8c(%rbp),%eax
> 0xffffffff817be98f <+31>: test %eax,%eax
> 0xffffffff817be991 <+33>: jne 0xffffffff817be9ba <lock_sock_nested+74>
> 0xffffffff817be993 <+35>: movl $0x1,0x8c(%rbp)
> 0xffffffff817be99d <+45>: mov %rbx,%rdi
> 0xffffffff817be9a0 <+48>: callq 0xffffffff810a0c20 <__raw_callee_save___pv_queued_spin_unlock>
> 0xffffffff817be9a5 <+53>: xchg %ax,%ax
> 0xffffffff817be9a7 <+55>: pop %rbx
> 0xffffffff817be9a8 <+56>: pop %rbp
> 0xffffffff817be9a9 <+57>: mov $0x200,%esi
> 0xffffffff817be9ae <+62>: mov $0xffffffff817be993,%rdi
> 0xffffffff817be9b5 <+69>: jmpq 0xffffffff81063aa0 <__local_bh_enable_ip>
> 0xffffffff817be9ba <+74>: mov %rbp,%rdi
> 0xffffffff817be9bd <+77>: callq 0xffffffff817be8c0 <__lock_sock>
> 0xffffffff817be9c2 <+82>: jmp 0xffffffff817be993 <lock_sock_nested+35>
> End of assembler dump.
>
> NATIVE:
>
> (gdb) disassemble lock_sock_nested
> Dump of assembler code for function lock_sock_nested:
> 0xffffffff817be970 <+0>: push %rbp
> 0xffffffff817be971 <+1>: mov %rdi,%rbp
> 0xffffffff817be974 <+4>: push %rbx
> 0xffffffff817be975 <+5>: lea 0x88(%rbp),%rbx
> 0xffffffff817be97c <+12>: callq 0xffffffff819f7160 <_cond_resched>
> 0xffffffff817be981 <+17>: mov %rbx,%rdi
> 0xffffffff817be984 <+20>: callq 0xffffffff819fbb00 <_raw_spin_lock_bh>
> 0xffffffff817be989 <+25>: mov 0x8c(%rbp),%eax
> 0xffffffff817be98f <+31>: test %eax,%eax
> 0xffffffff817be991 <+33>: jne 0xffffffff817be9ba <lock_sock_nested+74>
> 0xffffffff817be993 <+35>: movl $0x1,0x8c(%rbp)
> 0xffffffff817be99d <+45>: mov %rbx,%rdi
> 0xffffffff817be9a0 <+48>: movb $0x0,(%rdi)
> 0xffffffff817be9a3 <+51>: nopl 0x0(%rax)
> 0xffffffff817be9a7 <+55>: pop %rbx
> 0xffffffff817be9a8 <+56>: pop %rbp
> 0xffffffff817be9a9 <+57>: mov $0x200,%esi
> 0xffffffff817be9ae <+62>: mov $0xffffffff817be993,%rdi
> 0xffffffff817be9b5 <+69>: jmpq 0xffffffff81063ae0 <__local_bh_enable_ip>
> 0xffffffff817be9ba <+74>: mov %rbp,%rdi
> 0xffffffff817be9bd <+77>: callq 0xffffffff817be8c0 <__lock_sock>
> 0xffffffff817be9c2 <+82>: jmp 0xffffffff817be993 <lock_sock_nested+35>
> End of assembler dump.
>
>
> Fixes: 63f70270ccd9 ("[PATCH] i386: PARAVIRT: add common patching machinery")
> Fixes: 3010a0663fd9 ("x86/paravirt, objtool: Annotate indirect calls")
> Reported-by: Nadav Amit <namit at vmware.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz at infradead.org>
> Signed-off-by: Thomas Gleixner <tglx at linutronix.de>
> Reviewed-by: Juergen Gross <jgross at suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk at oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com>
> Cc: David Woodhouse <dwmw2 at infradead.org>
> Cc: stable at vger.kernel.org
> (cherry picked from commit 5800dc5c19f34e6e03b5adab1282535cb102fafd)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---
> arch/x86/kernel/paravirt.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index 99dc79e76bdc..930c88341e4e 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -88,10 +88,12 @@ unsigned paravirt_patch_call(void *insnbuf,
> struct branch *b = insnbuf;
> unsigned long delta = (unsigned long)target - (addr+5);
>
> - if (tgt_clobbers & ~site_clobbers)
> - return len; /* target would clobber too much for this site */
> - if (len < 5)
> + if (len < 5) {
> +#ifdef CONFIG_RETPOLINE
> + WARN_ONCE("Failing to patch indirect CALL in %ps\n", (void *)addr);
> +#endif
> return len; /* call too long for patch site */
> + }
>
> b->opcode = 0xe8; /* call */
> b->delta = delta;
> @@ -106,8 +108,12 @@ unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
> struct branch *b = insnbuf;
> unsigned long delta = (unsigned long)target - (addr+5);
>
> - if (len < 5)
> + if (len < 5) {
> +#ifdef CONFIG_RETPOLINE
> + WARN_ONCE("Failing to patch indirect JMP in %ps\n", (void *)addr);
> +#endif
> return len; /* call too long for patch site */
> + }
>
> b->opcode = 0xe9; /* jmp */
> b->delta = delta;
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180910/e3354266/attachment.sig>
More information about the kernel-team
mailing list