| Message ID | 20180910163555.11798-2-kleber.souza@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix for CVE-2018-15594 | expand |
On 09/10/2018 11:35 AM, Kleber Sacilotto de Souza wrote: > From: Peter Zijlstra <peterz@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@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> > Cc: stable@vger.kernel.org > (cherry picked from commit 5800dc5c19f34e6e03b5adab1282535cb102fafd) > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Acked-by: Tyler Hicks <tyhicks@canonical.com> Thanks! Tyler > --- > 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; >
On 10.09.2018 18:35, Kleber Sacilotto de Souza wrote: > From: Peter Zijlstra <peterz@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@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> > Cc: stable@vger.kernel.org > (cherry picked from commit 5800dc5c19f34e6e03b5adab1282535cb102fafd) > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Acked-by: Stefan Bader <stefan.bader@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; >
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;