| Message ID | 20200831111832.25275-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pv: Fix multiple bugs with SEGBASE_GS_USER_SEL | expand |
On 31.08.2020 13:18, Andrew Cooper wrote: > --- a/xen/arch/x86/x86_64/mm.c > +++ b/xen/arch/x86/x86_64/mm.c > @@ -1059,16 +1059,52 @@ long do_set_segment_base(unsigned int which, unsigned long base) > break; > > case SEGBASE_GS_USER_SEL: > - __asm__ __volatile__ ( > - " swapgs \n" > - "1: movl %k0,%%gs \n" > - " "safe_swapgs" \n" > - ".section .fixup,\"ax\" \n" > - "2: xorl %k0,%k0 \n" > - " jmp 1b \n" > - ".previous \n" > - _ASM_EXTABLE(1b, 2b) > - : "+r" (base) ); > + /* > + * We wish to update the user %gs from the GDT/LDT. Currently, the > + * guest kernel's GS_BASE is in context. > + */ > + asm volatile ( "swapgs" ); > + > + if ( base <= 3 ) Either !(base & 0xfffc) or you want to truncate the input to uint16_t first. The upper 48 bits have always been ignored. With this addressed one way or another Reviewed-by: Jan Beulich <jbeulich@suse.com> Yet two more minor comments: > + { > + /* Work around NUL segment behaviour on AMD hardware. */ > + if ( boot_cpu_data.x86_vendor & > + (X86_VENDOR_AMD | X86_VENDOR_HYGON) ) > + asm volatile ( "mov %[sel], %%gs" > + :: [sel] "rm" (FLAT_USER_DS32) ); > + } > + else > + /* Fix up RPL. */ > + base |= 3; For a fair part of this block you could save a level of indentation by inverting the initial condition and using "else if" later on. > + /* > + * Load the chosen selector, with fault handling. > + * > + * Errors ought to fail the hypercall, but that was never built in > + * originally, and Linux will BUG() if this call fails. > + * > + * NUL the selector in the case of an error. This too needs to deal > + * with the AMD NUL segment behaviour, but it is already a slowpath in > + * #GP context so perform the flat load unconditionally to avoid > + * complicated logic. > + * > + * Anyone wanting to check for errors from this hypercall should > + * re-read %gs and compare against the input 'base' selector. > + */ > + asm volatile ( "1: mov %k[sel], %%gs\n\t" > + ".section .fixup, \"ax\", @progbits\n\t" > + "2: mov %k[flat], %%gs\n\t" > + " xor %k[sel], %k[sel]\n\t" > + " jmp 1b\n\t" > + ".previous\n\t" > + _ASM_EXTABLE(1b, 2b) > + : [sel] "+r" (base) > + : [flat] "rm" (FLAT_USER_DS32) ); "m" is pointless to specify here - the compiler won't instantiate a memory variable when the value is an immediate. This can be observed best when you specify _just_ "m" here. Jan
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index 29048d34dc..95ee05cd5d 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -1059,16 +1059,52 @@ long do_set_segment_base(unsigned int which, unsigned long base) break; case SEGBASE_GS_USER_SEL: - __asm__ __volatile__ ( - " swapgs \n" - "1: movl %k0,%%gs \n" - " "safe_swapgs" \n" - ".section .fixup,\"ax\" \n" - "2: xorl %k0,%k0 \n" - " jmp 1b \n" - ".previous \n" - _ASM_EXTABLE(1b, 2b) - : "+r" (base) ); + /* + * We wish to update the user %gs from the GDT/LDT. Currently, the + * guest kernel's GS_BASE is in context. + */ + asm volatile ( "swapgs" ); + + if ( base <= 3 ) + { + /* Work around NUL segment behaviour on AMD hardware. */ + if ( boot_cpu_data.x86_vendor & + (X86_VENDOR_AMD | X86_VENDOR_HYGON) ) + asm volatile ( "mov %[sel], %%gs" + :: [sel] "rm" (FLAT_USER_DS32) ); + } + else + /* Fix up RPL. */ + base |= 3; + + /* + * Load the chosen selector, with fault handling. + * + * Errors ought to fail the hypercall, but that was never built in + * originally, and Linux will BUG() if this call fails. + * + * NUL the selector in the case of an error. This too needs to deal + * with the AMD NUL segment behaviour, but it is already a slowpath in + * #GP context so perform the flat load unconditionally to avoid + * complicated logic. + * + * Anyone wanting to check for errors from this hypercall should + * re-read %gs and compare against the input 'base' selector. + */ + asm volatile ( "1: mov %k[sel], %%gs\n\t" + ".section .fixup, \"ax\", @progbits\n\t" + "2: mov %k[flat], %%gs\n\t" + " xor %k[sel], %k[sel]\n\t" + " jmp 1b\n\t" + ".previous\n\t" + _ASM_EXTABLE(1b, 2b) + : [sel] "+r" (base) + : [flat] "rm" (FLAT_USER_DS32) ); + + /* Update the cache of the inactive base, as read from the GDT/LDT. */ + v->arch.pv.gs_base_user = rdgsbase(); + + asm volatile ( safe_swapgs ); break; default:
The logic takes the segment selector unmodified from guest context. This allowed the guest to load DPL0 descriptors into %gs. Fix up the RPL for non-NUL selectors to be 3. Xen's context switch logic skips saving the inactive %gs base, as it cannot be modified by the guest behind Xen's back. This depends on Xen caching updates to the inactive base, which is was missing from this path. The consequence is that, following SEGBASE_GS_USER_SEL, the next context switch will restore the stale inactive %gs base, and corrupt vcpu state. Rework the hypercall to update the cached idea of gs_base_user, and fix the behaviour in the case of the AMD NUL selector bug to always zero the segment base. Reported-by: Andy Lutomirski <luto@kernel.org> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Andy Lutomirski <luto@kernel.org> --- xen/arch/x86/x86_64/mm.c | 56 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 10 deletions(-)