lkml.org 
[lkml]   [2018]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 45/63] x86/paravirt: Fix spectre-v2 mitigations for paravirt guests
    3.16.58-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Peter Zijlstra <peterz@infradead.org>

    commit 5800dc5c19f34e6e03b5adab1282535cb102fafd upstream.

    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@vmware.com>
    Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Reviewed-by: Juergen Gross <jgross@suse.com>
    Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
    Cc: David Woodhouse <dwmw2@infradead.org>
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    arch/x86/kernel/paravirt.c | 14 ++++++++++----
    1 file changed, 10 insertions(+), 4 deletions(-)

    --- a/arch/x86/kernel/paravirt.c
    +++ b/arch/x86/kernel/paravirt.c
    @@ -97,10 +97,12 @@ unsigned paravirt_patch_call(void *insnb
    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;
    @@ -115,8 +117,12 @@ unsigned paravirt_patch_jmp(void *insnbu
    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;
    \
     
     \ /
      Last update: 2018-09-22 02:24    [W:4.160 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site