* [PATCH v2 0/2] x86: initial simplistic Meltdown mitigation
@ 2018-01-15 11:01 Jan Beulich
2018-01-15 11:06 ` [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 29+ messages in thread
From: Jan Beulich @ 2018-01-15 11:01 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
1: x86: Meltdown band-aid against malicious 64-bit PV guests
2: x86: allow Meltdown band-aid to be disabled
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Addressing review comments for patch 1 (see there) and new
patch 2.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-15 11:01 [PATCH v2 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
@ 2018-01-15 11:06 ` Jan Beulich
2018-01-15 18:23 ` Andrew Cooper
2018-01-15 11:07 ` [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
2018-01-16 15:16 ` [PATCH v3 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
2 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-01-15 11:06 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
This is a very simplistic change limiting the amount of memory a running
64-bit PV guest has mapped (and hence available for attacking): Only the
mappings of stack, IDT, and TSS are being cloned from the direct map
into per-CPU page tables. Guest controlled parts of the page tables are
being copied into those per-CPU page tables upon entry into the guest.
Cross-vCPU synchronization of top level page table entry changes is
being effected by forcing other active vCPU-s of the guest into the
hypervisor.
The change to context_switch() isn't strictly necessary, but there's no
reason to keep switching page tables once a PV guest is being scheduled
out.
There is certainly much room for improvement, especially of performance,
here - first and foremost suppressing all the negative effects on AMD
systems. But in the interest of backportability (including to really old
hypervisors, which may not even have alternative patching) any such is
being left out here.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use sensible numbers for the register classification constants also
for the low 8 registers. Insert 'r' into their names to make their
purpose more clear. Use "sub" instead of "add" when adding an
immediate of 128. Defer sync IPI in do_mmu_update() until the end of
the main loop. Use flush IPI there instead event check one. Make
setup functions return a proper error code. Use better suited local
variables in clone_mapping(). Add comment to new struct cpu_info
fields.
---
This also wants Andrew's "[PATCH RFC 11/44] x86/pt-shadow: Always set
_PAGE_ACCESSED on L4e updates".
---
Backporting notes:
- This needs f9eb74789a ("x86/entry: Remove support for partial
cpu_user_regs frames") as a prereq, due to the uses of %r14 and %r15.
But that's intended to be backported anyway (for Spectre/SP2).
- The use of "root" instead of "l4" here is mainly to not make 5-level
page table additions any harder. In backports "l4" should probably be
preferred.
---
Follow-up notes:
- use alternatives patching for fully suppressing performance effects
when disabled
- check whether running globally with CR4.PGE clear helps overall
performance
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1511,6 +1511,9 @@ void paravirt_ctxt_switch_to(struct vcpu
{
unsigned long cr4;
+ this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
+ l4e_from_page(v->domain->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
+
cr4 = pv_guest_cr4_to_real_cr4(v);
if ( unlikely(cr4 != read_cr4()) )
write_cr4(cr4);
@@ -1682,6 +1685,8 @@ void context_switch(struct vcpu *prev, s
ASSERT(local_irq_is_enabled());
+ get_cpu_info()->xen_cr3 = 0;
+
cpumask_copy(&dirty_mask, next->vcpu_dirty_cpumask);
/* Allow at most one CPU at a time to be dirty. */
ASSERT(cpumask_weight(&dirty_mask) <= 1);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3520,6 +3520,7 @@ long do_mmu_update(
struct vcpu *curr = current, *v = curr;
struct domain *d = v->domain, *pt_owner = d, *pg_owner;
mfn_t map_mfn = INVALID_MFN;
+ bool sync_guest = false;
uint32_t xsm_needed = 0;
uint32_t xsm_checked = 0;
int rc = put_old_guest_table(curr);
@@ -3683,6 +3684,8 @@ long do_mmu_update(
break;
rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+ if ( !rc )
+ sync_guest = true;
break;
case PGT_writable_page:
@@ -3787,6 +3790,24 @@ long do_mmu_update(
if ( va )
unmap_domain_page(va);
+ if ( sync_guest )
+ {
+ /*
+ * Force other vCPU-s of the affected guest to pick up L4 entry
+ * changes (if any). Issue a flush IPI with empty operation mask to
+ * facilitate this (including ourselves waiting for the IPI to
+ * actually have arrived). Utilize the fact that FLUSH_VA_VALID is
+ * meaningless without FLUSH_CACHE, but will allow to pass the no-op
+ * check in flush_area_mask().
+ */
+ unsigned int cpu = smp_processor_id();
+ cpumask_t *mask = per_cpu(scratch_cpumask, cpu);
+
+ cpumask_andnot(mask, pt_owner->domain_dirty_cpumask, cpumask_of(cpu));
+ if ( !cpumask_empty(mask) )
+ flush_area_mask(mask, ZERO_BLOCK_PTR, FLUSH_VA_VALID);
+ }
+
perfc_add(num_page_updates, i);
out:
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -327,6 +327,9 @@ void start_secondary(void *unused)
*/
spin_debug_disable();
+ get_cpu_info()->xen_cr3 = 0;
+ get_cpu_info()->pv_cr3 = __pa(this_cpu(root_pgt));
+
load_system_tables();
/* Full exception support from here on in. */
@@ -633,6 +636,187 @@ void cpu_exit_clear(unsigned int cpu)
set_cpu_state(CPU_STATE_DEAD);
}
+static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
+{
+ unsigned long linear = (unsigned long)ptr, pfn;
+ unsigned int flags;
+ l3_pgentry_t *pl3e = l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) +
+ l3_table_offset(linear);
+ l2_pgentry_t *pl2e;
+ l1_pgentry_t *pl1e;
+
+ if ( linear < DIRECTMAP_VIRT_START )
+ return 0;
+
+ flags = l3e_get_flags(*pl3e);
+ ASSERT(flags & _PAGE_PRESENT);
+ if ( flags & _PAGE_PSE )
+ {
+ pfn = (l3e_get_pfn(*pl3e) & ~((1UL << (2 * PAGETABLE_ORDER)) - 1)) |
+ (PFN_DOWN(linear) & ((1UL << (2 * PAGETABLE_ORDER)) - 1));
+ flags &= ~_PAGE_PSE;
+ }
+ else
+ {
+ pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(linear);
+ flags = l2e_get_flags(*pl2e);
+ ASSERT(flags & _PAGE_PRESENT);
+ if ( flags & _PAGE_PSE )
+ {
+ pfn = (l2e_get_pfn(*pl2e) & ~((1UL << PAGETABLE_ORDER) - 1)) |
+ (PFN_DOWN(linear) & ((1UL << PAGETABLE_ORDER) - 1));
+ flags &= ~_PAGE_PSE;
+ }
+ else
+ {
+ pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
+ flags = l1e_get_flags(*pl1e);
+ if ( !(flags & _PAGE_PRESENT) )
+ return 0;
+ pfn = l1e_get_pfn(*pl1e);
+ }
+ }
+
+ if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) )
+ {
+ pl3e = alloc_xen_pagetable();
+ if ( !pl3e )
+ return -ENOMEM;
+ clear_page(pl3e);
+ l4e_write(&rpt[root_table_offset(linear)],
+ l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR));
+ }
+ else
+ pl3e = l4e_to_l3e(rpt[root_table_offset(linear)]);
+
+ pl3e += l3_table_offset(linear);
+
+ if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
+ {
+ pl2e = alloc_xen_pagetable();
+ if ( !pl2e )
+ return -ENOMEM;
+ clear_page(pl2e);
+ l3e_write(pl3e, l3e_from_paddr(__pa(pl2e), __PAGE_HYPERVISOR));
+ }
+ else
+ {
+ ASSERT(!(l3e_get_flags(*pl3e) & _PAGE_PSE));
+ pl2e = l3e_to_l2e(*pl3e);
+ }
+
+ pl2e += l2_table_offset(linear);
+
+ if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
+ {
+ pl1e = alloc_xen_pagetable();
+ if ( !pl1e )
+ return -ENOMEM;
+ clear_page(pl1e);
+ l2e_write(pl2e, l2e_from_paddr(__pa(pl1e), __PAGE_HYPERVISOR));
+ }
+ else
+ {
+ ASSERT(!(l2e_get_flags(*pl2e) & _PAGE_PSE));
+ pl1e = l2e_to_l1e(*pl2e);
+ }
+
+ pl1e += l1_table_offset(linear);
+
+ if ( l1e_get_flags(*pl1e) & _PAGE_PRESENT )
+ {
+ ASSERT(l1e_get_pfn(*pl1e) == pfn);
+ ASSERT(l1e_get_flags(*pl1e) == flags);
+ }
+ else
+ l1e_write(pl1e, l1e_from_pfn(pfn, flags));
+
+ return 0;
+}
+
+DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
+
+static int setup_cpu_root_pgt(unsigned int cpu)
+{
+ root_pgentry_t *rpt = alloc_xen_pagetable();
+ unsigned int off;
+ int rc;
+
+ if ( !rpt )
+ return -ENOMEM;
+
+ clear_page(rpt);
+ per_cpu(root_pgt, cpu) = rpt;
+
+ rpt[root_table_offset(RO_MPT_VIRT_START)] =
+ idle_pg_table[root_table_offset(RO_MPT_VIRT_START)];
+ /* SH_LINEAR_PT inserted together with guest mappings. */
+ /* PERDOMAIN inserted during context switch. */
+ rpt[root_table_offset(XEN_VIRT_START)] =
+ idle_pg_table[root_table_offset(XEN_VIRT_START)];
+
+ /* Install direct map page table entries for stack, IDT, and TSS. */
+ for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
+ rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
+
+ if ( !rc )
+ rc = clone_mapping(idt_tables[cpu], rpt);
+ if ( !rc )
+ rc = clone_mapping(&per_cpu(init_tss, cpu), rpt);
+
+ return rc;
+}
+
+static void cleanup_cpu_root_pgt(unsigned int cpu)
+{
+ root_pgentry_t *rpt = per_cpu(root_pgt, cpu);
+ unsigned int r;
+
+ if ( !rpt )
+ return;
+
+ per_cpu(root_pgt, cpu) = NULL;
+
+ for ( r = root_table_offset(DIRECTMAP_VIRT_START);
+ r < root_table_offset(HYPERVISOR_VIRT_END); ++r )
+ {
+ l3_pgentry_t *l3t;
+ unsigned int i3;
+
+ if ( !(root_get_flags(rpt[r]) & _PAGE_PRESENT) )
+ continue;
+
+ l3t = l4e_to_l3e(rpt[r]);
+
+ for ( i3 = 0; i3 < L3_PAGETABLE_ENTRIES; ++i3 )
+ {
+ l2_pgentry_t *l2t;
+ unsigned int i2;
+
+ if ( !(l3e_get_flags(l3t[i3]) & _PAGE_PRESENT) )
+ continue;
+
+ ASSERT(!(l3e_get_flags(l3t[i3]) & _PAGE_PSE));
+ l2t = l3e_to_l2e(l3t[i3]);
+
+ for ( i2 = 0; i2 < L2_PAGETABLE_ENTRIES; ++i2 )
+ {
+ if ( !(l2e_get_flags(l2t[i2]) & _PAGE_PRESENT) )
+ continue;
+
+ ASSERT(!(l2e_get_flags(l2t[i2]) & _PAGE_PSE));
+ free_xen_pagetable(l2e_to_l1e(l2t[i2]));
+ }
+
+ free_xen_pagetable(l2t);
+ }
+
+ free_xen_pagetable(l3t);
+ }
+
+ free_xen_pagetable(rpt);
+}
+
static void cpu_smpboot_free(unsigned int cpu)
{
unsigned int order, socket = cpu_to_socket(cpu);
@@ -671,6 +855,8 @@ static void cpu_smpboot_free(unsigned in
free_domheap_page(mfn_to_page(mfn));
}
+ cleanup_cpu_root_pgt(cpu);
+
order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
free_xenheap_pages(per_cpu(gdt_table, cpu), order);
@@ -727,6 +913,11 @@ static int cpu_smpboot_alloc(unsigned in
set_ist(&idt_tables[cpu][TRAP_nmi], IST_NONE);
set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
+ rc = setup_cpu_root_pgt(cpu);
+ if ( rc )
+ goto out;
+ rc = -ENOMEM;
+
for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
i < nr_cpu_ids && i <= (cpu | (STUBS_PER_PAGE - 1)); ++i )
if ( cpu_online(i) && cpu_to_node(i) == node )
@@ -786,6 +977,8 @@ static struct notifier_block cpu_smpboot
void __init smp_prepare_cpus(unsigned int max_cpus)
{
+ int rc;
+
register_cpu_notifier(&cpu_smpboot_nfb);
mtrr_aps_sync_begin();
@@ -799,6 +992,11 @@ void __init smp_prepare_cpus(unsigned in
stack_base[0] = stack_start;
+ rc = setup_cpu_root_pgt(0);
+ if ( rc )
+ panic("Error %d setting up PV root page table\n", rc);
+ get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
+
set_nr_sockets();
socket_cpumask = xzalloc_array(cpumask_t *, nr_sockets);
@@ -867,6 +1065,8 @@ void __init smp_prepare_boot_cpu(void)
#if NR_CPUS > 2 * BITS_PER_LONG
per_cpu(scratch_cpumask, cpu) = &scratch_cpu0mask;
#endif
+
+ get_cpu_info()->xen_cr3 = 0;
}
static void
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -137,6 +137,8 @@ void __dummy__(void)
OFFSET(CPUINFO_processor_id, struct cpu_info, processor_id);
OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
OFFSET(CPUINFO_cr4, struct cpu_info, cr4);
+ OFFSET(CPUINFO_xen_cr3, struct cpu_info, xen_cr3);
+ OFFSET(CPUINFO_pv_cr3, struct cpu_info, pv_cr3);
DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
BLANK();
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -199,6 +199,17 @@ ENTRY(cstar_enter)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+UNLIKELY_START(nz, cstar_cr3)
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+UNLIKELY_END(cstar_cr3)
+
GET_CURRENT(bx)
movq VCPU_domain(%rbx),%rcx
cmpb $0,DOMAIN_is_32bit_pv(%rcx)
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -37,6 +37,32 @@ ENTRY(switch_to_kernel)
/* %rbx: struct vcpu, interrupts disabled */
restore_all_guest:
ASSERT_INTERRUPTS_DISABLED
+
+ /* Copy guest mappings and switch to per-CPU root page table. */
+ mov %cr3, %r9
+ GET_STACK_END(dx)
+ mov STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rdi
+ movabs $PADDR_MASK & PAGE_MASK, %rsi
+ movabs $DIRECTMAP_VIRT_START, %rcx
+ mov %rdi, %rax
+ and %rsi, %rdi
+ and %r9, %rsi
+ add %rcx, %rdi
+ add %rcx, %rsi
+ mov $ROOT_PAGETABLE_FIRST_XEN_SLOT, %ecx
+ mov root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rsi), %r8
+ mov %r8, root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rdi)
+ rep movsq
+ mov $ROOT_PAGETABLE_ENTRIES - \
+ ROOT_PAGETABLE_LAST_XEN_SLOT - 1, %ecx
+ sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
+ ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rsi
+ sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
+ ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi
+ rep movsq
+ mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
+ write_cr3 rax, rdi, rsi
+
RESTORE_ALL
testw $TRAP_syscall,4(%rsp)
jz iret_exit_to_guest
@@ -71,6 +97,18 @@ iret_exit_to_guest:
ALIGN
/* No special register assumptions. */
restore_all_xen:
+ /*
+ * Check whether we need to switch to the per-CPU page tables, in
+ * case we return to late PV exit code (from an NMI or #MC).
+ */
+ GET_STACK_END(ax)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rax), %rdx
+ mov STACK_CPUINFO_FIELD(pv_cr3)(%rax), %rax
+ test %rdx, %rdx
+UNLIKELY_START(g, exit_cr3)
+ write_cr3 rax, rdi, rsi
+UNLIKELY_END(exit_cr3)
+
RESTORE_ALL adj=8
iretq
@@ -100,7 +138,18 @@ ENTRY(lstar_enter)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
- GET_CURRENT(bx)
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+ jz .Llstar_cr3_okay
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+.Llstar_cr3_okay:
+
+ __GET_CURRENT(bx)
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jz switch_to_kernel
@@ -192,7 +241,18 @@ GLOBAL(sysenter_eflags_saved)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
- GET_CURRENT(bx)
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+ jz .Lsyse_cr3_okay
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+.Lsyse_cr3_okay:
+
+ __GET_CURRENT(bx)
cmpb $0,VCPU_sysenter_disables_events(%rbx)
movq VCPU_sysenter_addr(%rbx),%rax
setne %cl
@@ -228,13 +288,23 @@ ENTRY(int80_direct_trap)
movl $0x80, 4(%rsp)
SAVE_ALL
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+UNLIKELY_START(nz, int80_cr3)
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+UNLIKELY_END(int80_cr3)
+
cmpb $0,untrusted_msi(%rip)
UNLIKELY_START(ne, msi_check)
movl $0x80,%edi
call check_for_unexpected_msi
UNLIKELY_END(msi_check)
- GET_CURRENT(bx)
+ __GET_CURRENT(bx)
/* Check that the callback is non-null. */
leaq VCPU_int80_bounce(%rbx),%rdx
@@ -391,9 +461,27 @@ ENTRY(dom_crash_sync_extable)
ENTRY(common_interrupt)
SAVE_ALL CLAC
+
+ GET_STACK_END(14)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
+ mov %rcx, %r15
+ neg %rcx
+ jz .Lintr_cr3_okay
+ jns .Lintr_cr3_load
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ neg %rcx
+.Lintr_cr3_load:
+ write_cr3 rcx, rdi, rsi
+ xor %ecx, %ecx
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ testb $3, UREGS_cs(%rsp)
+ cmovnz %rcx, %r15
+.Lintr_cr3_okay:
+
CR4_PV32_RESTORE
movq %rsp,%rdi
callq do_IRQ
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
jmp ret_from_intr
/* No special register assumptions. */
@@ -411,6 +499,23 @@ ENTRY(page_fault)
/* No special register assumptions. */
GLOBAL(handle_exception)
SAVE_ALL CLAC
+
+ GET_STACK_END(14)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
+ mov %rcx, %r15
+ neg %rcx
+ jz .Lxcpt_cr3_okay
+ jns .Lxcpt_cr3_load
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ neg %rcx
+.Lxcpt_cr3_load:
+ write_cr3 rcx, rdi, rsi
+ xor %ecx, %ecx
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ testb $3, UREGS_cs(%rsp)
+ cmovnz %rcx, %r15
+.Lxcpt_cr3_okay:
+
handle_exception_saved:
GET_CURRENT(bx)
testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
@@ -475,6 +580,7 @@ handle_exception_saved:
leaq exception_table(%rip),%rdx
PERFC_INCR(exceptions, %rax, %rbx)
callq *(%rdx,%rax,8)
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
testb $3,UREGS_cs(%rsp)
jz restore_all_xen
leaq VCPU_trap_bounce(%rbx),%rdx
@@ -507,6 +613,7 @@ exception_with_ints_disabled:
rep; movsq # make room for ec/ev
1: movq UREGS_error_code(%rsp),%rax # ec/ev
movq %rax,UREGS_kernel_sizeof(%rsp)
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
jmp restore_all_xen # return to fixup code
/* No special register assumptions. */
@@ -585,6 +692,17 @@ ENTRY(double_fault)
movl $TRAP_double_fault,4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
SAVE_ALL STAC
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
+ test %rbx, %rbx
+ jz .Ldblf_cr3_okay
+ jns .Ldblf_cr3_load
+ neg %rbx
+.Ldblf_cr3_load:
+ write_cr3 rbx, rdi, rsi
+.Ldblf_cr3_okay:
+
movq %rsp,%rdi
call do_double_fault
BUG /* do_double_fault() shouldn't return. */
@@ -603,10 +721,28 @@ ENTRY(nmi)
movl $TRAP_nmi,4(%rsp)
handle_ist_exception:
SAVE_ALL CLAC
+
+ GET_STACK_END(14)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
+ mov %rcx, %r15
+ neg %rcx
+ jz .List_cr3_okay
+ jns .List_cr3_load
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ neg %rcx
+.List_cr3_load:
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+.List_cr3_okay:
+
CR4_PV32_RESTORE
testb $3,UREGS_cs(%rsp)
jz 1f
- /* Interrupted guest context. Copy the context to stack bottom. */
+ /*
+ * Interrupted guest context. Clear the restore value for xen_cr3
+ * and copy the context to stack bottom.
+ */
+ xor %r15, %r15
GET_CPUINFO_FIELD(guest_cpu_user_regs,di)
movq %rsp,%rsi
movl $UREGS_kernel_sizeof/8,%ecx
@@ -616,6 +752,7 @@ handle_ist_exception:
movzbl UREGS_entry_vector(%rsp),%eax
leaq exception_table(%rip),%rdx
callq *(%rdx,%rax,8)
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
cmpb $TRAP_nmi,UREGS_entry_vector(%rsp)
jne ret_from_intr
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -93,9 +93,30 @@ void ret_from_intr(void);
UNLIKELY_DONE(mp, tag); \
__UNLIKELY_END(tag)
+ .equ .Lrax, 0
+ .equ .Lrcx, 1
+ .equ .Lrdx, 2
+ .equ .Lrbx, 3
+ .equ .Lrsp, 4
+ .equ .Lrbp, 5
+ .equ .Lrsi, 6
+ .equ .Lrdi, 7
+ .equ .Lr8, 8
+ .equ .Lr9, 9
+ .equ .Lr10, 10
+ .equ .Lr11, 11
+ .equ .Lr12, 12
+ .equ .Lr13, 13
+ .equ .Lr14, 14
+ .equ .Lr15, 15
+
#define STACK_CPUINFO_FIELD(field) (1 - CPUINFO_sizeof + CPUINFO_##field)
#define GET_STACK_END(reg) \
+ .if .Lr##reg > 8; \
+ movq $STACK_SIZE-1, %r##reg; \
+ .else; \
movl $STACK_SIZE-1, %e##reg; \
+ .endif; \
orq %rsp, %r##reg
#define GET_CPUINFO_FIELD(field, reg) \
@@ -177,6 +198,15 @@ void ret_from_intr(void);
#define ASM_STAC ASM_AC(STAC)
#define ASM_CLAC ASM_AC(CLAC)
+.macro write_cr3 val:req, tmp1:req, tmp2:req
+ mov %cr4, %\tmp1
+ mov %\tmp1, %\tmp2
+ and $~X86_CR4_PGE, %\tmp1
+ mov %\tmp1, %cr4
+ mov %\val, %cr3
+ mov %\tmp2, %cr4
+.endm
+
#define CR4_PV32_RESTORE \
667: ASM_NOP5; \
.pushsection .altinstr_replacement, "ax"; \
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -41,6 +41,18 @@ struct cpu_info {
struct vcpu *current_vcpu;
unsigned long per_cpu_offset;
unsigned long cr4;
+ /*
+ * Of the two following fields the latter is being set to the CR3 value
+ * to be used on the given pCPU for loading whenever 64-bit PV guest
+ * context is being entered. The value never changes once set.
+ * The former is the value to restore when re-entering Xen, if any. IOW
+ * its value being zero means there's nothing to restore. However, its
+ * value can also be negative, indicating to the exit-to-Xen code that
+ * restoring is not necessary, but allowing any nested entry code paths
+ * to still know the value to put back into CR3.
+ */
+ unsigned long xen_cr3;
+ unsigned long pv_cr3;
/* get_stack_bottom() must be 16-byte aligned */
};
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -462,6 +462,7 @@ extern idt_entry_t idt_table[];
extern idt_entry_t *idt_tables[];
DECLARE_PER_CPU(struct tss_struct, init_tss);
+DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
extern void init_int80_direct_trap(struct vcpu *v);
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -24,8 +24,8 @@
/* These are architectural limits. Current CPUs support only 40-bit phys. */
#define PADDR_BITS 52
#define VADDR_BITS 48
-#define PADDR_MASK ((1UL << PADDR_BITS)-1)
-#define VADDR_MASK ((1UL << VADDR_BITS)-1)
+#define PADDR_MASK ((_AC(1,UL) << PADDR_BITS)-1)
+#define VADDR_MASK ((_AC(1,UL) << VADDR_BITS)-1)
#define VADDR_TOP_BIT (1UL << (VADDR_BITS - 1))
#define CANONICAL_MASK (~0UL & ~VADDR_MASK)
@@ -107,6 +107,7 @@ typedef l4_pgentry_t root_pgentry_t;
: (((_s) < ROOT_PAGETABLE_FIRST_XEN_SLOT) || \
((_s) > ROOT_PAGETABLE_LAST_XEN_SLOT)))
+#define root_table_offset l4_table_offset
#define root_get_pfn l4e_get_pfn
#define root_get_flags l4e_get_flags
#define root_get_intpte l4e_get_intpte
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-15 11:01 [PATCH v2 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
2018-01-15 11:06 ` [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
@ 2018-01-15 11:07 ` Jan Beulich
2018-01-15 18:26 ` Andrew Cooper
2018-01-16 12:12 ` George Dunlap
2018-01-16 15:16 ` [PATCH v3 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
2 siblings, 2 replies; 29+ messages in thread
From: Jan Beulich @ 2018-01-15 11:07 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
First of all we don't need it on AMD systems. Additionally allow its use
to be controlled by command line option. For best backportability, this
intentionally doesn't use alternative instruction patching to achieve
the intended effect - while we likely want it, this will be later
follow-up.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1849,6 +1849,15 @@ In the case that x2apic is in use, this
clustered mode. The default, given no hint from the **FADT**, is cluster
mode.
+### xpti
+> `= <boolean>`
+
+> Default: `false` on AMD hardware
+> Default: `true` everywhere else
+
+Override default selection of whether to isolate 64-bit PV guest page
+tables.
+
### xsave
> `= <boolean>`
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1511,8 +1511,10 @@ void paravirt_ctxt_switch_to(struct vcpu
{
unsigned long cr4;
- this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
- l4e_from_page(v->domain->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
+ if ( this_cpu(root_pgt) )
+ this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
+ l4e_from_page(v->domain->arch.perdomain_l3_pg,
+ __PAGE_HYPERVISOR_RW);
cr4 = pv_guest_cr4_to_real_cr4(v);
if ( unlikely(cr4 != read_cr4()) )
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3685,7 +3685,7 @@ long do_mmu_update(
rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
if ( !rc )
- sync_guest = true;
+ sync_guest = this_cpu(root_pgt);
break;
case PGT_writable_page:
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -328,7 +328,7 @@ void start_secondary(void *unused)
spin_debug_disable();
get_cpu_info()->xen_cr3 = 0;
- get_cpu_info()->pv_cr3 = __pa(this_cpu(root_pgt));
+ get_cpu_info()->pv_cr3 = this_cpu(root_pgt) ? __pa(this_cpu(root_pgt)) : 0;
load_system_tables();
@@ -734,14 +734,20 @@ static int clone_mapping(const void *ptr
return 0;
}
+static __read_mostly int8_t opt_xpti = -1;
+boolean_param("xpti", opt_xpti);
DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
static int setup_cpu_root_pgt(unsigned int cpu)
{
- root_pgentry_t *rpt = alloc_xen_pagetable();
+ root_pgentry_t *rpt;
unsigned int off;
int rc;
+ if ( !opt_xpti )
+ return 0;
+
+ rpt = alloc_xen_pagetable();
if ( !rpt )
return -ENOMEM;
@@ -992,10 +998,13 @@ void __init smp_prepare_cpus(unsigned in
stack_base[0] = stack_start;
+ if ( opt_xpti < 0 )
+ opt_xpti = boot_cpu_data.x86_vendor != X86_VENDOR_AMD;
rc = setup_cpu_root_pgt(0);
if ( rc )
panic("Error %d setting up PV root page table\n", rc);
- get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
+ if ( per_cpu(root_pgt, 0) )
+ get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
set_nr_sockets();
@@ -1067,6 +1076,7 @@ void __init smp_prepare_boot_cpu(void)
#endif
get_cpu_info()->xen_cr3 = 0;
+ get_cpu_info()->pv_cr3 = 0;
}
static void
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -46,6 +46,7 @@ restore_all_guest:
movabs $DIRECTMAP_VIRT_START, %rcx
mov %rdi, %rax
and %rsi, %rdi
+ jz .Lrag_keep_cr3
and %r9, %rsi
add %rcx, %rdi
add %rcx, %rsi
@@ -62,6 +63,7 @@ restore_all_guest:
rep movsq
mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
write_cr3 rax, rdi, rsi
+.Lrag_keep_cr3:
RESTORE_ALL
testw $TRAP_syscall,4(%rsp)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-15 11:06 ` [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
@ 2018-01-15 18:23 ` Andrew Cooper
2018-01-16 7:46 ` Jan Beulich
2018-01-16 9:33 ` Jan Beulich
0 siblings, 2 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-01-15 18:23 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 15/01/18 11:06, Jan Beulich wrote:
> This is a very simplistic change limiting the amount of memory a running
> 64-bit PV guest has mapped (and hence available for attacking): Only the
> mappings of stack, IDT, and TSS are being cloned from the direct map
> into per-CPU page tables. Guest controlled parts of the page tables are
> being copied into those per-CPU page tables upon entry into the guest.
> Cross-vCPU synchronization of top level page table entry changes is
> being effected by forcing other active vCPU-s of the guest into the
> hypervisor.
>
> The change to context_switch() isn't strictly necessary, but there's no
> reason to keep switching page tables once a PV guest is being scheduled
> out.
>
> There is certainly much room for improvement, especially of performance,
> here - first and foremost suppressing all the negative effects on AMD
> systems. But in the interest of backportability (including to really old
> hypervisors, which may not even have alternative patching) any such is
> being left out here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Use sensible numbers for the register classification constants also
> for the low 8 registers. Insert 'r' into their names to make their
> purpose more clear. Use "sub" instead of "add" when adding an
> immediate of 128. Defer sync IPI in do_mmu_update() until the end of
> the main loop. Use flush IPI there instead event check one. Make
> setup functions return a proper error code. Use better suited local
> variables in clone_mapping(). Add comment to new struct cpu_info
> fields.
> ---
> This also wants Andrew's "[PATCH RFC 11/44] x86/pt-shadow: Always set
> _PAGE_ACCESSED on L4e updates".
I've cleaned this patch up and committed it in preparation.
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bd61fe94bee0556bc2f64999a4a8315b93f90f21
> ---
> Backporting notes:
> - This needs f9eb74789a ("x86/entry: Remove support for partial
> cpu_user_regs frames") as a prereq, due to the uses of %r14 and %r15.
> But that's intended to be backported anyway (for Spectre/SP2).
> - The use of "root" instead of "l4" here is mainly to not make 5-level
> page table additions any harder. In backports "l4" should probably be
> preferred.
> ---
> Follow-up notes:
> - use alternatives patching for fully suppressing performance effects
> when disabled
> - check whether running globally with CR4.PGE clear helps overall
> performance
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1511,6 +1511,9 @@ void paravirt_ctxt_switch_to(struct vcpu
> {
> unsigned long cr4;
>
> + this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
> + l4e_from_page(v->domain->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
> +
> cr4 = pv_guest_cr4_to_real_cr4(v);
> if ( unlikely(cr4 != read_cr4()) )
> write_cr4(cr4);
> @@ -1682,6 +1685,8 @@ void context_switch(struct vcpu *prev, s
>
> ASSERT(local_irq_is_enabled());
>
> + get_cpu_info()->xen_cr3 = 0;
> +
> cpumask_copy(&dirty_mask, next->vcpu_dirty_cpumask);
> /* Allow at most one CPU at a time to be dirty. */
> ASSERT(cpumask_weight(&dirty_mask) <= 1);
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3520,6 +3520,7 @@ long do_mmu_update(
> struct vcpu *curr = current, *v = curr;
> struct domain *d = v->domain, *pt_owner = d, *pg_owner;
> mfn_t map_mfn = INVALID_MFN;
> + bool sync_guest = false;
> uint32_t xsm_needed = 0;
> uint32_t xsm_checked = 0;
> int rc = put_old_guest_table(curr);
> @@ -3683,6 +3684,8 @@ long do_mmu_update(
> break;
> rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> + if ( !rc )
> + sync_guest = true;
> break;
>
> case PGT_writable_page:
> @@ -3787,6 +3790,24 @@ long do_mmu_update(
> if ( va )
> unmap_domain_page(va);
>
> + if ( sync_guest )
> + {
> + /*
> + * Force other vCPU-s of the affected guest to pick up L4 entry
> + * changes (if any). Issue a flush IPI with empty operation mask to
> + * facilitate this (including ourselves waiting for the IPI to
> + * actually have arrived). Utilize the fact that FLUSH_VA_VALID is
> + * meaningless without FLUSH_CACHE, but will allow to pass the no-op
> + * check in flush_area_mask().
> + */
> + unsigned int cpu = smp_processor_id();
> + cpumask_t *mask = per_cpu(scratch_cpumask, cpu);
> +
> + cpumask_andnot(mask, pt_owner->domain_dirty_cpumask, cpumask_of(cpu));
> + if ( !cpumask_empty(mask) )
> + flush_area_mask(mask, ZERO_BLOCK_PTR, FLUSH_VA_VALID);
> + }
> +
> perfc_add(num_page_updates, i);
>
> out:
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -327,6 +327,9 @@ void start_secondary(void *unused)
> */
> spin_debug_disable();
>
> + get_cpu_info()->xen_cr3 = 0;
> + get_cpu_info()->pv_cr3 = __pa(this_cpu(root_pgt));
> +
> load_system_tables();
>
> /* Full exception support from here on in. */
> @@ -633,6 +636,187 @@ void cpu_exit_clear(unsigned int cpu)
> set_cpu_state(CPU_STATE_DEAD);
> }
>
> +static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
> +{
> + unsigned long linear = (unsigned long)ptr, pfn;
> + unsigned int flags;
> + l3_pgentry_t *pl3e = l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) +
> + l3_table_offset(linear);
> + l2_pgentry_t *pl2e;
> + l1_pgentry_t *pl1e;
> +
> + if ( linear < DIRECTMAP_VIRT_START )
> + return 0;
> +
> + flags = l3e_get_flags(*pl3e);
> + ASSERT(flags & _PAGE_PRESENT);
> + if ( flags & _PAGE_PSE )
> + {
> + pfn = (l3e_get_pfn(*pl3e) & ~((1UL << (2 * PAGETABLE_ORDER)) - 1)) |
> + (PFN_DOWN(linear) & ((1UL << (2 * PAGETABLE_ORDER)) - 1));
> + flags &= ~_PAGE_PSE;
> + }
> + else
> + {
> + pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(linear);
> + flags = l2e_get_flags(*pl2e);
> + ASSERT(flags & _PAGE_PRESENT);
> + if ( flags & _PAGE_PSE )
> + {
> + pfn = (l2e_get_pfn(*pl2e) & ~((1UL << PAGETABLE_ORDER) - 1)) |
> + (PFN_DOWN(linear) & ((1UL << PAGETABLE_ORDER) - 1));
> + flags &= ~_PAGE_PSE;
> + }
> + else
> + {
> + pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
> + flags = l1e_get_flags(*pl1e);
> + if ( !(flags & _PAGE_PRESENT) )
> + return 0;
> + pfn = l1e_get_pfn(*pl1e);
> + }
> + }
> +
> + if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) )
> + {
> + pl3e = alloc_xen_pagetable();
> + if ( !pl3e )
> + return -ENOMEM;
> + clear_page(pl3e);
> + l4e_write(&rpt[root_table_offset(linear)],
> + l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR));
> + }
> + else
> + pl3e = l4e_to_l3e(rpt[root_table_offset(linear)]);
> +
> + pl3e += l3_table_offset(linear);
> +
> + if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
> + {
> + pl2e = alloc_xen_pagetable();
> + if ( !pl2e )
> + return -ENOMEM;
> + clear_page(pl2e);
> + l3e_write(pl3e, l3e_from_paddr(__pa(pl2e), __PAGE_HYPERVISOR));
> + }
> + else
> + {
> + ASSERT(!(l3e_get_flags(*pl3e) & _PAGE_PSE));
> + pl2e = l3e_to_l2e(*pl3e);
> + }
> +
> + pl2e += l2_table_offset(linear);
> +
> + if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
> + {
> + pl1e = alloc_xen_pagetable();
> + if ( !pl1e )
> + return -ENOMEM;
> + clear_page(pl1e);
> + l2e_write(pl2e, l2e_from_paddr(__pa(pl1e), __PAGE_HYPERVISOR));
> + }
> + else
> + {
> + ASSERT(!(l2e_get_flags(*pl2e) & _PAGE_PSE));
> + pl1e = l2e_to_l1e(*pl2e);
> + }
> +
> + pl1e += l1_table_offset(linear);
> +
> + if ( l1e_get_flags(*pl1e) & _PAGE_PRESENT )
> + {
> + ASSERT(l1e_get_pfn(*pl1e) == pfn);
> + ASSERT(l1e_get_flags(*pl1e) == flags);
> + }
> + else
> + l1e_write(pl1e, l1e_from_pfn(pfn, flags));
> +
> + return 0;
> +}
> +
> +DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
> +
> +static int setup_cpu_root_pgt(unsigned int cpu)
> +{
> + root_pgentry_t *rpt = alloc_xen_pagetable();
> + unsigned int off;
> + int rc;
> +
> + if ( !rpt )
> + return -ENOMEM;
> +
> + clear_page(rpt);
> + per_cpu(root_pgt, cpu) = rpt;
> +
> + rpt[root_table_offset(RO_MPT_VIRT_START)] =
> + idle_pg_table[root_table_offset(RO_MPT_VIRT_START)];
> + /* SH_LINEAR_PT inserted together with guest mappings. */
> + /* PERDOMAIN inserted during context switch. */
> + rpt[root_table_offset(XEN_VIRT_START)] =
> + idle_pg_table[root_table_offset(XEN_VIRT_START)];
> +
> + /* Install direct map page table entries for stack, IDT, and TSS. */
> + for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
> + rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
> +
> + if ( !rc )
> + rc = clone_mapping(idt_tables[cpu], rpt);
> + if ( !rc )
> + rc = clone_mapping(&per_cpu(init_tss, cpu), rpt);
> +
> + return rc;
> +}
> +
> +static void cleanup_cpu_root_pgt(unsigned int cpu)
> +{
> + root_pgentry_t *rpt = per_cpu(root_pgt, cpu);
> + unsigned int r;
> +
> + if ( !rpt )
> + return;
> +
> + per_cpu(root_pgt, cpu) = NULL;
> +
> + for ( r = root_table_offset(DIRECTMAP_VIRT_START);
> + r < root_table_offset(HYPERVISOR_VIRT_END); ++r )
> + {
> + l3_pgentry_t *l3t;
> + unsigned int i3;
> +
> + if ( !(root_get_flags(rpt[r]) & _PAGE_PRESENT) )
> + continue;
> +
> + l3t = l4e_to_l3e(rpt[r]);
> +
> + for ( i3 = 0; i3 < L3_PAGETABLE_ENTRIES; ++i3 )
> + {
> + l2_pgentry_t *l2t;
> + unsigned int i2;
> +
> + if ( !(l3e_get_flags(l3t[i3]) & _PAGE_PRESENT) )
> + continue;
> +
> + ASSERT(!(l3e_get_flags(l3t[i3]) & _PAGE_PSE));
> + l2t = l3e_to_l2e(l3t[i3]);
> +
> + for ( i2 = 0; i2 < L2_PAGETABLE_ENTRIES; ++i2 )
> + {
> + if ( !(l2e_get_flags(l2t[i2]) & _PAGE_PRESENT) )
> + continue;
> +
> + ASSERT(!(l2e_get_flags(l2t[i2]) & _PAGE_PSE));
> + free_xen_pagetable(l2e_to_l1e(l2t[i2]));
> + }
> +
> + free_xen_pagetable(l2t);
> + }
> +
> + free_xen_pagetable(l3t);
> + }
> +
> + free_xen_pagetable(rpt);
> +}
> +
> static void cpu_smpboot_free(unsigned int cpu)
> {
> unsigned int order, socket = cpu_to_socket(cpu);
> @@ -671,6 +855,8 @@ static void cpu_smpboot_free(unsigned in
> free_domheap_page(mfn_to_page(mfn));
> }
>
> + cleanup_cpu_root_pgt(cpu);
> +
> order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
> free_xenheap_pages(per_cpu(gdt_table, cpu), order);
>
> @@ -727,6 +913,11 @@ static int cpu_smpboot_alloc(unsigned in
> set_ist(&idt_tables[cpu][TRAP_nmi], IST_NONE);
> set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
>
> + rc = setup_cpu_root_pgt(cpu);
> + if ( rc )
> + goto out;
> + rc = -ENOMEM;
> +
> for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
> i < nr_cpu_ids && i <= (cpu | (STUBS_PER_PAGE - 1)); ++i )
> if ( cpu_online(i) && cpu_to_node(i) == node )
> @@ -786,6 +977,8 @@ static struct notifier_block cpu_smpboot
>
> void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> + int rc;
> +
> register_cpu_notifier(&cpu_smpboot_nfb);
>
> mtrr_aps_sync_begin();
> @@ -799,6 +992,11 @@ void __init smp_prepare_cpus(unsigned in
>
> stack_base[0] = stack_start;
>
> + rc = setup_cpu_root_pgt(0);
> + if ( rc )
> + panic("Error %d setting up PV root page table\n", rc);
> + get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
> +
> set_nr_sockets();
>
> socket_cpumask = xzalloc_array(cpumask_t *, nr_sockets);
> @@ -867,6 +1065,8 @@ void __init smp_prepare_boot_cpu(void)
> #if NR_CPUS > 2 * BITS_PER_LONG
> per_cpu(scratch_cpumask, cpu) = &scratch_cpu0mask;
> #endif
> +
> + get_cpu_info()->xen_cr3 = 0;
> }
>
> static void
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -137,6 +137,8 @@ void __dummy__(void)
> OFFSET(CPUINFO_processor_id, struct cpu_info, processor_id);
> OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
> OFFSET(CPUINFO_cr4, struct cpu_info, cr4);
> + OFFSET(CPUINFO_xen_cr3, struct cpu_info, xen_cr3);
> + OFFSET(CPUINFO_pv_cr3, struct cpu_info, pv_cr3);
> DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
> BLANK();
>
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -199,6 +199,17 @@ ENTRY(cstar_enter)
> pushq $0
> movl $TRAP_syscall, 4(%rsp)
> SAVE_ALL
> +
> + GET_STACK_END(bx)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
> + neg %rcx
> +UNLIKELY_START(nz, cstar_cr3)
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> + neg %rcx
> + write_cr3 rcx, rdi, rsi
> + movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> +UNLIKELY_END(cstar_cr3)
These UNLIKELY()s aren't correct. It will depend on hardware and
command line setting as to whether we expect to update cr3.
Furthermore, they will complicate splitting the early entry code away
from the main .text section for a full isolation implementation.
For now, I'd drop them and have a simple jz .Lskip.
Also, can we collect these together into macros, rather than
opencoding? We seem to have 3 distinct variations.
> +
> GET_CURRENT(bx)
> movq VCPU_domain(%rbx),%rcx
> cmpb $0,DOMAIN_is_32bit_pv(%rcx)
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -37,6 +37,32 @@ ENTRY(switch_to_kernel)
> /* %rbx: struct vcpu, interrupts disabled */
> restore_all_guest:
> ASSERT_INTERRUPTS_DISABLED
> +
> + /* Copy guest mappings and switch to per-CPU root page table. */
> + mov %cr3, %r9
> + GET_STACK_END(dx)
> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rdi
> + movabs $PADDR_MASK & PAGE_MASK, %rsi
> + movabs $DIRECTMAP_VIRT_START, %rcx
> + mov %rdi, %rax
> + and %rsi, %rdi
> + and %r9, %rsi
> + add %rcx, %rdi
> + add %rcx, %rsi
> + mov $ROOT_PAGETABLE_FIRST_XEN_SLOT, %ecx
> + mov root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rsi), %r8
> + mov %r8, root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rdi)
> + rep movsq
> + mov $ROOT_PAGETABLE_ENTRIES - \
> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1, %ecx
> + sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rsi
> + sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi
> + rep movsq
> + mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
> + write_cr3 rax, rdi, rsi
> +
Can we possibly move this up into C? For this simplistic algorithm it
is ok in ASM, but if we want to do any optimisations to avoid the 4k
memcpy (generation count hidden somewhere in page_info?), ASM is going
quickly become unwieldy.
Another optimisation I found made a massive difference for the KAISER
series was to have an MRU cache of 4 pagetables, so in-guest syscalls
don't result in any copying as we pass in and out of Xen.
> RESTORE_ALL
> testw $TRAP_syscall,4(%rsp)
> jz iret_exit_to_guest
> @@ -71,6 +97,18 @@ iret_exit_to_guest:
> ALIGN
> /* No special register assumptions. */
> restore_all_xen:
> + /*
> + * Check whether we need to switch to the per-CPU page tables, in
> + * case we return to late PV exit code (from an NMI or #MC).
> + */
> + GET_STACK_END(ax)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rax), %rdx
> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rax), %rax
> + test %rdx, %rdx
> +UNLIKELY_START(g, exit_cr3)
cmp or ne ?
> + write_cr3 rax, rdi, rsi
> +UNLIKELY_END(exit_cr3)
> +
> RESTORE_ALL adj=8
> iretq
>
> @@ -100,7 +138,18 @@ ENTRY(lstar_enter)
> pushq $0
> movl $TRAP_syscall, 4(%rsp)
> SAVE_ALL
> - GET_CURRENT(bx)
> +
> + GET_STACK_END(bx)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
> + neg %rcx
> + jz .Llstar_cr3_okay
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> + neg %rcx
> + write_cr3 rcx, rdi, rsi
> + movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> +.Llstar_cr3_okay:
> +
> + __GET_CURRENT(bx)
> testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
> jz switch_to_kernel
>
> @@ -192,7 +241,18 @@ GLOBAL(sysenter_eflags_saved)
> pushq $0
> movl $TRAP_syscall, 4(%rsp)
> SAVE_ALL
> - GET_CURRENT(bx)
> +
> + GET_STACK_END(bx)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
> + neg %rcx
> + jz .Lsyse_cr3_okay
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> + neg %rcx
> + write_cr3 rcx, rdi, rsi
> + movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> +.Lsyse_cr3_okay:
> +
> + __GET_CURRENT(bx)
> cmpb $0,VCPU_sysenter_disables_events(%rbx)
> movq VCPU_sysenter_addr(%rbx),%rax
> setne %cl
> @@ -228,13 +288,23 @@ ENTRY(int80_direct_trap)
> movl $0x80, 4(%rsp)
> SAVE_ALL
>
> + GET_STACK_END(bx)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
> + neg %rcx
> +UNLIKELY_START(nz, int80_cr3)
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> + neg %rcx
> + write_cr3 rcx, rdi, rsi
> + movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
> +UNLIKELY_END(int80_cr3)
> +
> cmpb $0,untrusted_msi(%rip)
> UNLIKELY_START(ne, msi_check)
> movl $0x80,%edi
> call check_for_unexpected_msi
> UNLIKELY_END(msi_check)
>
> - GET_CURRENT(bx)
> + __GET_CURRENT(bx)
>
> /* Check that the callback is non-null. */
> leaq VCPU_int80_bounce(%rbx),%rdx
> @@ -391,9 +461,27 @@ ENTRY(dom_crash_sync_extable)
>
> ENTRY(common_interrupt)
> SAVE_ALL CLAC
> +
> + GET_STACK_END(14)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
> + mov %rcx, %r15
> + neg %rcx
> + jz .Lintr_cr3_okay
> + jns .Lintr_cr3_load
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> + neg %rcx
> +.Lintr_cr3_load:
> + write_cr3 rcx, rdi, rsi
> + xor %ecx, %ecx
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> + testb $3, UREGS_cs(%rsp)
> + cmovnz %rcx, %r15
> +.Lintr_cr3_okay:
> +
> CR4_PV32_RESTORE
> movq %rsp,%rdi
> callq do_IRQ
> + mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> jmp ret_from_intr
>
> /* No special register assumptions. */
> @@ -411,6 +499,23 @@ ENTRY(page_fault)
> /* No special register assumptions. */
> GLOBAL(handle_exception)
> SAVE_ALL CLAC
> +
> + GET_STACK_END(14)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
> + mov %rcx, %r15
> + neg %rcx
> + jz .Lxcpt_cr3_okay
> + jns .Lxcpt_cr3_load
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> + neg %rcx
> +.Lxcpt_cr3_load:
> + write_cr3 rcx, rdi, rsi
> + xor %ecx, %ecx
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> + testb $3, UREGS_cs(%rsp)
> + cmovnz %rcx, %r15
> +.Lxcpt_cr3_okay:
> +
> handle_exception_saved:
> GET_CURRENT(bx)
> testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
> @@ -475,6 +580,7 @@ handle_exception_saved:
> leaq exception_table(%rip),%rdx
> PERFC_INCR(exceptions, %rax, %rbx)
> callq *(%rdx,%rax,8)
> + mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> testb $3,UREGS_cs(%rsp)
> jz restore_all_xen
> leaq VCPU_trap_bounce(%rbx),%rdx
> @@ -507,6 +613,7 @@ exception_with_ints_disabled:
> rep; movsq # make room for ec/ev
> 1: movq UREGS_error_code(%rsp),%rax # ec/ev
> movq %rax,UREGS_kernel_sizeof(%rsp)
> + mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> jmp restore_all_xen # return to fixup code
>
> /* No special register assumptions. */
> @@ -585,6 +692,17 @@ ENTRY(double_fault)
> movl $TRAP_double_fault,4(%rsp)
> /* Set AC to reduce chance of further SMAP faults */
> SAVE_ALL STAC
> +
> + GET_STACK_END(bx)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
> + test %rbx, %rbx
> + jz .Ldblf_cr3_okay
> + jns .Ldblf_cr3_load
> + neg %rbx
> +.Ldblf_cr3_load:
> + write_cr3 rbx, rdi, rsi
> +.Ldblf_cr3_okay:
> +
It is moderately common for there to be cascade faults in #DF. This
would be better if it were the general IST switch.
~Andrew
> movq %rsp,%rdi
> call do_double_fault
> BUG /* do_double_fault() shouldn't return. */
> @@ -603,10 +721,28 @@ ENTRY(nmi)
> movl $TRAP_nmi,4(%rsp)
> handle_ist_exception:
> SAVE_ALL CLAC
> +
> + GET_STACK_END(14)
> + mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
> + mov %rcx, %r15
> + neg %rcx
> + jz .List_cr3_okay
> + jns .List_cr3_load
> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> + neg %rcx
> +.List_cr3_load:
> + write_cr3 rcx, rdi, rsi
> + movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> +.List_cr3_okay:
> +
> CR4_PV32_RESTORE
> testb $3,UREGS_cs(%rsp)
> jz 1f
> - /* Interrupted guest context. Copy the context to stack bottom. */
> + /*
> + * Interrupted guest context. Clear the restore value for xen_cr3
> + * and copy the context to stack bottom.
> + */
> + xor %r15, %r15
> GET_CPUINFO_FIELD(guest_cpu_user_regs,di)
> movq %rsp,%rsi
> movl $UREGS_kernel_sizeof/8,%ecx
> @@ -616,6 +752,7 @@ handle_ist_exception:
> movzbl UREGS_entry_vector(%rsp),%eax
> leaq exception_table(%rip),%rdx
> callq *(%rdx,%rax,8)
> + mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
> cmpb $TRAP_nmi,UREGS_entry_vector(%rsp)
> jne ret_from_intr
>
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -93,9 +93,30 @@ void ret_from_intr(void);
> UNLIKELY_DONE(mp, tag); \
> __UNLIKELY_END(tag)
>
> + .equ .Lrax, 0
> + .equ .Lrcx, 1
> + .equ .Lrdx, 2
> + .equ .Lrbx, 3
> + .equ .Lrsp, 4
> + .equ .Lrbp, 5
> + .equ .Lrsi, 6
> + .equ .Lrdi, 7
> + .equ .Lr8, 8
> + .equ .Lr9, 9
> + .equ .Lr10, 10
> + .equ .Lr11, 11
> + .equ .Lr12, 12
> + .equ .Lr13, 13
> + .equ .Lr14, 14
> + .equ .Lr15, 15
> +
> #define STACK_CPUINFO_FIELD(field) (1 - CPUINFO_sizeof + CPUINFO_##field)
> #define GET_STACK_END(reg) \
> + .if .Lr##reg > 8; \
> + movq $STACK_SIZE-1, %r##reg; \
> + .else; \
> movl $STACK_SIZE-1, %e##reg; \
> + .endif; \
> orq %rsp, %r##reg
>
> #define GET_CPUINFO_FIELD(field, reg) \
> @@ -177,6 +198,15 @@ void ret_from_intr(void);
> #define ASM_STAC ASM_AC(STAC)
> #define ASM_CLAC ASM_AC(CLAC)
>
> +.macro write_cr3 val:req, tmp1:req, tmp2:req
> + mov %cr4, %\tmp1
> + mov %\tmp1, %\tmp2
> + and $~X86_CR4_PGE, %\tmp1
> + mov %\tmp1, %cr4
> + mov %\val, %cr3
> + mov %\tmp2, %cr4
> +.endm
> +
> #define CR4_PV32_RESTORE \
> 667: ASM_NOP5; \
> .pushsection .altinstr_replacement, "ax"; \
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -41,6 +41,18 @@ struct cpu_info {
> struct vcpu *current_vcpu;
> unsigned long per_cpu_offset;
> unsigned long cr4;
> + /*
> + * Of the two following fields the latter is being set to the CR3 value
> + * to be used on the given pCPU for loading whenever 64-bit PV guest
> + * context is being entered. The value never changes once set.
> + * The former is the value to restore when re-entering Xen, if any. IOW
> + * its value being zero means there's nothing to restore. However, its
> + * value can also be negative, indicating to the exit-to-Xen code that
> + * restoring is not necessary, but allowing any nested entry code paths
> + * to still know the value to put back into CR3.
> + */
> + unsigned long xen_cr3;
> + unsigned long pv_cr3;
> /* get_stack_bottom() must be 16-byte aligned */
> };
>
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -462,6 +462,7 @@ extern idt_entry_t idt_table[];
> extern idt_entry_t *idt_tables[];
>
> DECLARE_PER_CPU(struct tss_struct, init_tss);
> +DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
>
> extern void init_int80_direct_trap(struct vcpu *v);
>
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -24,8 +24,8 @@
> /* These are architectural limits. Current CPUs support only 40-bit phys. */
> #define PADDR_BITS 52
> #define VADDR_BITS 48
> -#define PADDR_MASK ((1UL << PADDR_BITS)-1)
> -#define VADDR_MASK ((1UL << VADDR_BITS)-1)
> +#define PADDR_MASK ((_AC(1,UL) << PADDR_BITS)-1)
> +#define VADDR_MASK ((_AC(1,UL) << VADDR_BITS)-1)
>
> #define VADDR_TOP_BIT (1UL << (VADDR_BITS - 1))
> #define CANONICAL_MASK (~0UL & ~VADDR_MASK)
> @@ -107,6 +107,7 @@ typedef l4_pgentry_t root_pgentry_t;
> : (((_s) < ROOT_PAGETABLE_FIRST_XEN_SLOT) || \
> ((_s) > ROOT_PAGETABLE_LAST_XEN_SLOT)))
>
> +#define root_table_offset l4_table_offset
> #define root_get_pfn l4e_get_pfn
> #define root_get_flags l4e_get_flags
> #define root_get_intpte l4e_get_intpte
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-15 11:07 ` [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
@ 2018-01-15 18:26 ` Andrew Cooper
2018-01-16 8:12 ` Jan Beulich
2018-01-16 12:12 ` George Dunlap
1 sibling, 1 reply; 29+ messages in thread
From: Andrew Cooper @ 2018-01-15 18:26 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 15/01/18 11:07, Jan Beulich wrote:
> First of all we don't need it on AMD systems. Additionally allow its use
> to be controlled by command line option. For best backportability, this
> intentionally doesn't use alternative instruction patching to achieve
> the intended effect - while we likely want it, this will be later
> follow-up.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
>
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1849,6 +1849,15 @@ In the case that x2apic is in use, this
> clustered mode. The default, given no hint from the **FADT**, is cluster
> mode.
>
> +### xpti
> +> `= <boolean>`
> +
> +> Default: `false` on AMD hardware
> +> Default: `true` everywhere else
This is fine for now, but any further isolation is going to have to be
unconditional, or possibly compile-time choice, but maintaining that
going forwards is going to be truly horrible.
> +
> +Override default selection of whether to isolate 64-bit PV guest page
> +tables.
This wants a
** WARNING: Not yet a complete isolation implementation, but better than
nothing. **
or similar, just to avoid giving the people the impression that it is
complete. Perhaps also a similar warning in the patch 1 commit message?
> +
> ### xsave
> > `= <boolean>`
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1511,8 +1511,10 @@ void paravirt_ctxt_switch_to(struct vcpu
> {
> unsigned long cr4;
>
> - this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
> - l4e_from_page(v->domain->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
> + if ( this_cpu(root_pgt) )
> + this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
> + l4e_from_page(v->domain->arch.perdomain_l3_pg,
> + __PAGE_HYPERVISOR_RW);
This would be cleaner and have better generated code by pulling
this_cpu(root_pgt) into a local variable.
>
> cr4 = pv_guest_cr4_to_real_cr4(v);
> if ( unlikely(cr4 != read_cr4()) )
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3685,7 +3685,7 @@ long do_mmu_update(
> rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> if ( !rc )
> - sync_guest = true;
> + sync_guest = this_cpu(root_pgt);
This is quite deceptive, as it is actually sync_guest =
this_cpu(root_pgt) != NULL;
It would probably be cleaner to export opt_xpti and use that, rather
than imply things based on root_pgt. It would certainly be more
efficient code.
> break;
>
> case PGT_writable_page:
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -328,7 +328,7 @@ void start_secondary(void *unused)
> spin_debug_disable();
>
> get_cpu_info()->xen_cr3 = 0;
> - get_cpu_info()->pv_cr3 = __pa(this_cpu(root_pgt));
> + get_cpu_info()->pv_cr3 = this_cpu(root_pgt) ? __pa(this_cpu(root_pgt)) : 0;
>
> load_system_tables();
>
> @@ -734,14 +734,20 @@ static int clone_mapping(const void *ptr
> return 0;
> }
>
> +static __read_mostly int8_t opt_xpti = -1;
> +boolean_param("xpti", opt_xpti);
> DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
>
> static int setup_cpu_root_pgt(unsigned int cpu)
> {
> - root_pgentry_t *rpt = alloc_xen_pagetable();
> + root_pgentry_t *rpt;
> unsigned int off;
> int rc;
>
> + if ( !opt_xpti )
> + return 0;
> +
> + rpt = alloc_xen_pagetable();
> if ( !rpt )
> return -ENOMEM;
>
> @@ -992,10 +998,13 @@ void __init smp_prepare_cpus(unsigned in
>
> stack_base[0] = stack_start;
>
> + if ( opt_xpti < 0 )
> + opt_xpti = boot_cpu_data.x86_vendor != X86_VENDOR_AMD;
Newline?
> rc = setup_cpu_root_pgt(0);
> if ( rc )
> panic("Error %d setting up PV root page table\n", rc);
> - get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
> + if ( per_cpu(root_pgt, 0) )
> + get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
>
> set_nr_sockets();
>
> @@ -1067,6 +1076,7 @@ void __init smp_prepare_boot_cpu(void)
> #endif
>
> get_cpu_info()->xen_cr3 = 0;
> + get_cpu_info()->pv_cr3 = 0;
> }
>
> static void
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -46,6 +46,7 @@ restore_all_guest:
> movabs $DIRECTMAP_VIRT_START, %rcx
> mov %rdi, %rax
> and %rsi, %rdi
> + jz .Lrag_keep_cr3
What is rag? What about .L_skip_cr3_reload ?
~Andrew
> and %r9, %rsi
> add %rcx, %rdi
> add %rcx, %rsi
> @@ -62,6 +63,7 @@ restore_all_guest:
> rep movsq
> mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
> write_cr3 rax, rdi, rsi
> +.Lrag_keep_cr3:
>
> RESTORE_ALL
> testw $TRAP_syscall,4(%rsp)
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-15 18:23 ` Andrew Cooper
@ 2018-01-16 7:46 ` Jan Beulich
2018-01-16 11:51 ` Andrew Cooper
2018-01-16 9:33 ` Jan Beulich
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 7:46 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 15.01.18 at 19:23, <andrew.cooper3@citrix.com> wrote:
> On 15/01/18 11:06, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -199,6 +199,17 @@ ENTRY(cstar_enter)
>> pushq $0
>> movl $TRAP_syscall, 4(%rsp)
>> SAVE_ALL
>> +
>> + GET_STACK_END(bx)
>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
>> + neg %rcx
>> +UNLIKELY_START(nz, cstar_cr3)
>> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
>> + neg %rcx
>> + write_cr3 rcx, rdi, rsi
>> + movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
>> +UNLIKELY_END(cstar_cr3)
>
> These UNLIKELY()s aren't correct. It will depend on hardware and
> command line setting as to whether we expect to update cr3.
Why are they not correct? What 64-bit kernels do you know that
use the CSTAR entry method in PV mode? Afaik this was in
existence for a range of kernel versions only in our forward port.
The INT80 path is perhaps indeed more questionable in this regard.
> Furthermore, they will complicate splitting the early entry code away
> from the main .text section for a full isolation implementation.
>
> For now, I'd drop them and have a simple jz .Lskip.
I will replace them (on the entry paths; I think the one on the exit-
to-Xen path is valid) to please you and in the interest of forward
progress; maybe this will even slightly help backporting to really old
Xen versions, where the UNLIKELY* constructs don't exist yet.
> Also, can we collect these together into macros, rather than
> opencoding? We seem to have 3 distinct variations.
I had considered that (following the model you use in the SP2
series), but decided against it not the least because of the
dependent but placement-wise separated code additions to
restore original values. Plus again this might be a hindrance of
backporting to really old Xen (which then typically will also be
built on really old tool chains) - as you certainly recall old gas
had quite a few issues with macro handling.
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -37,6 +37,32 @@ ENTRY(switch_to_kernel)
>> /* %rbx: struct vcpu, interrupts disabled */
>> restore_all_guest:
>> ASSERT_INTERRUPTS_DISABLED
>> +
>> + /* Copy guest mappings and switch to per-CPU root page table. */
>> + mov %cr3, %r9
>> + GET_STACK_END(dx)
>> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rdi
>> + movabs $PADDR_MASK & PAGE_MASK, %rsi
>> + movabs $DIRECTMAP_VIRT_START, %rcx
>> + mov %rdi, %rax
>> + and %rsi, %rdi
>> + and %r9, %rsi
>> + add %rcx, %rdi
>> + add %rcx, %rsi
>> + mov $ROOT_PAGETABLE_FIRST_XEN_SLOT, %ecx
>> + mov root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rsi), %r8
>> + mov %r8, root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rdi)
>> + rep movsq
>> + mov $ROOT_PAGETABLE_ENTRIES - \
>> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1, %ecx
>> + sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
>> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rsi
>> + sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
>> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi
>> + rep movsq
>> + mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
>> + write_cr3 rax, rdi, rsi
>> +
>
> Can we possibly move this up into C? For this simplistic algorithm it
> is ok in ASM, but if we want to do any optimisations to avoid the 4k
> memcpy (generation count hidden somewhere in page_info?), ASM is going
> quickly become unwieldy.
I'd prefer to move it into C when it really becomes necessary. Also
you don't properly qualify "this" - for example, I'd rather not move
the write_cr3 invocation into C, yet the placement of your comment
suggests to do so.
> Another optimisation I found made a massive difference for the KAISER
> series was to have an MRU cache of 4 pagetables, so in-guest syscalls
> don't result in any copying as we pass in and out of Xen.
As said elsewhere - optimization can come later. Plus - is avoiding the
copying at _any_ time actually correct, considering possible racing
L4 entry updates on another vCPU?
>> @@ -71,6 +97,18 @@ iret_exit_to_guest:
>> ALIGN
>> /* No special register assumptions. */
>> restore_all_xen:
>> + /*
>> + * Check whether we need to switch to the per-CPU page tables, in
>> + * case we return to late PV exit code (from an NMI or #MC).
>> + */
>> + GET_STACK_END(ax)
>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rax), %rdx
>> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rax), %rax
>> + test %rdx, %rdx
>> +UNLIKELY_START(g, exit_cr3)
>
> cmp or ne ?
"ne" (or really "nz" when used with "test") is outright wrong - we
want to skip the restore when the value is zero _or_ negative.
What's wrong with "jg" and "test" in combination? There simply is
no "jnsz" (other than e.g. "jnbe"). "cmp" against zero could be
used here, but why would I use the larger instruction when "test"
does?
>> @@ -585,6 +692,17 @@ ENTRY(double_fault)
>> movl $TRAP_double_fault,4(%rsp)
>> /* Set AC to reduce chance of further SMAP faults */
>> SAVE_ALL STAC
>> +
>> + GET_STACK_END(bx)
>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
>> + test %rbx, %rbx
>> + jz .Ldblf_cr3_okay
>> + jns .Ldblf_cr3_load
>> + neg %rbx
>> +.Ldblf_cr3_load:
>> + write_cr3 rbx, rdi, rsi
>> +.Ldblf_cr3_okay:
>> +
>
> It is moderately common for there to be cascade faults in #DF. This
> would be better if it were the general IST switch.
Based on the issues I had with #DF occurring while debugging this,
I've decided to keep the code here as simple as possible without
being incorrect: There's no point looking at the incoming CR3.
There's no point in trying to avoid nested faults (including
subsequent #DF) restoring CR3. There's also no point in retaining
the value for later restoring here, as we never return. In fact, as
mentioned elsewhere, we should imo indeed consider unilaterally
switching to idle_pg_table[] here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-15 18:26 ` Andrew Cooper
@ 2018-01-16 8:12 ` Jan Beulich
2018-01-16 13:20 ` Andrew Cooper
0 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 8:12 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 15.01.18 at 19:26, <andrew.cooper3@citrix.com> wrote:
> On 15/01/18 11:07, Jan Beulich wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1849,6 +1849,15 @@ In the case that x2apic is in use, this
>> clustered mode. The default, given no hint from the **FADT**, is cluster
>> mode.
>>
>> +### xpti
>> +> `= <boolean>`
>> +
>> +> Default: `false` on AMD hardware
>> +> Default: `true` everywhere else
>
> This is fine for now, but any further isolation is going to have to be
> unconditional, or possibly compile-time choice, but maintaining that
> going forwards is going to be truly horrible.
>
>> +
>> +Override default selection of whether to isolate 64-bit PV guest page
>> +tables.
>
> This wants a
>
> ** WARNING: Not yet a complete isolation implementation, but better than
> nothing. **
>
> or similar, just to avoid giving the people the impression that it is
> complete. Perhaps also a similar warning in the patch 1 commit message?
I've added the warning text here as suggested, and a respective
remark to patch 1's description, albeit under the current guidelines
of when we would consider an information leak a security issue, I
think isolation is sufficiently complete: No parts of the direct map
not pertaining to the current guest are being exposed, which
implies that no parts of the Xen heap not pertaining to the current
guest are. Leaking control page contents of other guests as well
as leaking the bits and pieces of the Xen image is not an issue by
itself.
IOW I'm not convinced such a warning is - strictly from a "would
this need an XSA on its own" pov - entirely appropriate.
Furthermore the command line option really exists to _disable_
the isolation (it also being capable of enabling is more a side
effect, but perhaps a useful one in the niche of running Xen
itself virtualized with being given the impression of running on
AMD hardware, but actually running on Intel), at which point the
warning is completely irrelevant.
One thing we may want/need to consider (but which is orthogonal
to the changes here) is to overwrite the part of the hypervisor
stack which isn't already being overwritten during context switch.
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1511,8 +1511,10 @@ void paravirt_ctxt_switch_to(struct vcpu
>> {
>> unsigned long cr4;
>>
>> - this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
>> - l4e_from_page(v->domain->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
>> + if ( this_cpu(root_pgt) )
>> + this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
>> + l4e_from_page(v->domain->arch.perdomain_l3_pg,
>> + __PAGE_HYPERVISOR_RW);
>
> This would be cleaner and have better generated code by pulling
> this_cpu(root_pgt) into a local variable.
Done.
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -3685,7 +3685,7 @@ long do_mmu_update(
>> rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
>> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>> if ( !rc )
>> - sync_guest = true;
>> + sync_guest = this_cpu(root_pgt);
>
> This is quite deceptive, as it is actually sync_guest =
> this_cpu(root_pgt) != NULL;
We omit such "!= NULL" / "!= 0" elsewhere too.
> It would probably be cleaner to export opt_xpti and use that, rather
> than imply things based on root_pgt. It would certainly be more
> efficient code.
Again I had considered this and decided against: Other reasons to
have NULL in here may arise, so it felt better to key the decision
off of the actual fact that it depends on, rather than that one's
pre-condition.
>> @@ -992,10 +998,13 @@ void __init smp_prepare_cpus(unsigned in
>>
>> stack_base[0] = stack_start;
>>
>> + if ( opt_xpti < 0 )
>> + opt_xpti = boot_cpu_data.x86_vendor != X86_VENDOR_AMD;
>
> Newline?
I've added one, but imo it hides that the assignment is a pre-condition
for ...
>> rc = setup_cpu_root_pgt(0);
... this call (i.e. makes less obvious that re-ordering here is not an
option).
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -46,6 +46,7 @@ restore_all_guest:
>> movabs $DIRECTMAP_VIRT_START, %rcx
>> mov %rdi, %rax
>> and %rsi, %rdi
>> + jz .Lrag_keep_cr3
>
> What is rag? What about .L_skip_cr3_reload ?
Look at the function name: It's the acronym of "restore_all_guest".
I specifically wanted to avoid any unspecific names like the one
you suggest, and short of a better idea I've used this one (to
parallel infixes like "cstar" used elsewhere).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-15 18:23 ` Andrew Cooper
2018-01-16 7:46 ` Jan Beulich
@ 2018-01-16 9:33 ` Jan Beulich
2018-01-16 11:56 ` Andrew Cooper
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 9:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 15.01.18 at 19:23, <andrew.cooper3@citrix.com> wrote:
> On 15/01/18 11:06, Jan Beulich wrote:
>> This also wants Andrew's "[PATCH RFC 11/44] x86/pt-shadow: Always set
>> _PAGE_ACCESSED on L4e updates".
>
> I've cleaned this patch up and committed it in preparation.
>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bd61fe94bee0556bc2f6
> 4999a4a8315b93f90f21
Only now that I'm doing backports thereof I notice an oddity with
32-bit guest handling: Why would you set the accessed bit in that
case? The L4 is an internal thing there, and hence by us knowing
that we don't care, this is unnecessary (but of course also not
wrong). I'll do the 4.9 and older backports according to that
observation (making for slightly less of a code change).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 7:46 ` Jan Beulich
@ 2018-01-16 11:51 ` Andrew Cooper
2018-01-16 12:33 ` Jan Beulich
0 siblings, 1 reply; 29+ messages in thread
From: Andrew Cooper @ 2018-01-16 11:51 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 16/01/18 07:46, Jan Beulich wrote:
>>>> On 15.01.18 at 19:23, <andrew.cooper3@citrix.com> wrote:
>> On 15/01/18 11:06, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_64/compat/entry.S
>>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>>> @@ -199,6 +199,17 @@ ENTRY(cstar_enter)
>>> pushq $0
>>> movl $TRAP_syscall, 4(%rsp)
>>> SAVE_ALL
>>> +
>>> + GET_STACK_END(bx)
>>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
>>> + neg %rcx
>>> +UNLIKELY_START(nz, cstar_cr3)
>>> + mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
>>> + neg %rcx
>>> + write_cr3 rcx, rdi, rsi
>>> + movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
>>> +UNLIKELY_END(cstar_cr3)
>> These UNLIKELY()s aren't correct. It will depend on hardware and
>> command line setting as to whether we expect to update cr3.
> Why are they not correct? What 64-bit kernels do you know that
> use the CSTAR entry method in PV mode?
None, but I also don't see you excluding 32bit PV guests, which means we
do take the unlikely path all the time in general.
> Afaik this was in
> existence for a range of kernel versions only in our forward port.
> The INT80 path is perhaps indeed more questionable in this regard.
>
>> Furthermore, they will complicate splitting the early entry code away
>> from the main .text section for a full isolation implementation.
>>
>> For now, I'd drop them and have a simple jz .Lskip.
> I will replace them (on the entry paths; I think the one on the exit-
> to-Xen path is valid) to please you and in the interest of forward
> progress; maybe this will even slightly help backporting to really old
> Xen versions, where the UNLIKELY* constructs don't exist yet.
>
>> Also, can we collect these together into macros, rather than
>> opencoding? We seem to have 3 distinct variations.
> I had considered that (following the model you use in the SP2
> series), but decided against it not the least because of the
> dependent but placement-wise separated code additions to
> restore original values. Plus again this might be a hindrance of
> backporting to really old Xen (which then typically will also be
> built on really old tool chains) - as you certainly recall old gas
> had quite a few issues with macro handling.
There is nothing special in these macros though? I found the SP2-style
far easier to backport because they are a single slot-in line.
Anyway, I'm not overly fussed, but I have a
>
>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -37,6 +37,32 @@ ENTRY(switch_to_kernel)
>>> /* %rbx: struct vcpu, interrupts disabled */
>>> restore_all_guest:
>>> ASSERT_INTERRUPTS_DISABLED
>>> +
>>> + /* Copy guest mappings and switch to per-CPU root page table. */
>>> + mov %cr3, %r9
>>> + GET_STACK_END(dx)
>>> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rdi
>>> + movabs $PADDR_MASK & PAGE_MASK, %rsi
>>> + movabs $DIRECTMAP_VIRT_START, %rcx
>>> + mov %rdi, %rax
>>> + and %rsi, %rdi
>>> + and %r9, %rsi
>>> + add %rcx, %rdi
>>> + add %rcx, %rsi
>>> + mov $ROOT_PAGETABLE_FIRST_XEN_SLOT, %ecx
>>> + mov root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rsi), %r8
>>> + mov %r8, root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rdi)
>>> + rep movsq
>>> + mov $ROOT_PAGETABLE_ENTRIES - \
>>> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1, %ecx
>>> + sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
>>> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rsi
>>> + sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
>>> + ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi
>>> + rep movsq
>>> + mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
>>> + write_cr3 rax, rdi, rsi
>>> +
>> Can we possibly move this up into C? For this simplistic algorithm it
>> is ok in ASM, but if we want to do any optimisations to avoid the 4k
>> memcpy (generation count hidden somewhere in page_info?), ASM is going
>> quickly become unwieldy.
> I'd prefer to move it into C when it really becomes necessary. Also
> you don't properly qualify "this" - for example, I'd rather not move
> the write_cr3 invocation into C, yet the placement of your comment
> suggests to do so.
I meant the whole hunk, not write_cr3 itself.
>
>> Another optimisation I found made a massive difference for the KAISER
>> series was to have an MRU cache of 4 pagetables, so in-guest syscalls
>> don't result in any copying as we pass in and out of Xen.
> As said elsewhere - optimization can come later. Plus - is avoiding the
> copying at _any_ time actually correct, considering possible racing
> L4 entry updates on another vCPU?
later> indeed.
The common behaviour is that L4's don't really change after fork(), so
having a generation count on them will allow skipping of the memcpy() in
most cases.
You can't skip the check, but the amount of logic used to avoid a 4k
memcpy can be quite substantial and still be a performance win.
>
>>> @@ -71,6 +97,18 @@ iret_exit_to_guest:
>>> ALIGN
>>> /* No special register assumptions. */
>>> restore_all_xen:
>>> + /*
>>> + * Check whether we need to switch to the per-CPU page tables, in
>>> + * case we return to late PV exit code (from an NMI or #MC).
>>> + */
>>> + GET_STACK_END(ax)
>>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rax), %rdx
>>> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rax), %rax
>>> + test %rdx, %rdx
>>> +UNLIKELY_START(g, exit_cr3)
>> cmp or ne ?
> "ne" (or really "nz" when used with "test") is outright wrong - we
> want to skip the restore when the value is zero _or_ negative.
> What's wrong with "jg" and "test" in combination? There simply is
> no "jnsz" (other than e.g. "jnbe"). "cmp" against zero could be
> used here, but why would I use the larger instruction when "test"
> does?
greater or less than is not commonly related to the test instruction,
which is why this looks wrong to a reviewer.
A comment of /* Ideally jnsz, but jg will have to do */ would go a very
long way.
I've double checked the logic and I agree with your conclusions, but the
only reason this works is because test unconditionally zeroes the
overflow flag.
>
>>> @@ -585,6 +692,17 @@ ENTRY(double_fault)
>>> movl $TRAP_double_fault,4(%rsp)
>>> /* Set AC to reduce chance of further SMAP faults */
>>> SAVE_ALL STAC
>>> +
>>> + GET_STACK_END(bx)
>>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
>>> + test %rbx, %rbx
>>> + jz .Ldblf_cr3_okay
>>> + jns .Ldblf_cr3_load
>>> + neg %rbx
>>> +.Ldblf_cr3_load:
>>> + write_cr3 rbx, rdi, rsi
>>> +.Ldblf_cr3_okay:
>>> +
>> It is moderately common for there to be cascade faults in #DF. This
>> would be better if it were the general IST switch.
> Based on the issues I had with #DF occurring while debugging this,
> I've decided to keep the code here as simple as possible without
> being incorrect: There's no point looking at the incoming CR3.
> There's no point in trying to avoid nested faults (including
> subsequent #DF) restoring CR3. There's also no point in retaining
> the value for later restoring here, as we never return. In fact, as
> mentioned elsewhere, we should imo indeed consider unilaterally
> switching to idle_pg_table[] here.
Ok.
~Andrew
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 9:33 ` Jan Beulich
@ 2018-01-16 11:56 ` Andrew Cooper
2018-01-16 12:25 ` Jan Beulich
0 siblings, 1 reply; 29+ messages in thread
From: Andrew Cooper @ 2018-01-16 11:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 16/01/18 09:33, Jan Beulich wrote:
>>>> On 15.01.18 at 19:23, <andrew.cooper3@citrix.com> wrote:
>> On 15/01/18 11:06, Jan Beulich wrote:
>>> This also wants Andrew's "[PATCH RFC 11/44] x86/pt-shadow: Always set
>>> _PAGE_ACCESSED on L4e updates".
>> I've cleaned this patch up and committed it in preparation.
>>
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bd61fe94bee0556bc2f6
>> 4999a4a8315b93f90f21
> Only now that I'm doing backports thereof I notice an oddity with
> 32-bit guest handling: Why would you set the accessed bit in that
> case? The L4 is an internal thing there, and hence by us knowing
> that we don't care, this is unnecessary (but of course also not
> wrong). I'll do the 4.9 and older backports according to that
> observation (making for slightly less of a code change).
There are no paths leading to adjust_guest_l4e() for 32bit PV guests.
The only L4 handling which exists is constructing the monitor table,
which writes the PTEs directly.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-15 11:07 ` [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
2018-01-15 18:26 ` Andrew Cooper
@ 2018-01-16 12:12 ` George Dunlap
2018-01-16 12:21 ` Juergen Gross
2018-01-16 12:35 ` Jan Beulich
1 sibling, 2 replies; 29+ messages in thread
From: George Dunlap @ 2018-01-16 12:12 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper
On Mon, Jan 15, 2018 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
> First of all we don't need it on AMD systems. Additionally allow its use
> to be controlled by command line option. For best backportability, this
> intentionally doesn't use alternative instruction patching to achieve
> the intended effect - while we likely want it, this will be later
> follow-up.
Is it worth making it optional to apply to dom0? In most cases, if an
attacker can manage to get userspace on dom0, they should be able to
take over the whole system anyway; turning it off on dom0 to get
better performance seems like a policy decision that administrators
might reasonably make.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 12:12 ` George Dunlap
@ 2018-01-16 12:21 ` Juergen Gross
2018-01-16 12:39 ` George Dunlap
2018-01-16 12:35 ` Jan Beulich
1 sibling, 1 reply; 29+ messages in thread
From: Juergen Gross @ 2018-01-16 12:21 UTC (permalink / raw)
To: George Dunlap, Jan Beulich; +Cc: xen-devel, Andrew Cooper
On 16/01/18 13:12, George Dunlap wrote:
> On Mon, Jan 15, 2018 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> First of all we don't need it on AMD systems. Additionally allow its use
>> to be controlled by command line option. For best backportability, this
>> intentionally doesn't use alternative instruction patching to achieve
>> the intended effect - while we likely want it, this will be later
>> follow-up.
>
> Is it worth making it optional to apply to dom0? In most cases, if an
> attacker can manage to get userspace on dom0, they should be able to
> take over the whole system anyway; turning it off on dom0 to get
> better performance seems like a policy decision that administrators
> might reasonably make.
You are implying here that Linux is insecure: why does userspace access
allow you to take over the machine? I can see that being true for root
access, but not for any other unprivileged user account.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 11:56 ` Andrew Cooper
@ 2018-01-16 12:25 ` Jan Beulich
0 siblings, 0 replies; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 12:25 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 16.01.18 at 12:56, <andrew.cooper3@citrix.com> wrote:
> On 16/01/18 09:33, Jan Beulich wrote:
>>>>> On 15.01.18 at 19:23, <andrew.cooper3@citrix.com> wrote:
>>> On 15/01/18 11:06, Jan Beulich wrote:
>>>> This also wants Andrew's "[PATCH RFC 11/44] x86/pt-shadow: Always set
>>>> _PAGE_ACCESSED on L4e updates".
>>> I've cleaned this patch up and committed it in preparation.
>>>
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bd61fe94bee0556bc2f6
>
>>> 4999a4a8315b93f90f21
>> Only now that I'm doing backports thereof I notice an oddity with
>> 32-bit guest handling: Why would you set the accessed bit in that
>> case? The L4 is an internal thing there, and hence by us knowing
>> that we don't care, this is unnecessary (but of course also not
>> wrong). I'll do the 4.9 and older backports according to that
>> observation (making for slightly less of a code change).
>
> There are no paths leading to adjust_guest_l4e() for 32bit PV guests.
> The only L4 handling which exists is constructing the monitor table,
> which writes the PTEs directly.
Yeah, I did realize this after writing. The check could probably
go away altogether.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 11:51 ` Andrew Cooper
@ 2018-01-16 12:33 ` Jan Beulich
2018-01-16 13:26 ` Andrew Cooper
0 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 12:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 16.01.18 at 12:51, <andrew.cooper3@citrix.com> wrote:
> On 16/01/18 07:46, Jan Beulich wrote:
>>>>> On 15.01.18 at 19:23, <andrew.cooper3@citrix.com> wrote:
>>> Also, can we collect these together into macros, rather than
>>> opencoding? We seem to have 3 distinct variations.
>> I had considered that (following the model you use in the SP2
>> series), but decided against it not the least because of the
>> dependent but placement-wise separated code additions to
>> restore original values. Plus again this might be a hindrance of
>> backporting to really old Xen (which then typically will also be
>> built on really old tool chains) - as you certainly recall old gas
>> had quite a few issues with macro handling.
>
> There is nothing special in these macros though? I found the SP2-style
> far easier to backport because they are a single slot-in line.
I've just found the patch here needing a change in register use
in 4.7 - such would be a little harder with pre-cooked macros,
especially when they don't allow to specify all registers to be
used (including ones for temporaries).
> Anyway, I'm not overly fussed, but I have a
Unfinished sentence?
>>>> @@ -71,6 +97,18 @@ iret_exit_to_guest:
>>>> ALIGN
>>>> /* No special register assumptions. */
>>>> restore_all_xen:
>>>> + /*
>>>> + * Check whether we need to switch to the per-CPU page tables, in
>>>> + * case we return to late PV exit code (from an NMI or #MC).
>>>> + */
>>>> + GET_STACK_END(ax)
>>>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rax), %rdx
>>>> + mov STACK_CPUINFO_FIELD(pv_cr3)(%rax), %rax
>>>> + test %rdx, %rdx
>>>> +UNLIKELY_START(g, exit_cr3)
>>> cmp or ne ?
>> "ne" (or really "nz" when used with "test") is outright wrong - we
>> want to skip the restore when the value is zero _or_ negative.
>> What's wrong with "jg" and "test" in combination? There simply is
>> no "jnsz" (other than e.g. "jnbe"). "cmp" against zero could be
>> used here, but why would I use the larger instruction when "test"
>> does?
>
> greater or less than is not commonly related to the test instruction,
> which is why this looks wrong to a reviewer.
>
> A comment of /* Ideally jnsz, but jg will have to do */ would go a very
> long way.
Added.
> I've double checked the logic and I agree with your conclusions, but the
> only reason this works is because test unconditionally zeroes the
> overflow flag.
Because, well, there is no overflow possible for AND (and hence
TEST), OR, and XOR.
>>>> @@ -585,6 +692,17 @@ ENTRY(double_fault)
>>>> movl $TRAP_double_fault,4(%rsp)
>>>> /* Set AC to reduce chance of further SMAP faults */
>>>> SAVE_ALL STAC
>>>> +
>>>> + GET_STACK_END(bx)
>>>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
>>>> + test %rbx, %rbx
>>>> + jz .Ldblf_cr3_okay
>>>> + jns .Ldblf_cr3_load
>>>> + neg %rbx
>>>> +.Ldblf_cr3_load:
>>>> + write_cr3 rbx, rdi, rsi
>>>> +.Ldblf_cr3_okay:
>>>> +
>>> It is moderately common for there to be cascade faults in #DF. This
>>> would be better if it were the general IST switch.
>> Based on the issues I had with #DF occurring while debugging this,
>> I've decided to keep the code here as simple as possible without
>> being incorrect: There's no point looking at the incoming CR3.
>> There's no point in trying to avoid nested faults (including
>> subsequent #DF) restoring CR3. There's also no point in retaining
>> the value for later restoring here, as we never return. In fact, as
>> mentioned elsewhere, we should imo indeed consider unilaterally
>> switching to idle_pg_table[] here.
>
> Ok.
"Ok" to which parts of this - keeping the code simple, switching to
idle_pg_table[] (perhaps in a follow-up patch), or both?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 12:12 ` George Dunlap
2018-01-16 12:21 ` Juergen Gross
@ 2018-01-16 12:35 ` Jan Beulich
2018-01-16 12:40 ` George Dunlap
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 12:35 UTC (permalink / raw)
To: George Dunlap; +Cc: Andrew Cooper, xen-devel
>>> On 16.01.18 at 13:12, <dunlapg@umich.edu> wrote:
> On Mon, Jan 15, 2018 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> First of all we don't need it on AMD systems. Additionally allow its use
>> to be controlled by command line option. For best backportability, this
>> intentionally doesn't use alternative instruction patching to achieve
>> the intended effect - while we likely want it, this will be later
>> follow-up.
>
> Is it worth making it optional to apply to dom0? In most cases, if an
> attacker can manage to get userspace on dom0, they should be able to
> take over the whole system anyway; turning it off on dom0 to get
> better performance seems like a policy decision that administrators
> might reasonably make.
Irrespective of Jürgen's reply (which I agree with) this would be an
option, but I'd prefer to fold this into the stage 2 activities (if we
really want it in the first place).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 12:21 ` Juergen Gross
@ 2018-01-16 12:39 ` George Dunlap
0 siblings, 0 replies; 29+ messages in thread
From: George Dunlap @ 2018-01-16 12:39 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, Jan Beulich, Andrew Cooper
On Tue, Jan 16, 2018 at 12:21 PM, Juergen Gross <jgross@suse.com> wrote:
> On 16/01/18 13:12, George Dunlap wrote:
>> On Mon, Jan 15, 2018 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> First of all we don't need it on AMD systems. Additionally allow its use
>>> to be controlled by command line option. For best backportability, this
>>> intentionally doesn't use alternative instruction patching to achieve
>>> the intended effect - while we likely want it, this will be later
>>> follow-up.
>>
>> Is it worth making it optional to apply to dom0? In most cases, if an
>> attacker can manage to get userspace on dom0, they should be able to
>> take over the whole system anyway; turning it off on dom0 to get
>> better performance seems like a policy decision that administrators
>> might reasonably make.
>
> You are implying here that Linux is insecure: why does userspace access
> allow you to take over the machine? I can see that being true for root
> access, but not for any other unprivileged user account.
Well first of all, go look at my talk about local root exploits in
Linux -- usually there are a few found every month.
But let's ignore that out for a minute. Consider a "typical"
recommended dom0 setup:
- Dom0 on a separate network
- Nothing running on dom0 except Xen-related services, and toolstack
How would an attacker get userspace access on such a host at all?
- Attack a devicemodel running in dom0
- Attack a backend running in the kernel
- Attack xenstore
We don't yet have a deprivileged QEMu, so at the moment an attack on
any of these will already give you full control of the system.
Obviously this wouldn't be appropriate to all systems; but it could be
appropriate to a fair number of them.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 12:35 ` Jan Beulich
@ 2018-01-16 12:40 ` George Dunlap
0 siblings, 0 replies; 29+ messages in thread
From: George Dunlap @ 2018-01-16 12:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Tue, Jan 16, 2018 at 12:35 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.01.18 at 13:12, <dunlapg@umich.edu> wrote:
>> On Mon, Jan 15, 2018 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> First of all we don't need it on AMD systems. Additionally allow its use
>>> to be controlled by command line option. For best backportability, this
>>> intentionally doesn't use alternative instruction patching to achieve
>>> the intended effect - while we likely want it, this will be later
>>> follow-up.
>>
>> Is it worth making it optional to apply to dom0? In most cases, if an
>> attacker can manage to get userspace on dom0, they should be able to
>> take over the whole system anyway; turning it off on dom0 to get
>> better performance seems like a policy decision that administrators
>> might reasonably make.
>
> Irrespective of Jürgen's reply (which I agree with) this would be an
> option, but I'd prefer to fold this into the stage 2 activities (if we
> really want it in the first place).
That sounds reasonable.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 8:12 ` Jan Beulich
@ 2018-01-16 13:20 ` Andrew Cooper
2018-01-16 13:51 ` Jan Beulich
0 siblings, 1 reply; 29+ messages in thread
From: Andrew Cooper @ 2018-01-16 13:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 16/01/18 08:12, Jan Beulich wrote:
>>>> On 15.01.18 at 19:26, <andrew.cooper3@citrix.com> wrote:
>> On 15/01/18 11:07, Jan Beulich wrote:
>>> --- a/docs/misc/xen-command-line.markdown
>>> +++ b/docs/misc/xen-command-line.markdown
>>> @@ -1849,6 +1849,15 @@ In the case that x2apic is in use, this
>>> clustered mode. The default, given no hint from the **FADT**, is cluster
>>> mode.
>>>
>>> +### xpti
>>> +> `= <boolean>`
>>> +
>>> +> Default: `false` on AMD hardware
>>> +> Default: `true` everywhere else
>> This is fine for now, but any further isolation is going to have to be
>> unconditional, or possibly compile-time choice, but maintaining that
>> going forwards is going to be truly horrible.
>>
>>> +
>>> +Override default selection of whether to isolate 64-bit PV guest page
>>> +tables.
>> This wants a
>>
>> ** WARNING: Not yet a complete isolation implementation, but better than
>> nothing. **
>>
>> or similar, just to avoid giving the people the impression that it is
>> complete. Perhaps also a similar warning in the patch 1 commit message?
> I've added the warning text here as suggested, and a respective
> remark to patch 1's description, albeit under the current guidelines
> of when we would consider an information leak a security issue, I
> think isolation is sufficiently complete: No parts of the direct map
> not pertaining to the current guest are being exposed, which
> implies that no parts of the Xen heap not pertaining to the current
> guest are. Leaking control page contents of other guests as well
> as leaking the bits and pieces of the Xen image is not an issue by
> itself.
>
> IOW I'm not convinced such a warning is - strictly from a "would
> this need an XSA on its own" pov - entirely appropriate.
The isolation is definitely not complete. Amongst other things, remote
stacks are in view of an attacker, which is why my KAISER-prereq series
pushes for the fully isolated per-pcpu range.
We do not, under any circumstances, want to give the impression that
this patch makes them completely immune to leaks, but improvements on
this status quo shouldn't need an XSA.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 12:33 ` Jan Beulich
@ 2018-01-16 13:26 ` Andrew Cooper
0 siblings, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-01-16 13:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 16/01/18 12:33, Jan Beulich wrote:
>>
>>>>>> On 15.01.18 at 19:23, <andrew.cooper3@citrix.com> wrote:
>>>> can we collect these together into macros, rather than
>>>> opencoding? We seem to have 3 distinct variations.
>>> I had considered that (following the model you use in the SP2
>>> series), but decided against it not the least because of the
>>> dependent but placement-wise separated code additions to
>>> restore original values. Plus again this might be a hindrance of
>>> backporting to really old Xen (which then typically will also be
>>> built on really old tool chains) - as you certainly recall old gas
>>> had quite a few issues with macro handling.
>> There is nothing special in these macros though? I found the SP2-style
>> far easier to backport because they are a single slot-in line.
> I've just found the patch here needing a change in register use
> in 4.7 - such would be a little harder with pre-cooked macros,
> especially when they don't allow to specify all registers to be
> used (including ones for temporaries).
>
>> Anyway, I'm not overly fussed, but I have a
> Unfinished sentence?
Oops. "I have a feeling that the duplication will lead to subtle bugs
when we need to change things, and inevitably miss one."
>
>>>>> @@ -585,6 +692,17 @@ ENTRY(double_fault)
>>>>> movl $TRAP_double_fault,4(%rsp)
>>>>> /* Set AC to reduce chance of further SMAP faults */
>>>>> SAVE_ALL STAC
>>>>> +
>>>>> + GET_STACK_END(bx)
>>>>> + mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
>>>>> + test %rbx, %rbx
>>>>> + jz .Ldblf_cr3_okay
>>>>> + jns .Ldblf_cr3_load
>>>>> + neg %rbx
>>>>> +.Ldblf_cr3_load:
>>>>> + write_cr3 rbx, rdi, rsi
>>>>> +.Ldblf_cr3_okay:
>>>>> +
>>>> It is moderately common for there to be cascade faults in #DF. This
>>>> would be better if it were the general IST switch.
>>> Based on the issues I had with #DF occurring while debugging this,
>>> I've decided to keep the code here as simple as possible without
>>> being incorrect: There's no point looking at the incoming CR3.
>>> There's no point in trying to avoid nested faults (including
>>> subsequent #DF) restoring CR3. There's also no point in retaining
>>> the value for later restoring here, as we never return. In fact, as
>>> mentioned elsewhere, we should imo indeed consider unilaterally
>>> switching to idle_pg_table[] here.
>> Ok.
> "Ok" to which parts of this - keeping the code simple, switching to
> idle_pg_table[] (perhaps in a follow-up patch), or both?
Keeping the code as it is now, and adjusting in a follow-up. Thinking
about things, idle_pg_table[], while nice, is incompatible with a more
complete isolation strategy.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 13:20 ` Andrew Cooper
@ 2018-01-16 13:51 ` Jan Beulich
0 siblings, 0 replies; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 13:51 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 16.01.18 at 14:20, <andrew.cooper3@citrix.com> wrote:
> The isolation is definitely not complete. Amongst other things, remote
> stacks are in view of an attacker, which is why my KAISER-prereq series
> pushes for the fully isolated per-pcpu range.
How are remote stacks visible? The local page tables contain only
mappings of the local CPU's stack.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 0/2] x86: initial simplistic Meltdown mitigation
2018-01-15 11:01 [PATCH v2 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
2018-01-15 11:06 ` [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
2018-01-15 11:07 ` [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
@ 2018-01-16 15:16 ` Jan Beulich
2018-01-16 15:21 ` [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
2018-01-16 15:22 ` [PATCH v3 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
2 siblings, 2 replies; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 15:16 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
1: x86: Meltdown band-aid against malicious 64-bit PV guests
2: x86: allow Meltdown band-aid to be disabled
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Addressing review comments (see individual patches).
v2: Addressing review comments for patch 1 (see there) and new
patch 2.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 15:16 ` [PATCH v3 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
@ 2018-01-16 15:21 ` Jan Beulich
2018-01-16 16:05 ` Andrew Cooper
2018-01-16 17:28 ` Andy Smith
2018-01-16 15:22 ` [PATCH v3 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
1 sibling, 2 replies; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 15:21 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
This is a very simplistic change limiting the amount of memory a running
64-bit PV guest has mapped (and hence available for attacking): Only the
mappings of stack, IDT, and TSS are being cloned from the direct map
into per-CPU page tables. Guest controlled parts of the page tables are
being copied into those per-CPU page tables upon entry into the guest.
Cross-vCPU synchronization of top level page table entry changes is
being effected by forcing other active vCPU-s of the guest into the
hypervisor.
The change to context_switch() isn't strictly necessary, but there's no
reason to keep switching page tables once a PV guest is being scheduled
out.
This isn't providing full isolation yet, but it should be covering all
pieces of information exposure of which would otherwise require an XSA.
There is certainly much room for improvement, especially of performance,
here - first and foremost suppressing all the negative effects on AMD
systems. But in the interest of backportability (including to really old
hypervisors, which may not even have alternative patching) any such is
being left out here.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Add remark in description as to not doing full isolation here. Drop
use of UNLIKELY_*() from CSTAR and INT80 entry paths.
v2: Use sensible numbers for the register classification constants also
for the low 8 registers. Insert 'r' into their names to make their
purpose more clear. Use "sub" instead of "add" when adding an
immediate of 128. Defer sync IPI in do_mmu_update() until the end of
the main loop. Use flush IPI there instead event check one. Make
setup functions return a proper error code. Use better suited local
variables in clone_mapping(). Add comment to new struct cpu_info
fields.
---
Backporting notes:
- This needs f9eb74789a ("x86/entry: Remove support for partial
cpu_user_regs frames") as a prereq, due to the uses of %r14 and %r15.
But that's intended to be backported anyway (for Spectre/SP2).
- This further needs bd61fe94be ("x86/mm: Always set _PAGE_ACCESSED on
L4e updates") as a prereq.
- The use of "root" instead of "l4" here is mainly to not make 5-level
page table additions any harder. In backports "l4" should probably be
preferred.
---
Follow-up notes:
- use alternatives patching for fully suppressing performance effects
when disabled
- check whether running globally with CR4.PGE clear helps overall
performance
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1511,6 +1511,9 @@ void paravirt_ctxt_switch_to(struct vcpu
{
unsigned long cr4;
+ this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
+ l4e_from_page(v->domain->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
+
cr4 = pv_guest_cr4_to_real_cr4(v);
if ( unlikely(cr4 != read_cr4()) )
write_cr4(cr4);
@@ -1682,6 +1685,8 @@ void context_switch(struct vcpu *prev, s
ASSERT(local_irq_is_enabled());
+ get_cpu_info()->xen_cr3 = 0;
+
cpumask_copy(&dirty_mask, next->vcpu_dirty_cpumask);
/* Allow at most one CPU at a time to be dirty. */
ASSERT(cpumask_weight(&dirty_mask) <= 1);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3520,6 +3520,7 @@ long do_mmu_update(
struct vcpu *curr = current, *v = curr;
struct domain *d = v->domain, *pt_owner = d, *pg_owner;
mfn_t map_mfn = INVALID_MFN;
+ bool sync_guest = false;
uint32_t xsm_needed = 0;
uint32_t xsm_checked = 0;
int rc = put_old_guest_table(curr);
@@ -3683,6 +3684,8 @@ long do_mmu_update(
break;
rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+ if ( !rc )
+ sync_guest = true;
break;
case PGT_writable_page:
@@ -3787,6 +3790,24 @@ long do_mmu_update(
if ( va )
unmap_domain_page(va);
+ if ( sync_guest )
+ {
+ /*
+ * Force other vCPU-s of the affected guest to pick up L4 entry
+ * changes (if any). Issue a flush IPI with empty operation mask to
+ * facilitate this (including ourselves waiting for the IPI to
+ * actually have arrived). Utilize the fact that FLUSH_VA_VALID is
+ * meaningless without FLUSH_CACHE, but will allow to pass the no-op
+ * check in flush_area_mask().
+ */
+ unsigned int cpu = smp_processor_id();
+ cpumask_t *mask = per_cpu(scratch_cpumask, cpu);
+
+ cpumask_andnot(mask, pt_owner->domain_dirty_cpumask, cpumask_of(cpu));
+ if ( !cpumask_empty(mask) )
+ flush_area_mask(mask, ZERO_BLOCK_PTR, FLUSH_VA_VALID);
+ }
+
perfc_add(num_page_updates, i);
out:
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -327,6 +327,9 @@ void start_secondary(void *unused)
*/
spin_debug_disable();
+ get_cpu_info()->xen_cr3 = 0;
+ get_cpu_info()->pv_cr3 = __pa(this_cpu(root_pgt));
+
load_system_tables();
/* Full exception support from here on in. */
@@ -633,6 +636,187 @@ void cpu_exit_clear(unsigned int cpu)
set_cpu_state(CPU_STATE_DEAD);
}
+static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
+{
+ unsigned long linear = (unsigned long)ptr, pfn;
+ unsigned int flags;
+ l3_pgentry_t *pl3e = l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) +
+ l3_table_offset(linear);
+ l2_pgentry_t *pl2e;
+ l1_pgentry_t *pl1e;
+
+ if ( linear < DIRECTMAP_VIRT_START )
+ return 0;
+
+ flags = l3e_get_flags(*pl3e);
+ ASSERT(flags & _PAGE_PRESENT);
+ if ( flags & _PAGE_PSE )
+ {
+ pfn = (l3e_get_pfn(*pl3e) & ~((1UL << (2 * PAGETABLE_ORDER)) - 1)) |
+ (PFN_DOWN(linear) & ((1UL << (2 * PAGETABLE_ORDER)) - 1));
+ flags &= ~_PAGE_PSE;
+ }
+ else
+ {
+ pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(linear);
+ flags = l2e_get_flags(*pl2e);
+ ASSERT(flags & _PAGE_PRESENT);
+ if ( flags & _PAGE_PSE )
+ {
+ pfn = (l2e_get_pfn(*pl2e) & ~((1UL << PAGETABLE_ORDER) - 1)) |
+ (PFN_DOWN(linear) & ((1UL << PAGETABLE_ORDER) - 1));
+ flags &= ~_PAGE_PSE;
+ }
+ else
+ {
+ pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
+ flags = l1e_get_flags(*pl1e);
+ if ( !(flags & _PAGE_PRESENT) )
+ return 0;
+ pfn = l1e_get_pfn(*pl1e);
+ }
+ }
+
+ if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) )
+ {
+ pl3e = alloc_xen_pagetable();
+ if ( !pl3e )
+ return -ENOMEM;
+ clear_page(pl3e);
+ l4e_write(&rpt[root_table_offset(linear)],
+ l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR));
+ }
+ else
+ pl3e = l4e_to_l3e(rpt[root_table_offset(linear)]);
+
+ pl3e += l3_table_offset(linear);
+
+ if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
+ {
+ pl2e = alloc_xen_pagetable();
+ if ( !pl2e )
+ return -ENOMEM;
+ clear_page(pl2e);
+ l3e_write(pl3e, l3e_from_paddr(__pa(pl2e), __PAGE_HYPERVISOR));
+ }
+ else
+ {
+ ASSERT(!(l3e_get_flags(*pl3e) & _PAGE_PSE));
+ pl2e = l3e_to_l2e(*pl3e);
+ }
+
+ pl2e += l2_table_offset(linear);
+
+ if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
+ {
+ pl1e = alloc_xen_pagetable();
+ if ( !pl1e )
+ return -ENOMEM;
+ clear_page(pl1e);
+ l2e_write(pl2e, l2e_from_paddr(__pa(pl1e), __PAGE_HYPERVISOR));
+ }
+ else
+ {
+ ASSERT(!(l2e_get_flags(*pl2e) & _PAGE_PSE));
+ pl1e = l2e_to_l1e(*pl2e);
+ }
+
+ pl1e += l1_table_offset(linear);
+
+ if ( l1e_get_flags(*pl1e) & _PAGE_PRESENT )
+ {
+ ASSERT(l1e_get_pfn(*pl1e) == pfn);
+ ASSERT(l1e_get_flags(*pl1e) == flags);
+ }
+ else
+ l1e_write(pl1e, l1e_from_pfn(pfn, flags));
+
+ return 0;
+}
+
+DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
+
+static int setup_cpu_root_pgt(unsigned int cpu)
+{
+ root_pgentry_t *rpt = alloc_xen_pagetable();
+ unsigned int off;
+ int rc;
+
+ if ( !rpt )
+ return -ENOMEM;
+
+ clear_page(rpt);
+ per_cpu(root_pgt, cpu) = rpt;
+
+ rpt[root_table_offset(RO_MPT_VIRT_START)] =
+ idle_pg_table[root_table_offset(RO_MPT_VIRT_START)];
+ /* SH_LINEAR_PT inserted together with guest mappings. */
+ /* PERDOMAIN inserted during context switch. */
+ rpt[root_table_offset(XEN_VIRT_START)] =
+ idle_pg_table[root_table_offset(XEN_VIRT_START)];
+
+ /* Install direct map page table entries for stack, IDT, and TSS. */
+ for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
+ rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
+
+ if ( !rc )
+ rc = clone_mapping(idt_tables[cpu], rpt);
+ if ( !rc )
+ rc = clone_mapping(&per_cpu(init_tss, cpu), rpt);
+
+ return rc;
+}
+
+static void cleanup_cpu_root_pgt(unsigned int cpu)
+{
+ root_pgentry_t *rpt = per_cpu(root_pgt, cpu);
+ unsigned int r;
+
+ if ( !rpt )
+ return;
+
+ per_cpu(root_pgt, cpu) = NULL;
+
+ for ( r = root_table_offset(DIRECTMAP_VIRT_START);
+ r < root_table_offset(HYPERVISOR_VIRT_END); ++r )
+ {
+ l3_pgentry_t *l3t;
+ unsigned int i3;
+
+ if ( !(root_get_flags(rpt[r]) & _PAGE_PRESENT) )
+ continue;
+
+ l3t = l4e_to_l3e(rpt[r]);
+
+ for ( i3 = 0; i3 < L3_PAGETABLE_ENTRIES; ++i3 )
+ {
+ l2_pgentry_t *l2t;
+ unsigned int i2;
+
+ if ( !(l3e_get_flags(l3t[i3]) & _PAGE_PRESENT) )
+ continue;
+
+ ASSERT(!(l3e_get_flags(l3t[i3]) & _PAGE_PSE));
+ l2t = l3e_to_l2e(l3t[i3]);
+
+ for ( i2 = 0; i2 < L2_PAGETABLE_ENTRIES; ++i2 )
+ {
+ if ( !(l2e_get_flags(l2t[i2]) & _PAGE_PRESENT) )
+ continue;
+
+ ASSERT(!(l2e_get_flags(l2t[i2]) & _PAGE_PSE));
+ free_xen_pagetable(l2e_to_l1e(l2t[i2]));
+ }
+
+ free_xen_pagetable(l2t);
+ }
+
+ free_xen_pagetable(l3t);
+ }
+
+ free_xen_pagetable(rpt);
+}
+
static void cpu_smpboot_free(unsigned int cpu)
{
unsigned int order, socket = cpu_to_socket(cpu);
@@ -671,6 +855,8 @@ static void cpu_smpboot_free(unsigned in
free_domheap_page(mfn_to_page(mfn));
}
+ cleanup_cpu_root_pgt(cpu);
+
order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
free_xenheap_pages(per_cpu(gdt_table, cpu), order);
@@ -727,6 +913,11 @@ static int cpu_smpboot_alloc(unsigned in
set_ist(&idt_tables[cpu][TRAP_nmi], IST_NONE);
set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
+ rc = setup_cpu_root_pgt(cpu);
+ if ( rc )
+ goto out;
+ rc = -ENOMEM;
+
for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
i < nr_cpu_ids && i <= (cpu | (STUBS_PER_PAGE - 1)); ++i )
if ( cpu_online(i) && cpu_to_node(i) == node )
@@ -786,6 +977,8 @@ static struct notifier_block cpu_smpboot
void __init smp_prepare_cpus(unsigned int max_cpus)
{
+ int rc;
+
register_cpu_notifier(&cpu_smpboot_nfb);
mtrr_aps_sync_begin();
@@ -799,6 +992,11 @@ void __init smp_prepare_cpus(unsigned in
stack_base[0] = stack_start;
+ rc = setup_cpu_root_pgt(0);
+ if ( rc )
+ panic("Error %d setting up PV root page table\n", rc);
+ get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
+
set_nr_sockets();
socket_cpumask = xzalloc_array(cpumask_t *, nr_sockets);
@@ -867,6 +1065,8 @@ void __init smp_prepare_boot_cpu(void)
#if NR_CPUS > 2 * BITS_PER_LONG
per_cpu(scratch_cpumask, cpu) = &scratch_cpu0mask;
#endif
+
+ get_cpu_info()->xen_cr3 = 0;
}
static void
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -137,6 +137,8 @@ void __dummy__(void)
OFFSET(CPUINFO_processor_id, struct cpu_info, processor_id);
OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
OFFSET(CPUINFO_cr4, struct cpu_info, cr4);
+ OFFSET(CPUINFO_xen_cr3, struct cpu_info, xen_cr3);
+ OFFSET(CPUINFO_pv_cr3, struct cpu_info, pv_cr3);
DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
BLANK();
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -199,6 +199,17 @@ ENTRY(cstar_enter)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+ jz .Lcstar_cr3_okay
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+.Lcstar_cr3_okay:
+
GET_CURRENT(bx)
movq VCPU_domain(%rbx),%rcx
cmpb $0,DOMAIN_is_32bit_pv(%rcx)
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -37,6 +37,32 @@ ENTRY(switch_to_kernel)
/* %rbx: struct vcpu, interrupts disabled */
restore_all_guest:
ASSERT_INTERRUPTS_DISABLED
+
+ /* Copy guest mappings and switch to per-CPU root page table. */
+ mov %cr3, %r9
+ GET_STACK_END(dx)
+ mov STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rdi
+ movabs $PADDR_MASK & PAGE_MASK, %rsi
+ movabs $DIRECTMAP_VIRT_START, %rcx
+ mov %rdi, %rax
+ and %rsi, %rdi
+ and %r9, %rsi
+ add %rcx, %rdi
+ add %rcx, %rsi
+ mov $ROOT_PAGETABLE_FIRST_XEN_SLOT, %ecx
+ mov root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rsi), %r8
+ mov %r8, root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rdi)
+ rep movsq
+ mov $ROOT_PAGETABLE_ENTRIES - \
+ ROOT_PAGETABLE_LAST_XEN_SLOT - 1, %ecx
+ sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
+ ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rsi
+ sub $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
+ ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi
+ rep movsq
+ mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
+ write_cr3 rax, rdi, rsi
+
RESTORE_ALL
testw $TRAP_syscall,4(%rsp)
jz iret_exit_to_guest
@@ -71,6 +97,22 @@ iret_exit_to_guest:
ALIGN
/* No special register assumptions. */
restore_all_xen:
+ /*
+ * Check whether we need to switch to the per-CPU page tables, in
+ * case we return to late PV exit code (from an NMI or #MC).
+ */
+ GET_STACK_END(ax)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rax), %rdx
+ mov STACK_CPUINFO_FIELD(pv_cr3)(%rax), %rax
+ test %rdx, %rdx
+ /*
+ * Ideally the condition would be "nsz", but such doesn't exist,
+ * so "g" will have to do.
+ */
+UNLIKELY_START(g, exit_cr3)
+ write_cr3 rax, rdi, rsi
+UNLIKELY_END(exit_cr3)
+
RESTORE_ALL adj=8
iretq
@@ -100,7 +142,18 @@ ENTRY(lstar_enter)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
- GET_CURRENT(bx)
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+ jz .Llstar_cr3_okay
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+.Llstar_cr3_okay:
+
+ __GET_CURRENT(bx)
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jz switch_to_kernel
@@ -192,7 +245,18 @@ GLOBAL(sysenter_eflags_saved)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
- GET_CURRENT(bx)
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+ jz .Lsyse_cr3_okay
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+.Lsyse_cr3_okay:
+
+ __GET_CURRENT(bx)
cmpb $0,VCPU_sysenter_disables_events(%rbx)
movq VCPU_sysenter_addr(%rbx),%rax
setne %cl
@@ -228,13 +292,23 @@ ENTRY(int80_direct_trap)
movl $0x80, 4(%rsp)
SAVE_ALL
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
+ neg %rcx
+ jz .Lint80_cr3_okay
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+ neg %rcx
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
+.Lint80_cr3_okay:
+
cmpb $0,untrusted_msi(%rip)
UNLIKELY_START(ne, msi_check)
movl $0x80,%edi
call check_for_unexpected_msi
UNLIKELY_END(msi_check)
- GET_CURRENT(bx)
+ __GET_CURRENT(bx)
/* Check that the callback is non-null. */
leaq VCPU_int80_bounce(%rbx),%rdx
@@ -391,9 +465,27 @@ ENTRY(dom_crash_sync_extable)
ENTRY(common_interrupt)
SAVE_ALL CLAC
+
+ GET_STACK_END(14)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
+ mov %rcx, %r15
+ neg %rcx
+ jz .Lintr_cr3_okay
+ jns .Lintr_cr3_load
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ neg %rcx
+.Lintr_cr3_load:
+ write_cr3 rcx, rdi, rsi
+ xor %ecx, %ecx
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ testb $3, UREGS_cs(%rsp)
+ cmovnz %rcx, %r15
+.Lintr_cr3_okay:
+
CR4_PV32_RESTORE
movq %rsp,%rdi
callq do_IRQ
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
jmp ret_from_intr
/* No special register assumptions. */
@@ -411,6 +503,23 @@ ENTRY(page_fault)
/* No special register assumptions. */
GLOBAL(handle_exception)
SAVE_ALL CLAC
+
+ GET_STACK_END(14)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
+ mov %rcx, %r15
+ neg %rcx
+ jz .Lxcpt_cr3_okay
+ jns .Lxcpt_cr3_load
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ neg %rcx
+.Lxcpt_cr3_load:
+ write_cr3 rcx, rdi, rsi
+ xor %ecx, %ecx
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ testb $3, UREGS_cs(%rsp)
+ cmovnz %rcx, %r15
+.Lxcpt_cr3_okay:
+
handle_exception_saved:
GET_CURRENT(bx)
testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
@@ -475,6 +584,7 @@ handle_exception_saved:
leaq exception_table(%rip),%rdx
PERFC_INCR(exceptions, %rax, %rbx)
callq *(%rdx,%rax,8)
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
testb $3,UREGS_cs(%rsp)
jz restore_all_xen
leaq VCPU_trap_bounce(%rbx),%rdx
@@ -507,6 +617,7 @@ exception_with_ints_disabled:
rep; movsq # make room for ec/ev
1: movq UREGS_error_code(%rsp),%rax # ec/ev
movq %rax,UREGS_kernel_sizeof(%rsp)
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
jmp restore_all_xen # return to fixup code
/* No special register assumptions. */
@@ -585,6 +696,17 @@ ENTRY(double_fault)
movl $TRAP_double_fault,4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
SAVE_ALL STAC
+
+ GET_STACK_END(bx)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
+ test %rbx, %rbx
+ jz .Ldblf_cr3_okay
+ jns .Ldblf_cr3_load
+ neg %rbx
+.Ldblf_cr3_load:
+ write_cr3 rbx, rdi, rsi
+.Ldblf_cr3_okay:
+
movq %rsp,%rdi
call do_double_fault
BUG /* do_double_fault() shouldn't return. */
@@ -603,10 +725,28 @@ ENTRY(nmi)
movl $TRAP_nmi,4(%rsp)
handle_ist_exception:
SAVE_ALL CLAC
+
+ GET_STACK_END(14)
+ mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
+ mov %rcx, %r15
+ neg %rcx
+ jz .List_cr3_okay
+ jns .List_cr3_load
+ mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+ neg %rcx
+.List_cr3_load:
+ write_cr3 rcx, rdi, rsi
+ movq $0, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
+.List_cr3_okay:
+
CR4_PV32_RESTORE
testb $3,UREGS_cs(%rsp)
jz 1f
- /* Interrupted guest context. Copy the context to stack bottom. */
+ /*
+ * Interrupted guest context. Clear the restore value for xen_cr3
+ * and copy the context to stack bottom.
+ */
+ xor %r15, %r15
GET_CPUINFO_FIELD(guest_cpu_user_regs,di)
movq %rsp,%rsi
movl $UREGS_kernel_sizeof/8,%ecx
@@ -616,6 +756,7 @@ handle_ist_exception:
movzbl UREGS_entry_vector(%rsp),%eax
leaq exception_table(%rip),%rdx
callq *(%rdx,%rax,8)
+ mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
cmpb $TRAP_nmi,UREGS_entry_vector(%rsp)
jne ret_from_intr
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -93,9 +93,30 @@ void ret_from_intr(void);
UNLIKELY_DONE(mp, tag); \
__UNLIKELY_END(tag)
+ .equ .Lrax, 0
+ .equ .Lrcx, 1
+ .equ .Lrdx, 2
+ .equ .Lrbx, 3
+ .equ .Lrsp, 4
+ .equ .Lrbp, 5
+ .equ .Lrsi, 6
+ .equ .Lrdi, 7
+ .equ .Lr8, 8
+ .equ .Lr9, 9
+ .equ .Lr10, 10
+ .equ .Lr11, 11
+ .equ .Lr12, 12
+ .equ .Lr13, 13
+ .equ .Lr14, 14
+ .equ .Lr15, 15
+
#define STACK_CPUINFO_FIELD(field) (1 - CPUINFO_sizeof + CPUINFO_##field)
#define GET_STACK_END(reg) \
+ .if .Lr##reg > 8; \
+ movq $STACK_SIZE-1, %r##reg; \
+ .else; \
movl $STACK_SIZE-1, %e##reg; \
+ .endif; \
orq %rsp, %r##reg
#define GET_CPUINFO_FIELD(field, reg) \
@@ -177,6 +198,15 @@ void ret_from_intr(void);
#define ASM_STAC ASM_AC(STAC)
#define ASM_CLAC ASM_AC(CLAC)
+.macro write_cr3 val:req, tmp1:req, tmp2:req
+ mov %cr4, %\tmp1
+ mov %\tmp1, %\tmp2
+ and $~X86_CR4_PGE, %\tmp1
+ mov %\tmp1, %cr4
+ mov %\val, %cr3
+ mov %\tmp2, %cr4
+.endm
+
#define CR4_PV32_RESTORE \
667: ASM_NOP5; \
.pushsection .altinstr_replacement, "ax"; \
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -41,6 +41,18 @@ struct cpu_info {
struct vcpu *current_vcpu;
unsigned long per_cpu_offset;
unsigned long cr4;
+ /*
+ * Of the two following fields the latter is being set to the CR3 value
+ * to be used on the given pCPU for loading whenever 64-bit PV guest
+ * context is being entered. The value never changes once set.
+ * The former is the value to restore when re-entering Xen, if any. IOW
+ * its value being zero means there's nothing to restore. However, its
+ * value can also be negative, indicating to the exit-to-Xen code that
+ * restoring is not necessary, but allowing any nested entry code paths
+ * to still know the value to put back into CR3.
+ */
+ unsigned long xen_cr3;
+ unsigned long pv_cr3;
/* get_stack_bottom() must be 16-byte aligned */
};
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -462,6 +462,7 @@ extern idt_entry_t idt_table[];
extern idt_entry_t *idt_tables[];
DECLARE_PER_CPU(struct tss_struct, init_tss);
+DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
extern void init_int80_direct_trap(struct vcpu *v);
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -24,8 +24,8 @@
/* These are architectural limits. Current CPUs support only 40-bit phys. */
#define PADDR_BITS 52
#define VADDR_BITS 48
-#define PADDR_MASK ((1UL << PADDR_BITS)-1)
-#define VADDR_MASK ((1UL << VADDR_BITS)-1)
+#define PADDR_MASK ((_AC(1,UL) << PADDR_BITS)-1)
+#define VADDR_MASK ((_AC(1,UL) << VADDR_BITS)-1)
#define VADDR_TOP_BIT (1UL << (VADDR_BITS - 1))
#define CANONICAL_MASK (~0UL & ~VADDR_MASK)
@@ -107,6 +107,7 @@ typedef l4_pgentry_t root_pgentry_t;
: (((_s) < ROOT_PAGETABLE_FIRST_XEN_SLOT) || \
((_s) > ROOT_PAGETABLE_LAST_XEN_SLOT)))
+#define root_table_offset l4_table_offset
#define root_get_pfn l4e_get_pfn
#define root_get_flags l4e_get_flags
#define root_get_intpte l4e_get_intpte
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 15:16 ` [PATCH v3 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
2018-01-16 15:21 ` [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
@ 2018-01-16 15:22 ` Jan Beulich
2018-01-16 16:07 ` Andrew Cooper
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2018-01-16 15:22 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
First of all we don't need it on AMD systems. Additionally allow its use
to be controlled by command line option. For best backportability, this
intentionally doesn't use alternative instruction patching to achieve
the intended effect - while we likely want it, this will be later
follow-up.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Add warning about incomplete implementation to command line option
doc. Use local variable in paravirt_ctxt_switch_to().
v2: New.
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1849,6 +1849,18 @@ In the case that x2apic is in use, this
clustered mode. The default, given no hint from the **FADT**, is cluster
mode.
+### xpti
+> `= <boolean>`
+
+> Default: `false` on AMD hardware
+> Default: `true` everywhere else
+
+Override default selection of whether to isolate 64-bit PV guest page
+tables.
+
+** WARNING: Not yet a complete isolation implementation, but better than
+nothing. **
+
### xsave
> `= <boolean>`
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1509,10 +1509,13 @@ void paravirt_ctxt_switch_from(struct vc
void paravirt_ctxt_switch_to(struct vcpu *v)
{
+ root_pgentry_t *root_pgt = this_cpu(root_pgt);
unsigned long cr4;
- this_cpu(root_pgt)[root_table_offset(PERDOMAIN_VIRT_START)] =
- l4e_from_page(v->domain->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
+ if ( root_pgt )
+ root_pgt[root_table_offset(PERDOMAIN_VIRT_START)] =
+ l4e_from_page(v->domain->arch.perdomain_l3_pg,
+ __PAGE_HYPERVISOR_RW);
cr4 = pv_guest_cr4_to_real_cr4(v);
if ( unlikely(cr4 != read_cr4()) )
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3685,7 +3685,7 @@ long do_mmu_update(
rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
if ( !rc )
- sync_guest = true;
+ sync_guest = this_cpu(root_pgt);
break;
case PGT_writable_page:
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -328,7 +328,7 @@ void start_secondary(void *unused)
spin_debug_disable();
get_cpu_info()->xen_cr3 = 0;
- get_cpu_info()->pv_cr3 = __pa(this_cpu(root_pgt));
+ get_cpu_info()->pv_cr3 = this_cpu(root_pgt) ? __pa(this_cpu(root_pgt)) : 0;
load_system_tables();
@@ -734,14 +734,20 @@ static int clone_mapping(const void *ptr
return 0;
}
+static __read_mostly int8_t opt_xpti = -1;
+boolean_param("xpti", opt_xpti);
DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
static int setup_cpu_root_pgt(unsigned int cpu)
{
- root_pgentry_t *rpt = alloc_xen_pagetable();
+ root_pgentry_t *rpt;
unsigned int off;
int rc;
+ if ( !opt_xpti )
+ return 0;
+
+ rpt = alloc_xen_pagetable();
if ( !rpt )
return -ENOMEM;
@@ -992,10 +998,14 @@ void __init smp_prepare_cpus(unsigned in
stack_base[0] = stack_start;
+ if ( opt_xpti < 0 )
+ opt_xpti = boot_cpu_data.x86_vendor != X86_VENDOR_AMD;
+
rc = setup_cpu_root_pgt(0);
if ( rc )
panic("Error %d setting up PV root page table\n", rc);
- get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
+ if ( per_cpu(root_pgt, 0) )
+ get_cpu_info()->pv_cr3 = __pa(per_cpu(root_pgt, 0));
set_nr_sockets();
@@ -1067,6 +1077,7 @@ void __init smp_prepare_boot_cpu(void)
#endif
get_cpu_info()->xen_cr3 = 0;
+ get_cpu_info()->pv_cr3 = 0;
}
static void
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -46,6 +46,7 @@ restore_all_guest:
movabs $DIRECTMAP_VIRT_START, %rcx
mov %rdi, %rax
and %rsi, %rdi
+ jz .Lrag_keep_cr3
and %r9, %rsi
add %rcx, %rdi
add %rcx, %rsi
@@ -62,6 +63,7 @@ restore_all_guest:
rep movsq
mov %r9, STACK_CPUINFO_FIELD(xen_cr3)(%rdx)
write_cr3 rax, rdi, rsi
+.Lrag_keep_cr3:
RESTORE_ALL
testw $TRAP_syscall,4(%rsp)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 15:21 ` [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
@ 2018-01-16 16:05 ` Andrew Cooper
2018-01-16 17:28 ` Andy Smith
1 sibling, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-01-16 16:05 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 16/01/18 15:21, Jan Beulich wrote:
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -24,8 +24,8 @@
> /* These are architectural limits. Current CPUs support only 40-bit phys. */
> #define PADDR_BITS 52
> #define VADDR_BITS 48
> -#define PADDR_MASK ((1UL << PADDR_BITS)-1)
> -#define VADDR_MASK ((1UL << VADDR_BITS)-1)
> +#define PADDR_MASK ((_AC(1,UL) << PADDR_BITS)-1)
> +#define VADDR_MASK ((_AC(1,UL) << VADDR_BITS)-1)
Sorry to only notice these now, but spaces as you're adjusting these.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> #define VADDR_TOP_BIT (1UL << (VADDR_BITS - 1))
> #define CANONICAL_MASK (~0UL & ~VADDR_MASK)
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] x86: allow Meltdown band-aid to be disabled
2018-01-16 15:22 ` [PATCH v3 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
@ 2018-01-16 16:07 ` Andrew Cooper
0 siblings, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-01-16 16:07 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 16/01/18 15:22, Jan Beulich wrote:
> First of all we don't need it on AMD systems. Additionally allow its use
> to be controlled by command line option. For best backportability, this
> intentionally doesn't use alternative instruction patching to achieve
> the intended effect - while we likely want it, this will be later
> follow-up.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 15:21 ` [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
2018-01-16 16:05 ` Andrew Cooper
@ 2018-01-16 17:28 ` Andy Smith
2018-01-16 18:02 ` George Dunlap
` (2 more replies)
1 sibling, 3 replies; 29+ messages in thread
From: Andy Smith @ 2018-01-16 17:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
Hi Jan,
On Tue, Jan 16, 2018 at 08:21:52AM -0700, Jan Beulich wrote:
> This is a very simplistic change limiting the amount of memory a running
> 64-bit PV guest has mapped (and hence available for attacking): Only the
> mappings of stack, IDT, and TSS are being cloned from the direct map
> into per-CPU page tables.
Can this be used with Comet/Vixen to further protect PV guests? i.e.
if the shim hypervisor has these changes then will it also limit
what a process in the PV guest can see in that shim hypervisor,
which therefore protects its own guest kernel a bit too?
Thanks,
Andy
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 17:28 ` Andy Smith
@ 2018-01-16 18:02 ` George Dunlap
2018-01-16 18:13 ` Andrew Cooper
2018-01-16 19:02 ` Wei Liu
2 siblings, 0 replies; 29+ messages in thread
From: George Dunlap @ 2018-01-16 18:02 UTC (permalink / raw)
To: Andy Smith; +Cc: xen-devel, Jan Beulich
On Tue, Jan 16, 2018 at 5:28 PM, Andy Smith <andy@strugglers.net> wrote:
> Hi Jan,
>
> On Tue, Jan 16, 2018 at 08:21:52AM -0700, Jan Beulich wrote:
>> This is a very simplistic change limiting the amount of memory a running
>> 64-bit PV guest has mapped (and hence available for attacking): Only the
>> mappings of stack, IDT, and TSS are being cloned from the direct map
>> into per-CPU page tables.
>
> Can this be used with Comet/Vixen to further protect PV guests? i.e.
> if the shim hypervisor has these changes then will it also limit
> what a process in the PV guest can see in that shim hypervisor,
> which therefore protects its own guest kernel a bit too?
Technically, yes, it should.
However,
1) It should be unnecessary. If you're running PV with the
"bandaid", you should be reasonably safe without using the shim
2) The shim adds nearly 40% overhead in my words-case tests; and so
does the bandaid. Together I think your performance would be pretty
terrible.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 17:28 ` Andy Smith
2018-01-16 18:02 ` George Dunlap
@ 2018-01-16 18:13 ` Andrew Cooper
2018-01-16 19:02 ` Wei Liu
2 siblings, 0 replies; 29+ messages in thread
From: Andrew Cooper @ 2018-01-16 18:13 UTC (permalink / raw)
To: Andy Smith, Jan Beulich; +Cc: xen-devel
On 16/01/18 17:28, Andy Smith wrote:
> Hi Jan,
>
> On Tue, Jan 16, 2018 at 08:21:52AM -0700, Jan Beulich wrote:
>> This is a very simplistic change limiting the amount of memory a running
>> 64-bit PV guest has mapped (and hence available for attacking): Only the
>> mappings of stack, IDT, and TSS are being cloned from the direct map
>> into per-CPU page tables.
> Can this be used with Comet/Vixen to further protect PV guests? i.e.
> if the shim hypervisor has these changes then will it also limit
> what a process in the PV guest can see in that shim hypervisor,
> which therefore protects its own guest kernel a bit too?
Yes.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests
2018-01-16 17:28 ` Andy Smith
2018-01-16 18:02 ` George Dunlap
2018-01-16 18:13 ` Andrew Cooper
@ 2018-01-16 19:02 ` Wei Liu
2 siblings, 0 replies; 29+ messages in thread
From: Wei Liu @ 2018-01-16 19:02 UTC (permalink / raw)
To: Andy Smith; +Cc: xen-devel, Wei Liu, Jan Beulich
On Tue, Jan 16, 2018 at 05:28:40PM +0000, Andy Smith wrote:
> Hi Jan,
>
> On Tue, Jan 16, 2018 at 08:21:52AM -0700, Jan Beulich wrote:
> > This is a very simplistic change limiting the amount of memory a running
> > 64-bit PV guest has mapped (and hence available for attacking): Only the
> > mappings of stack, IDT, and TSS are being cloned from the direct map
> > into per-CPU page tables.
>
> Can this be used with Comet/Vixen to further protect PV guests? i.e.
> if the shim hypervisor has these changes then will it also limit
> what a process in the PV guest can see in that shim hypervisor,
> which therefore protects its own guest kernel a bit too?
>
Yes, but please be warned that the guest is very very slow. I don't
think XPTI + shim is very usable at this stage.
If you're interested in trying that out, check out staging branch and
build a shim from there.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-01-16 19:03 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-15 11:01 [PATCH v2 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
2018-01-15 11:06 ` [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
2018-01-15 18:23 ` Andrew Cooper
2018-01-16 7:46 ` Jan Beulich
2018-01-16 11:51 ` Andrew Cooper
2018-01-16 12:33 ` Jan Beulich
2018-01-16 13:26 ` Andrew Cooper
2018-01-16 9:33 ` Jan Beulich
2018-01-16 11:56 ` Andrew Cooper
2018-01-16 12:25 ` Jan Beulich
2018-01-15 11:07 ` [PATCH v2 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
2018-01-15 18:26 ` Andrew Cooper
2018-01-16 8:12 ` Jan Beulich
2018-01-16 13:20 ` Andrew Cooper
2018-01-16 13:51 ` Jan Beulich
2018-01-16 12:12 ` George Dunlap
2018-01-16 12:21 ` Juergen Gross
2018-01-16 12:39 ` George Dunlap
2018-01-16 12:35 ` Jan Beulich
2018-01-16 12:40 ` George Dunlap
2018-01-16 15:16 ` [PATCH v3 0/2] x86: initial simplistic Meltdown mitigation Jan Beulich
2018-01-16 15:21 ` [PATCH v3 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests Jan Beulich
2018-01-16 16:05 ` Andrew Cooper
2018-01-16 17:28 ` Andy Smith
2018-01-16 18:02 ` George Dunlap
2018-01-16 18:13 ` Andrew Cooper
2018-01-16 19:02 ` Wei Liu
2018-01-16 15:22 ` [PATCH v3 2/2] x86: allow Meltdown band-aid to be disabled Jan Beulich
2018-01-16 16:07 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.