* [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
@ 2017-05-02 18:05 ` Andrew Cooper
2017-05-03 8:10 ` Jan Beulich
2017-05-03 9:48 ` Wei Liu
2017-05-02 18:05 ` [PATCH 2/7] x86/traps: Poison unused stack pointers in the TSS Andrew Cooper
` (6 subsequent siblings)
7 siblings, 2 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-02 18:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
The backlink field doesn't exist in a 64bit TSS, and union for esp{0..2} is of
no practical use. Specify everything with stdint types, and empty bitfields
for reserved values.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/traps.c | 2 +-
xen/include/asm-x86/processor.h | 23 +++++++++++------------
2 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index ca0a04a..1c90177 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -461,7 +461,7 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
printk("Valid stack range: %p-%p, sp=%p, tss.esp0=%p\n",
(void *)esp_top, (void *)esp_bottom, (void *)esp,
- (void *)per_cpu(init_tss, cpu).esp0);
+ (void *)per_cpu(init_tss, cpu).rsp0);
/*
* Trigger overflow trace if %esp is anywhere within the guard page, or
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 73dd3ab..f244e10 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -428,19 +428,18 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
#define IOBMP_INVALID_OFFSET 0x8000
struct __packed __cacheline_aligned tss_struct {
- unsigned short back_link,__blh;
- union { u64 rsp0, esp0; };
- union { u64 rsp1, esp1; };
- union { u64 rsp2, esp2; };
- u64 reserved1;
- u64 ist[7]; /* Interrupt Stack Table is 1-based so tss->ist[0]
- * corresponds to an IST value of 1 in an Interrupt
- * Descriptor */
- u64 reserved2;
- u16 reserved3;
- u16 bitmap;
+ uint32_t :32;
+ uint64_t rsp0, rsp1, rsp2;
+ uint64_t :64;
+ /*
+ * Interrupt Stack Table is 1-based so tss->ist[0] corresponds to an IST
+ * value of 1 in an Interrupt Descriptor.
+ */
+ uint64_t ist[7];
+ uint64_t :64;
+ uint16_t :16, bitmap;
/* Pads the TSS to be cacheline-aligned (total size is 0x80). */
- u8 __cacheline_filler[24];
+ uint8_t __cacheline_filler[24];
};
#define IST_NONE 0UL
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct
2017-05-02 18:05 ` [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct Andrew Cooper
@ 2017-05-03 8:10 ` Jan Beulich
2017-05-03 12:33 ` Andrew Cooper
2017-05-03 9:48 ` Wei Liu
1 sibling, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 8:10 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> The backlink field doesn't exist in a 64bit TSS, and union for esp{0..2} is of
> no practical use. Specify everything with stdint types, and empty bitfields
> for reserved values.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one minor extra adjustment:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -461,7 +461,7 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
>
> printk("Valid stack range: %p-%p, sp=%p, tss.esp0=%p\n",
Would you mind dropping or replacing the e in esp0 here too?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct
2017-05-03 8:10 ` Jan Beulich
@ 2017-05-03 12:33 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 12:33 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 03/05/17 09:10, Jan Beulich wrote:
>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> The backlink field doesn't exist in a 64bit TSS, and union for esp{0..2} is of
>> no practical use. Specify everything with stdint types, and empty bitfields
>> for reserved values.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one minor extra adjustment:
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -461,7 +461,7 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
>>
>> printk("Valid stack range: %p-%p, sp=%p, tss.esp0=%p\n",
> Would you mind dropping or replacing the e in esp0 here too?
Fixed.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct
2017-05-02 18:05 ` [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct Andrew Cooper
2017-05-03 8:10 ` Jan Beulich
@ 2017-05-03 9:48 ` Wei Liu
1 sibling, 0 replies; 48+ messages in thread
From: Wei Liu @ 2017-05-03 9:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel
On Tue, May 02, 2017 at 07:05:20PM +0100, Andrew Cooper wrote:
> The backlink field doesn't exist in a 64bit TSS, and union for esp{0..2} is of
> no practical use. Specify everything with stdint types, and empty bitfields
> for reserved values.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 2/7] x86/traps: Poison unused stack pointers in the TSS
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
2017-05-02 18:05 ` [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct Andrew Cooper
@ 2017-05-02 18:05 ` Andrew Cooper
2017-05-03 8:14 ` Jan Beulich
2017-05-03 13:29 ` [PATCH v2 " Andrew Cooper
2017-05-02 18:05 ` [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings Andrew Cooper
` (5 subsequent siblings)
7 siblings, 2 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-02 18:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
This is for additional defence-in-depth following LDT/GDT/IDT corruption.
It causes attempted control transfers to ring 1 or 2 (via a call gate), or
attempts to use IST 3 through 7 to yield #SS[0], rather than executing with a
stack starting at the top of virtual address space.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/cpu/common.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 6c27008..8796568 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -645,6 +645,14 @@ void load_system_tables(void)
tss->ist[IST_DF - 1] = stack_top + IST_DF * PAGE_SIZE;
tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
+ /* Poision all other stack pointers to prevent their accidental use. */
+ tss->rsp1 = 0x8600111111111111ul;
+ tss->rsp2 = 0x8600222222222222ul;
+ tss->ist[3] = 0x8600444444444444ul;
+ tss->ist[4] = 0x8600555555555555ul;
+ tss->ist[5] = 0x8600666666666666ul;
+ tss->ist[6] = 0x8600777777777777ul;
+
_set_tssldt_desc(
gdt + TSS_ENTRY,
(unsigned long)tss,
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 2/7] x86/traps: Poison unused stack pointers in the TSS
2017-05-02 18:05 ` [PATCH 2/7] x86/traps: Poison unused stack pointers in the TSS Andrew Cooper
@ 2017-05-03 8:14 ` Jan Beulich
2017-05-03 12:47 ` Andrew Cooper
2017-05-03 13:29 ` [PATCH v2 " Andrew Cooper
1 sibling, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 8:14 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -645,6 +645,14 @@ void load_system_tables(void)
> tss->ist[IST_DF - 1] = stack_top + IST_DF * PAGE_SIZE;
> tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
>
> + /* Poision all other stack pointers to prevent their accidental use. */
> + tss->rsp1 = 0x8600111111111111ul;
> + tss->rsp2 = 0x8600222222222222ul;
> + tss->ist[3] = 0x8600444444444444ul;
> + tss->ist[4] = 0x8600555555555555ul;
> + tss->ist[5] = 0x8600666666666666ul;
> + tss->ist[6] = 0x8600777777777777ul;
I think the ->ist[] part of this should be a loop from IST_MAX + 1
to 7 instead of the above, as what you have now doesn't easily
cope with IST indexes being added/removed.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/7] x86/traps: Poison unused stack pointers in the TSS
2017-05-03 8:14 ` Jan Beulich
@ 2017-05-03 12:47 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 12:47 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 03/05/17 09:14, Jan Beulich wrote:
>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -645,6 +645,14 @@ void load_system_tables(void)
>> tss->ist[IST_DF - 1] = stack_top + IST_DF * PAGE_SIZE;
>> tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
>>
>> + /* Poision all other stack pointers to prevent their accidental use. */
>> + tss->rsp1 = 0x8600111111111111ul;
>> + tss->rsp2 = 0x8600222222222222ul;
>> + tss->ist[3] = 0x8600444444444444ul;
>> + tss->ist[4] = 0x8600555555555555ul;
>> + tss->ist[5] = 0x8600666666666666ul;
>> + tss->ist[6] = 0x8600777777777777ul;
> I think the ->ist[] part of this should be a loop from IST_MAX + 1
> to 7 instead of the above, as what you have now doesn't easily
> cope with IST indexes being added/removed.
Part of the intention of having them here is that they are adjacent to
the other tss->ist[] setup, which will necessarily change if the indexes
get altered.
As for the constants in use, I'd originally intended to make each
pointer identifiable, but testing revealed that these constants end up
nowhere useful were crash to end up happening. Therefore, I don't have
a problem with using alternative constants, and indeed the same poison
constant for each pointer.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 2/7] x86/traps: Poison unused stack pointers in the TSS
2017-05-02 18:05 ` [PATCH 2/7] x86/traps: Poison unused stack pointers in the TSS Andrew Cooper
2017-05-03 8:14 ` Jan Beulich
@ 2017-05-03 13:29 ` Andrew Cooper
2017-05-03 13:45 ` Jan Beulich
1 sibling, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 13:29 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
This is for additional defence-in-depth following LDT/GDT/IDT corruption.
It causes attempted control transfers to ring 1 or 2 (via a call gate), or
attempts to use IST 3 through 7 to yield #SS, rather than executing with a
stack starting at the top of virtual address space.
Express the TSS setup in terms of structure assignment, which should be less
fragile if the IST indexes need to change, and has the useful side effect of
zeroing the reserved fields.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
v2:
* Rearange to avoid 6 straight assignments.
---
xen/arch/x86/cpu/common.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 6c27008..2fdc2f9 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -636,14 +636,29 @@ void load_system_tables(void)
.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
};
- /* Main stack for interrupts/exceptions. */
- tss->rsp0 = stack_bottom;
- tss->bitmap = IOBMP_INVALID_OFFSET;
-
- /* MCE, NMI and Double Fault handlers get their own stacks. */
- tss->ist[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE;
- tss->ist[IST_DF - 1] = stack_top + IST_DF * PAGE_SIZE;
- tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
+ *tss = (struct tss_struct){
+ /* Main stack for interrupts/exceptions. */
+ .rsp0 = stack_bottom,
+
+ /* Ring 1 and 2 stacks poisoned. */
+ .rsp1 = 0x8600111111111111ul,
+ .rsp2 = 0x8600111111111111ul,
+
+ /*
+ * MCE, NMI and Double Fault handlers get their own stacks.
+ * All others poisoned.
+ */
+ .ist = {
+ [IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE,
+ [IST_DF - 1] = stack_top + IST_DF * PAGE_SIZE,
+ [IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE,
+
+ [IST_MAX ... ARRAY_SIZE(tss->ist) - 1] =
+ 0x8600111111111111ul,
+ },
+
+ .bitmap = IOBMP_INVALID_OFFSET,
+ };
_set_tssldt_desc(
gdt + TSS_ENTRY,
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 2/7] x86/traps: Poison unused stack pointers in the TSS
2017-05-03 13:29 ` [PATCH v2 " Andrew Cooper
@ 2017-05-03 13:45 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 13:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 03.05.17 at 15:29, <andrew.cooper3@citrix.com> wrote:
> This is for additional defence-in-depth following LDT/GDT/IDT corruption.
>
> It causes attempted control transfers to ring 1 or 2 (via a call gate), or
> attempts to use IST 3 through 7 to yield #SS, rather than executing with a
> stack starting at the top of virtual address space.
>
> Express the TSS setup in terms of structure assignment, which should be less
> fragile if the IST indexes need to change, and has the useful side effect of
> zeroing the reserved fields.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
2017-05-02 18:05 ` [PATCH 1/7] x86/traps: Drop 32bit fields out of tss_struct Andrew Cooper
2017-05-02 18:05 ` [PATCH 2/7] x86/traps: Poison unused stack pointers in the TSS Andrew Cooper
@ 2017-05-02 18:05 ` Andrew Cooper
2017-05-03 8:49 ` Jan Beulich
` (3 more replies)
2017-05-02 18:05 ` [PATCH 4/7] x86/traps: Rename compat_hypercall() to entry_int82() Andrew Cooper
` (4 subsequent siblings)
7 siblings, 4 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-02 18:05 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Tim Deegan, Jan Beulich
As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
where the guest has full read access and a lot of direct or indirect control
over the written content. It isn't hard for a PV guest to hide shellcode
here.
Therefore, increase defence in depth by auditing our current pagetable
mappings.
* The regular linear, shadow linear, and per-domain slots have no business
being executable (but need to be written), so are updated to be NX.
* The Read Only mappings of the M2P (compat and regular) don't need to be
writeable or executable.
* The PV GDT mappings don't need to be executable.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/mm.c | 10 +++++-----
xen/arch/x86/mm/hap/hap.c | 4 ++--
xen/arch/x86/mm/shadow/multi.c | 18 +++++++++---------
xen/arch/x86/x86_64/mm.c | 12 ++++++------
5 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 90e2b1f..ef8c05a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2040,7 +2040,7 @@ static void __context_switch(void)
for ( i = 0; i < NR_RESERVED_GDT_PAGES; i++ )
l1e_write(pl1e + FIRST_RESERVED_GDT_PAGE + i,
- l1e_from_pfn(mfn + i, __PAGE_HYPERVISOR));
+ l1e_from_pfn(mfn + i, __PAGE_HYPERVISOR_RW));
}
if ( need_full_gdt(pd) &&
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e639ce2..77b0af1 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -384,7 +384,7 @@ void __init arch_init_memory(void)
for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
l3tab[i] = l3e_empty();
split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
- __PAGE_HYPERVISOR);
+ __PAGE_HYPERVISOR_RW);
}
else
++root_pgt_pv_xen_slots;
@@ -1588,9 +1588,9 @@ void init_guest_l4_table(l4_pgentry_t l4tab[], const struct domain *d,
split_l4e;
#endif
l4tab[l4_table_offset(LINEAR_PT_VIRT_START)] =
- l4e_from_pfn(domain_page_map_to_mfn(l4tab), __PAGE_HYPERVISOR);
+ l4e_from_pfn(domain_page_map_to_mfn(l4tab), __PAGE_HYPERVISOR_RW);
l4tab[l4_table_offset(PERDOMAIN_VIRT_START)] =
- l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR);
+ l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
if ( zap_ro_mpt || is_pv_32bit_domain(d) || paging_mode_refcounts(d) )
l4tab[l4_table_offset(RO_MPT_VIRT_START)] = l4e_empty();
}
@@ -6380,7 +6380,7 @@ int create_perdomain_mapping(struct domain *d, unsigned long va,
}
l2tab = __map_domain_page(pg);
clear_page(l2tab);
- l3tab[l3_table_offset(va)] = l3e_from_page(pg, __PAGE_HYPERVISOR);
+ l3tab[l3_table_offset(va)] = l3e_from_page(pg, __PAGE_HYPERVISOR_RW);
}
else
l2tab = map_domain_page(_mfn(l3e_get_pfn(l3tab[l3_table_offset(va)])));
@@ -6422,7 +6422,7 @@ int create_perdomain_mapping(struct domain *d, unsigned long va,
l1tab = __map_domain_page(pg);
}
clear_page(l1tab);
- *pl2e = l2e_from_page(pg, __PAGE_HYPERVISOR);
+ *pl2e = l2e_from_page(pg, __PAGE_HYPERVISOR_RW);
}
else if ( !l1tab )
l1tab = map_domain_page(_mfn(l2e_get_pfn(*pl2e)));
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index b981432..8476269 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -409,11 +409,11 @@ static void hap_install_xen_entries_in_l4(struct vcpu *v, mfn_t l4mfn)
/* Install the per-domain mappings for this domain */
l4e[l4_table_offset(PERDOMAIN_VIRT_START)] =
l4e_from_pfn(mfn_x(page_to_mfn(d->arch.perdomain_l3_pg)),
- __PAGE_HYPERVISOR);
+ __PAGE_HYPERVISOR_RW);
/* Install a linear mapping */
l4e[l4_table_offset(LINEAR_PT_VIRT_START)] =
- l4e_from_pfn(mfn_x(l4mfn), __PAGE_HYPERVISOR);
+ l4e_from_pfn(mfn_x(l4mfn), __PAGE_HYPERVISOR_RW);
unmap_domain_page(l4e);
}
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 2fb0125..f65ffc6 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -1475,7 +1475,7 @@ void sh_install_xen_entries_in_l4(struct domain *d, mfn_t gl4mfn, mfn_t sl4mfn)
/* Install the per-domain mappings for this domain */
sl4e[shadow_l4_table_offset(PERDOMAIN_VIRT_START)] =
shadow_l4e_from_mfn(page_to_mfn(d->arch.perdomain_l3_pg),
- __PAGE_HYPERVISOR);
+ __PAGE_HYPERVISOR_RW);
if ( !shadow_mode_external(d) && !is_pv_32bit_domain(d) &&
!VM_ASSIST(d, m2p_strict) )
@@ -1489,7 +1489,7 @@ void sh_install_xen_entries_in_l4(struct domain *d, mfn_t gl4mfn, mfn_t sl4mfn)
* monitor pagetable structure, which is built in make_monitor_table
* and maintained by sh_update_linear_entries. */
sl4e[shadow_l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
- shadow_l4e_from_mfn(sl4mfn, __PAGE_HYPERVISOR);
+ shadow_l4e_from_mfn(sl4mfn, __PAGE_HYPERVISOR_RW);
/* Self linear mapping. */
if ( shadow_mode_translate(d) && !shadow_mode_external(d) )
@@ -1501,7 +1501,7 @@ void sh_install_xen_entries_in_l4(struct domain *d, mfn_t gl4mfn, mfn_t sl4mfn)
else
{
sl4e[shadow_l4_table_offset(LINEAR_PT_VIRT_START)] =
- shadow_l4e_from_mfn(gl4mfn, __PAGE_HYPERVISOR);
+ shadow_l4e_from_mfn(gl4mfn, __PAGE_HYPERVISOR_RW);
}
unmap_domain_page(sl4e);
@@ -1654,12 +1654,12 @@ sh_make_monitor_table(struct vcpu *v)
m3mfn = shadow_alloc(d, SH_type_monitor_table, 0);
mfn_to_page(m3mfn)->shadow_flags = 3;
l4e[shadow_l4_table_offset(SH_LINEAR_PT_VIRT_START)]
- = l4e_from_pfn(mfn_x(m3mfn), __PAGE_HYPERVISOR);
+ = l4e_from_pfn(mfn_x(m3mfn), __PAGE_HYPERVISOR_RW);
m2mfn = shadow_alloc(d, SH_type_monitor_table, 0);
mfn_to_page(m2mfn)->shadow_flags = 2;
l3e = map_domain_page(m3mfn);
- l3e[0] = l3e_from_pfn(mfn_x(m2mfn), __PAGE_HYPERVISOR);
+ l3e[0] = l3e_from_pfn(mfn_x(m2mfn), __PAGE_HYPERVISOR_RW);
unmap_domain_page(l3e);
if ( is_pv_32bit_domain(d) )
@@ -1668,7 +1668,7 @@ sh_make_monitor_table(struct vcpu *v)
* area into its usual VAs in the monitor tables */
m3mfn = shadow_alloc(d, SH_type_monitor_table, 0);
mfn_to_page(m3mfn)->shadow_flags = 3;
- l4e[0] = l4e_from_pfn(mfn_x(m3mfn), __PAGE_HYPERVISOR);
+ l4e[0] = l4e_from_pfn(mfn_x(m3mfn), __PAGE_HYPERVISOR_RW);
m2mfn = shadow_alloc(d, SH_type_monitor_table, 0);
mfn_to_page(m2mfn)->shadow_flags = 2;
@@ -3838,7 +3838,7 @@ sh_update_linear_entries(struct vcpu *v)
{
__linear_l4_table[l4_linear_offset(SH_LINEAR_PT_VIRT_START)] =
l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
- __PAGE_HYPERVISOR);
+ __PAGE_HYPERVISOR_RW);
}
else
{
@@ -3846,7 +3846,7 @@ sh_update_linear_entries(struct vcpu *v)
ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
ml4e[l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
- __PAGE_HYPERVISOR);
+ __PAGE_HYPERVISOR_RW);
unmap_domain_page(ml4e);
}
}
@@ -3902,7 +3902,7 @@ sh_update_linear_entries(struct vcpu *v)
ml2e[i] =
(shadow_l3e_get_flags(sl3e[i]) & _PAGE_PRESENT)
? l2e_from_pfn(mfn_x(shadow_l3e_get_mfn(sl3e[i])),
- __PAGE_HYPERVISOR)
+ __PAGE_HYPERVISOR_RW)
: l2e_empty();
}
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 34f3250..ac358a8 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -470,7 +470,7 @@ static int setup_m2p_table(struct mem_hotadd_info *info)
clear_page(l2_ro_mpt);
l3e_write(&l3_ro_mpt[l3_table_offset(va)],
l3e_from_paddr(__pa(l2_ro_mpt),
- __PAGE_HYPERVISOR | _PAGE_USER));
+ __PAGE_HYPERVISOR_RO | _PAGE_USER));
l2_ro_mpt += l2_table_offset(va);
}
@@ -515,7 +515,7 @@ void __init paging_init(void)
l3_ro_mpt = page_to_virt(l3_pg);
clear_page(l3_ro_mpt);
l4e_write(&idle_pg_table[l4_table_offset(va)],
- l4e_from_page(l3_pg, __PAGE_HYPERVISOR));
+ l4e_from_page(l3_pg, __PAGE_HYPERVISOR_RW));
}
}
@@ -525,7 +525,7 @@ void __init paging_init(void)
l3_ro_mpt = page_to_virt(l3_pg);
clear_page(l3_ro_mpt);
l4e_write(&idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)],
- l4e_from_page(l3_pg, __PAGE_HYPERVISOR | _PAGE_USER));
+ l4e_from_page(l3_pg, __PAGE_HYPERVISOR_RO | _PAGE_USER));
/*
* Allocate and map the machine-to-phys table.
@@ -612,7 +612,7 @@ void __init paging_init(void)
l2_ro_mpt = page_to_virt(l2_pg);
clear_page(l2_ro_mpt);
l3e_write(&l3_ro_mpt[l3_table_offset(va)],
- l3e_from_page(l2_pg, __PAGE_HYPERVISOR | _PAGE_USER));
+ l3e_from_page(l2_pg, __PAGE_HYPERVISOR_RO | _PAGE_USER));
ASSERT(!l2_table_offset(va));
}
/* NB. Cannot be GLOBAL: guest user mode should not see it. */
@@ -634,7 +634,7 @@ void __init paging_init(void)
compat_idle_pg_table_l2 = l2_ro_mpt;
clear_page(l2_ro_mpt);
l3e_write(&l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)],
- l3e_from_paddr(__pa(l2_ro_mpt), __PAGE_HYPERVISOR));
+ l3e_from_paddr(__pa(l2_ro_mpt), __PAGE_HYPERVISOR_RO));
l2_ro_mpt += l2_table_offset(HIRO_COMPAT_MPT_VIRT_START);
/* Allocate and map the compatibility mode machine-to-phys table. */
mpt_size = (mpt_size >> 1) + (1UL << (L2_PAGETABLE_SHIFT - 1));
@@ -679,7 +679,7 @@ void __init paging_init(void)
/* Set up linear page table mapping. */
l4e_write(&idle_pg_table[l4_table_offset(LINEAR_PT_VIRT_START)],
- l4e_from_paddr(__pa(idle_pg_table), __PAGE_HYPERVISOR));
+ l4e_from_paddr(__pa(idle_pg_table), __PAGE_HYPERVISOR_RW));
return;
nomem:
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings
2017-05-02 18:05 ` [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings Andrew Cooper
@ 2017-05-03 8:49 ` Jan Beulich
2017-05-03 13:38 ` Andrew Cooper
2017-05-03 9:48 ` Wei Liu
` (2 subsequent siblings)
3 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 8:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Tim Deegan, Xen-devel
>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
> where the guest has full read access and a lot of direct or indirect control
> over the written content. It isn't hard for a PV guest to hide shellcode
> here.
>
> Therefore, increase defence in depth by auditing our current pagetable
> mappings.
>
> * The regular linear, shadow linear, and per-domain slots have no business
> being executable (but need to be written), so are updated to be NX.
> * The Read Only mappings of the M2P (compat and regular) don't need to be
> writeable or executable.
> * The PV GDT mappings don't need to be executable.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two remarks:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -384,7 +384,7 @@ void __init arch_init_memory(void)
> for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
> l3tab[i] = l3e_empty();
> split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
> - __PAGE_HYPERVISOR);
> + __PAGE_HYPERVISOR_RW);
Would be nice if this change (affecting the direct map) was also
mentioned in the commit message, even if it's only debugging
code.
> @@ -515,7 +515,7 @@ void __init paging_init(void)
> l3_ro_mpt = page_to_virt(l3_pg);
> clear_page(l3_ro_mpt);
> l4e_write(&idle_pg_table[l4_table_offset(va)],
> - l4e_from_page(l3_pg, __PAGE_HYPERVISOR));
> + l4e_from_page(l3_pg, __PAGE_HYPERVISOR_RW));
Similarly here (again affecting the direct map).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings
2017-05-03 8:49 ` Jan Beulich
@ 2017-05-03 13:38 ` Andrew Cooper
2017-05-03 13:48 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 13:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Tim Deegan, Xen-devel
On 03/05/17 09:49, Jan Beulich wrote:
>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
>> where the guest has full read access and a lot of direct or indirect control
>> over the written content. It isn't hard for a PV guest to hide shellcode
>> here.
>>
>> Therefore, increase defence in depth by auditing our current pagetable
>> mappings.
>>
>> * The regular linear, shadow linear, and per-domain slots have no business
>> being executable (but need to be written), so are updated to be NX.
>> * The Read Only mappings of the M2P (compat and regular) don't need to be
>> writeable or executable.
>> * The PV GDT mappings don't need to be executable.
>>
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with two remarks:
>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -384,7 +384,7 @@ void __init arch_init_memory(void)
>> for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
>> l3tab[i] = l3e_empty();
>> split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
>> - __PAGE_HYPERVISOR);
>> + __PAGE_HYPERVISOR_RW);
> Would be nice if this change (affecting the direct map) was also
> mentioned in the commit message, even if it's only debugging
> code.
>
>> @@ -515,7 +515,7 @@ void __init paging_init(void)
>> l3_ro_mpt = page_to_virt(l3_pg);
>> clear_page(l3_ro_mpt);
>> l4e_write(&idle_pg_table[l4_table_offset(va)],
>> - l4e_from_page(l3_pg, __PAGE_HYPERVISOR));
>> + l4e_from_page(l3_pg, __PAGE_HYPERVISOR_RW));
> Similarly here (again affecting the direct map).
Updated. The text now reads
* The PV GDT mappings and bits of the directmap don't need to be
executable.
My method of working out which areas to change were to consider all uses
of __PAGE_HYPERVISOR. I have half a mind to submit a change renaming it
to __PAGE_PGTABLE, as it should only really be used to build
intermediate pagetable entries where we control X/NX, R/W or S/U at a
more fine grained level.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings
2017-05-03 13:38 ` Andrew Cooper
@ 2017-05-03 13:48 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 13:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Tim Deegan, Xen-devel
>>> On 03.05.17 at 15:38, <andrew.cooper3@citrix.com> wrote:
> My method of working out which areas to change were to consider all uses
> of __PAGE_HYPERVISOR. I have half a mind to submit a change renaming it
> to __PAGE_PGTABLE, as it should only really be used to build
> intermediate pagetable entries where we control X/NX, R/W or S/U at a
> more fine grained level.
Yeah, getting rid of that overly permissive __PAGE_HYPERVISOR
would likely be a good thing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings
2017-05-02 18:05 ` [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings Andrew Cooper
2017-05-03 8:49 ` Jan Beulich
@ 2017-05-03 9:48 ` Wei Liu
2017-05-03 10:11 ` Tim Deegan
2017-05-03 11:13 ` George Dunlap
3 siblings, 0 replies; 48+ messages in thread
From: Wei Liu @ 2017-05-03 9:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Wei Liu, Tim Deegan, Jan Beulich, Xen-devel
On Tue, May 02, 2017 at 07:05:22PM +0100, Andrew Cooper wrote:
> As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
> where the guest has full read access and a lot of direct or indirect control
> over the written content. It isn't hard for a PV guest to hide shellcode
> here.
>
> Therefore, increase defence in depth by auditing our current pagetable
> mappings.
>
> * The regular linear, shadow linear, and per-domain slots have no business
> being executable (but need to be written), so are updated to be NX.
> * The Read Only mappings of the M2P (compat and regular) don't need to be
> writeable or executable.
> * The PV GDT mappings don't need to be executable.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings
2017-05-02 18:05 ` [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings Andrew Cooper
2017-05-03 8:49 ` Jan Beulich
2017-05-03 9:48 ` Wei Liu
@ 2017-05-03 10:11 ` Tim Deegan
2017-05-03 11:13 ` George Dunlap
3 siblings, 0 replies; 48+ messages in thread
From: Tim Deegan @ 2017-05-03 10:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Jan Beulich, Xen-devel
Hi,
At 19:05 +0100 on 02 May (1493751922), Andrew Cooper wrote:
> As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
> where the guest has full read access and a lot of direct or indirect control
> over the written content. It isn't hard for a PV guest to hide shellcode
> here.
>
> Therefore, increase defence in depth by auditing our current pagetable
> mappings.
>
> * The regular linear, shadow linear, and per-domain slots have no business
> being executable (but need to be written), so are updated to be NX.
> * The Read Only mappings of the M2P (compat and regular) don't need to be
> writeable or executable.
> * The PV GDT mappings don't need to be executable.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings
2017-05-02 18:05 ` [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings Andrew Cooper
` (2 preceding siblings ...)
2017-05-03 10:11 ` Tim Deegan
@ 2017-05-03 11:13 ` George Dunlap
3 siblings, 0 replies; 48+ messages in thread
From: George Dunlap @ 2017-05-03 11:13 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Tim Deegan, Jan Beulich
On 02/05/17 19:05, Andrew Cooper wrote:
> As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
> where the guest has full read access and a lot of direct or indirect control
> over the written content. It isn't hard for a PV guest to hide shellcode
> here.
>
> Therefore, increase defence in depth by auditing our current pagetable
> mappings.
>
> * The regular linear, shadow linear, and per-domain slots have no business
> being executable (but need to be written), so are updated to be NX.
> * The Read Only mappings of the M2P (compat and regular) don't need to be
> writeable or executable.
> * The PV GDT mappings don't need to be executable.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 4/7] x86/traps: Rename compat_hypercall() to entry_int82()
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
` (2 preceding siblings ...)
2017-05-02 18:05 ` [PATCH 3/7] x86/mm: Further restrict permissions on some virtual mappings Andrew Cooper
@ 2017-05-02 18:05 ` Andrew Cooper
2017-05-03 8:55 ` Jan Beulich
2017-05-02 18:05 ` [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C Andrew Cooper
` (3 subsequent siblings)
7 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-02 18:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
This follows the Linux example of naming the entry point by how it is arrived
at, rather than its purpose.
Doing so highlights that the SAVE_VOLATILE instantiation sets up the wrong
entry_vector on the stack (although this is benign as we never sysret to a
32bit PV guest, and the iret path doesn't care).
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/traps.c | 2 +-
xen/arch/x86/x86_64/compat/entry.S | 4 ++--
xen/include/asm-x86/processor.h | 3 ++-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 1c90177..3ab70e5 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3872,7 +3872,7 @@ void __init trap_init(void)
/* The 32-on-64 hypercall vector is only accessible from ring 1. */
_set_gate(idt_table + HYPERCALL_VECTOR,
- SYS_DESC_trap_gate, 1, &compat_hypercall);
+ SYS_DESC_trap_gate, 1, &entry_int82);
/* Fast trap for int80 (faster than taking the #GP-fixup path). */
_set_gate(idt_table + 0x80, SYS_DESC_trap_gate, 3, &int80_direct_trap);
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index fb72464..abf1094 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -11,10 +11,10 @@
#include <public/xen.h>
#include <irq_vectors.h>
-ENTRY(compat_hypercall)
+ENTRY(entry_int82)
ASM_CLAC
pushq $0
- SAVE_VOLATILE type=TRAP_syscall compat=1
+ SAVE_VOLATILE type=HYPERCALL_VECTOR compat=1
CR4_PV32_RESTORE
cmpb $0,untrusted_msi(%rip)
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index f244e10..1d1a4ff 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -529,6 +529,8 @@ DECLARE_TRAP_HANDLER(simd_coprocessor_error);
DECLARE_TRAP_HANDLER_CONST(machine_check);
DECLARE_TRAP_HANDLER(alignment_check);
+DECLARE_TRAP_HANDLER(entry_int82);
+
#undef DECLARE_TRAP_HANDLER_CONST
#undef DECLARE_TRAP_HANDLER
@@ -538,7 +540,6 @@ void do_reserved_trap(struct cpu_user_regs *regs);
void sysenter_entry(void);
void sysenter_eflags_saved(void);
-void compat_hypercall(void);
void int80_direct_trap(void);
#define STUBS_PER_PAGE (PAGE_SIZE / STUB_BUF_SIZE)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 4/7] x86/traps: Rename compat_hypercall() to entry_int82()
2017-05-02 18:05 ` [PATCH 4/7] x86/traps: Rename compat_hypercall() to entry_int82() Andrew Cooper
@ 2017-05-03 8:55 ` Jan Beulich
2017-05-03 13:41 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 8:55 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -3872,7 +3872,7 @@ void __init trap_init(void)
>
> /* The 32-on-64 hypercall vector is only accessible from ring 1. */
> _set_gate(idt_table + HYPERCALL_VECTOR,
> - SYS_DESC_trap_gate, 1, &compat_hypercall);
> + SYS_DESC_trap_gate, 1, &entry_int82);
Would you mind at once dropping the unnecessary & ?
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -529,6 +529,8 @@ DECLARE_TRAP_HANDLER(simd_coprocessor_error);
> DECLARE_TRAP_HANDLER_CONST(machine_check);
> DECLARE_TRAP_HANDLER(alignment_check);
>
> +DECLARE_TRAP_HANDLER(entry_int82);
This is inappropriate, I think: There's no do_entry_int82. With this
changed to a simply void entry_int82(void) declaration
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/7] x86/traps: Rename compat_hypercall() to entry_int82()
2017-05-03 8:55 ` Jan Beulich
@ 2017-05-03 13:41 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 13:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 03/05/17 09:55, Jan Beulich wrote:
>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -3872,7 +3872,7 @@ void __init trap_init(void)
>>
>> /* The 32-on-64 hypercall vector is only accessible from ring 1. */
>> _set_gate(idt_table + HYPERCALL_VECTOR,
>> - SYS_DESC_trap_gate, 1, &compat_hypercall);
>> + SYS_DESC_trap_gate, 1, &entry_int82);
> Would you mind at once dropping the unnecessary & ?
Fixed.
>
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -529,6 +529,8 @@ DECLARE_TRAP_HANDLER(simd_coprocessor_error);
>> DECLARE_TRAP_HANDLER_CONST(machine_check);
>> DECLARE_TRAP_HANDLER(alignment_check);
>>
>> +DECLARE_TRAP_HANDLER(entry_int82);
> This is inappropriate, I think: There's no do_entry_int82. With this
> changed to a simply void entry_int82(void) declaration
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
I hadn't even realised I had split the introduction of the two functions
across two patches, but I'd prefer to keep them split like this and I
will ensure that this gets committed at the same time as the following
patch.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
` (3 preceding siblings ...)
2017-05-02 18:05 ` [PATCH 4/7] x86/traps: Rename compat_hypercall() to entry_int82() Andrew Cooper
@ 2017-05-02 18:05 ` Andrew Cooper
2017-05-03 9:02 ` Jan Beulich
2017-05-02 18:05 ` [PATCH 6/7] x86/asm: Fold LOAD_C_CLOBBERED into RESTORE_ALL Andrew Cooper
` (2 subsequent siblings)
7 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-02 18:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
This is more readable, maintainable, and livepatchable.
This involves declaring check_for_unexpected_msi(), untrusted_msi and
pv_hypercall() suitably for use by C. While making these changes,
untrusted_msi is switched over to being a C99 bool.
No behavioural change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/pv/Makefile | 2 ++
xen/arch/x86/pv/traps.c | 44 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/x86_64/compat/entry.S | 9 +-------
xen/drivers/passthrough/vtd/iommu.c | 4 ++--
xen/include/asm-x86/apic.h | 1 +
xen/include/asm-x86/hypercall.h | 4 ++++
xen/include/asm-x86/iommu.h | 2 ++
7 files changed, 56 insertions(+), 10 deletions(-)
create mode 100644 xen/arch/x86/pv/traps.c
diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
index ea94599..8a295d0 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -1,2 +1,4 @@
obj-y += hypercall.o
+obj-y += traps.o
+
obj-bin-y += dom0_build.init.o
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
new file mode 100644
index 0000000..f3d8f70
--- /dev/null
+++ b/xen/arch/x86/pv/traps.c
@@ -0,0 +1,44 @@
+/******************************************************************************
+ * arch/x86/pv/traps.c
+ *
+ * PV low level entry points.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2017 Citrix Systems Ltd.
+ */
+
+#include <xen/hypercall.h>
+
+#include <asm/apic.h>
+
+#ifdef CONFIG_COMPAT
+void do_entry_int82(struct cpu_user_regs *regs)
+{
+ if ( unlikely(untrusted_msi) )
+ check_for_unexpected_msi((uint8_t)regs->entry_vector);
+
+ pv_hypercall(regs);
+}
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index abf1094..90bda09 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -17,17 +17,10 @@ ENTRY(entry_int82)
SAVE_VOLATILE type=HYPERCALL_VECTOR compat=1
CR4_PV32_RESTORE
- cmpb $0,untrusted_msi(%rip)
-UNLIKELY_START(ne, msi_check)
- movl $HYPERCALL_VECTOR,%edi
- call check_for_unexpected_msi
- LOAD_C_CLOBBERED compat=1 ax=0
-UNLIKELY_END(msi_check)
-
GET_CURRENT(bx)
mov %rsp, %rdi
- call pv_hypercall
+ call do_entry_int82
/* %rbx: struct vcpu */
ENTRY(compat_test_all_events)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index a5c61c6..19328f6 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -48,7 +48,7 @@ struct mapped_rmrr {
};
/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
-bool_t __read_mostly untrusted_msi;
+bool __read_mostly untrusted_msi;
int nr_iommus;
@@ -2334,7 +2334,7 @@ static int reassign_device_ownership(
* by the root complex unless interrupt remapping is enabled.
*/
if ( (target != hardware_domain) && !iommu_intremap )
- untrusted_msi = 1;
+ untrusted_msi = true;
/*
* If the device belongs to the hardware domain, and it has RMRR, don't
diff --git a/xen/include/asm-x86/apic.h b/xen/include/asm-x86/apic.h
index 2f1398df..9952039 100644
--- a/xen/include/asm-x86/apic.h
+++ b/xen/include/asm-x86/apic.h
@@ -213,6 +213,7 @@ extern int lapic_suspend(void);
extern int lapic_resume(void);
extern void record_boot_APIC_mode(void);
extern enum apic_mode current_local_apic_mode(void);
+extern void check_for_unexpected_msi(unsigned int vector);
extern int check_nmi_watchdog (void);
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index c59aa69..95d48c0 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -25,6 +25,10 @@ typedef struct {
extern const hypercall_args_t hypercall_args_table[NR_hypercalls];
+#ifdef CONFIG_PV
+void pv_hypercall(struct cpu_user_regs *regs);
+#endif
+
/*
* Both do_mmuext_op() and do_mmu_update():
* We steal the m.s.b. of the @count parameter to indicate whether this
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 0431233..14ad048 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -92,6 +92,8 @@ bool_t iommu_supports_eim(void);
int iommu_enable_x2apic_IR(void);
void iommu_disable_x2apic_IR(void);
+extern bool untrusted_msi;
+
int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
const uint8_t gvec);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-02 18:05 ` [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C Andrew Cooper
@ 2017-05-03 9:02 ` Jan Beulich
2017-05-03 11:26 ` Wei Liu
2017-05-04 10:01 ` [PATCH v2 " Andrew Cooper
0 siblings, 2 replies; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 9:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/pv/traps.c
> @@ -0,0 +1,44 @@
> +/******************************************************************************
> + * arch/x86/pv/traps.c
> + *
> + * PV low level entry points.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2017 Citrix Systems Ltd.
> + */
> +
> +#include <xen/hypercall.h>
> +
> +#include <asm/apic.h>
> +
> +#ifdef CONFIG_COMPAT
As expressed before, I disagree to the re-introduction of such
conditionals in x86 code.
> +void do_entry_int82(struct cpu_user_regs *regs)
Ah, so here we go. If this and patch 4 get committed together, I
withdraw my declaration specific change request on that other
patch.
> --- a/xen/include/asm-x86/hypercall.h
> +++ b/xen/include/asm-x86/hypercall.h
> @@ -25,6 +25,10 @@ typedef struct {
>
> extern const hypercall_args_t hypercall_args_table[NR_hypercalls];
>
> +#ifdef CONFIG_PV
> +void pv_hypercall(struct cpu_user_regs *regs);
> +#endif
Are such #ifdef-s really useful? Having the declaration does no harm,
linking would fail if the config setting is off.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 9:02 ` Jan Beulich
@ 2017-05-03 11:26 ` Wei Liu
2017-05-03 11:38 ` Andrew Cooper
2017-05-03 12:00 ` Jan Beulich
2017-05-04 10:01 ` [PATCH v2 " Andrew Cooper
1 sibling, 2 replies; 48+ messages in thread
From: Wei Liu @ 2017-05-03 11:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, Xen-devel
On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
> >>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> > --- /dev/null
> > +++ b/xen/arch/x86/pv/traps.c
> > @@ -0,0 +1,44 @@
> > +/******************************************************************************
> > + * arch/x86/pv/traps.c
> > + *
> > + * PV low level entry points.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Copyright (c) 2017 Citrix Systems Ltd.
> > + */
> > +
> > +#include <xen/hypercall.h>
> > +
> > +#include <asm/apic.h>
> > +
> > +#ifdef CONFIG_COMPAT
>
> As expressed before, I disagree to the re-introduction of such
> conditionals in x86 code.
>
I'm curious to know how the COMPAT interface is treated long term.
I guess you're of the opinion that we should always have them enabled?
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 11:26 ` Wei Liu
@ 2017-05-03 11:38 ` Andrew Cooper
2017-05-03 11:43 ` Wei Liu
2017-05-03 12:02 ` Jan Beulich
2017-05-03 12:00 ` Jan Beulich
1 sibling, 2 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 11:38 UTC (permalink / raw)
To: Wei Liu, Jan Beulich; +Cc: Xen-devel
On 03/05/17 12:26, Wei Liu wrote:
> On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
>>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/pv/traps.c
>>> @@ -0,0 +1,44 @@
>>> +/******************************************************************************
>>> + * arch/x86/pv/traps.c
>>> + *
>>> + * PV low level entry points.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + * Copyright (c) 2017 Citrix Systems Ltd.
>>> + */
>>> +
>>> +#include <xen/hypercall.h>
>>> +
>>> +#include <asm/apic.h>
>>> +
>>> +#ifdef CONFIG_COMPAT
>> As expressed before, I disagree to the re-introduction of such
>> conditionals in x86 code.
>>
> I'm curious to know how the COMPAT interface is treated long term.
>
> I guess you're of the opinion that we should always have them enabled?
There is a valid usecase to disable CONFIG_COMPAT, seeing as sufficient
PVH interfaces exist to start APs straight in 64bit mode, as it provides
a meaningful reduction in hypervisor attack surface.
As it is a configurable option, I intend to work in a direction which
eventually makes it usable under x86.
If there is a wish to move in an opposite direction, that should be a
separate discussion made over a patch removing its entry from
common/Kconfig.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 11:38 ` Andrew Cooper
@ 2017-05-03 11:43 ` Wei Liu
2017-05-03 12:02 ` Jan Beulich
1 sibling, 0 replies; 48+ messages in thread
From: Wei Liu @ 2017-05-03 11:43 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel
On Wed, May 03, 2017 at 12:38:15PM +0100, Andrew Cooper wrote:
> On 03/05/17 12:26, Wei Liu wrote:
> > On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
> >>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/pv/traps.c
> >>> @@ -0,0 +1,44 @@
> >>> +/******************************************************************************
> >>> + * arch/x86/pv/traps.c
> >>> + *
> >>> + * PV low level entry points.
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify
> >>> + * it under the terms of the GNU General Public License as published by
> >>> + * the Free Software Foundation; either version 2 of the License, or
> >>> + * (at your option) any later version.
> >>> + *
> >>> + * This program is distributed in the hope that it will be useful,
> >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >>> + * GNU General Public License for more details.
> >>> + *
> >>> + * You should have received a copy of the GNU General Public License
> >>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> >>> + *
> >>> + * Copyright (c) 2017 Citrix Systems Ltd.
> >>> + */
> >>> +
> >>> +#include <xen/hypercall.h>
> >>> +
> >>> +#include <asm/apic.h>
> >>> +
> >>> +#ifdef CONFIG_COMPAT
> >> As expressed before, I disagree to the re-introduction of such
> >> conditionals in x86 code.
> >>
> > I'm curious to know how the COMPAT interface is treated long term.
> >
> > I guess you're of the opinion that we should always have them enabled?
>
> There is a valid usecase to disable CONFIG_COMPAT, seeing as sufficient
> PVH interfaces exist to start APs straight in 64bit mode, as it provides
> a meaningful reduction in hypervisor attack surface.
>
I agree.
> As it is a configurable option, I intend to work in a direction which
> eventually makes it usable under x86.
>
> If there is a wish to move in an opposite direction, that should be a
> separate discussion made over a patch removing its entry from
> common/Kconfig.
>
Yes, I think a proper discussion is needed to clarify the future
direction.
Wei.
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 11:38 ` Andrew Cooper
2017-05-03 11:43 ` Wei Liu
@ 2017-05-03 12:02 ` Jan Beulich
2017-05-03 12:18 ` Andrew Cooper
1 sibling, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 12:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 03.05.17 at 13:38, <andrew.cooper3@citrix.com> wrote:
> On 03/05/17 12:26, Wei Liu wrote:
>> On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
>>>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/pv/traps.c
>>>> @@ -0,0 +1,44 @@
>>>>
> +/***************************************************************************
> ***
>>>> + * arch/x86/pv/traps.c
>>>> + *
>>>> + * PV low level entry points.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>>> + *
>>>> + * Copyright (c) 2017 Citrix Systems Ltd.
>>>> + */
>>>> +
>>>> +#include <xen/hypercall.h>
>>>> +
>>>> +#include <asm/apic.h>
>>>> +
>>>> +#ifdef CONFIG_COMPAT
>>> As expressed before, I disagree to the re-introduction of such
>>> conditionals in x86 code.
>>>
>> I'm curious to know how the COMPAT interface is treated long term.
>>
>> I guess you're of the opinion that we should always have them enabled?
>
> There is a valid usecase to disable CONFIG_COMPAT, seeing as sufficient
> PVH interfaces exist to start APs straight in 64bit mode, as it provides
> a meaningful reduction in hypervisor attack surface.
What does PVH have to do with 32-bit PV compat guest support?
> As it is a configurable option, I intend to work in a direction which
> eventually makes it usable under x86.
>
> If there is a wish to move in an opposite direction, that should be a
> separate discussion made over a patch removing its entry from
> common/Kconfig.
Once again - from common code perspective this is a valid config
option to have. X86, however, unconditionally selects it, so there's
no point having such conditionals in x86 code.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 12:02 ` Jan Beulich
@ 2017-05-03 12:18 ` Andrew Cooper
2017-05-03 12:37 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 12:18 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 03/05/17 13:02, Jan Beulich wrote:
>>>> On 03.05.17 at 13:38, <andrew.cooper3@citrix.com> wrote:
>> On 03/05/17 12:26, Wei Liu wrote:
>>> On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
>>>>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/pv/traps.c
>>>>> @@ -0,0 +1,44 @@
>>>>>
>> +/***************************************************************************
>> ***
>>>>> + * arch/x86/pv/traps.c
>>>>> + *
>>>>> + * PV low level entry points.
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License as published by
>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>> + * (at your option) any later version.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>> + * GNU General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General Public License
>>>>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>>>> + *
>>>>> + * Copyright (c) 2017 Citrix Systems Ltd.
>>>>> + */
>>>>> +
>>>>> +#include <xen/hypercall.h>
>>>>> +
>>>>> +#include <asm/apic.h>
>>>>> +
>>>>> +#ifdef CONFIG_COMPAT
>>>> As expressed before, I disagree to the re-introduction of such
>>>> conditionals in x86 code.
>>>>
>>> I'm curious to know how the COMPAT interface is treated long term.
>>>
>>> I guess you're of the opinion that we should always have them enabled?
>> There is a valid usecase to disable CONFIG_COMPAT, seeing as sufficient
>> PVH interfaces exist to start APs straight in 64bit mode, as it provides
>> a meaningful reduction in hypervisor attack surface.
> What does PVH have to do with 32-bit PV compat guest support?
Nothing. I am unsure as to why do you think it does?
The CONFIG_COMPAT infrastructure by HVM guests as well.
>
>> As it is a configurable option, I intend to work in a direction which
>> eventually makes it usable under x86.
>>
>> If there is a wish to move in an opposite direction, that should be a
>> separate discussion made over a patch removing its entry from
>> common/Kconfig.
> Once again - from common code perspective this is a valid config
> option to have. X86, however, unconditionally selects it, so there's
> no point having such conditionals in x86 code.
I don't agree with this reasoning. If it is a reasonable configuration
for common code to use, it is a reasonable configuration for x86 code to
use.
Or do you disagree with my argument for why it should be a configurable
option which can be turned off in an x86 build?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 12:18 ` Andrew Cooper
@ 2017-05-03 12:37 ` Jan Beulich
2017-05-03 18:29 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 12:37 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 03.05.17 at 14:18, <andrew.cooper3@citrix.com> wrote:
> On 03/05/17 13:02, Jan Beulich wrote:
>>>>> On 03.05.17 at 13:38, <andrew.cooper3@citrix.com> wrote:
>>> On 03/05/17 12:26, Wei Liu wrote:
>>>> On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
>>>>>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/arch/x86/pv/traps.c
>>>>>> @@ -0,0 +1,44 @@
>>>>>>
>>>
> +/***************************************************************************
>>> ***
>>>>>> + * arch/x86/pv/traps.c
>>>>>> + *
>>>>>> + * PV low level entry points.
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>> + * it under the terms of the GNU General Public License as published by
>>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>>> + * (at your option) any later version.
>>>>>> + *
>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>>> + * GNU General Public License for more details.
>>>>>> + *
>>>>>> + * You should have received a copy of the GNU General Public License
>>>>>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>>>>> + *
>>>>>> + * Copyright (c) 2017 Citrix Systems Ltd.
>>>>>> + */
>>>>>> +
>>>>>> +#include <xen/hypercall.h>
>>>>>> +
>>>>>> +#include <asm/apic.h>
>>>>>> +
>>>>>> +#ifdef CONFIG_COMPAT
>>>>> As expressed before, I disagree to the re-introduction of such
>>>>> conditionals in x86 code.
>>>>>
>>>> I'm curious to know how the COMPAT interface is treated long term.
>>>>
>>>> I guess you're of the opinion that we should always have them enabled?
>>> There is a valid usecase to disable CONFIG_COMPAT, seeing as sufficient
>>> PVH interfaces exist to start APs straight in 64bit mode, as it provides
>>> a meaningful reduction in hypervisor attack surface.
>> What does PVH have to do with 32-bit PV compat guest support?
>
> Nothing. I am unsure as to why do you think it does?
>
> The CONFIG_COMPAT infrastructure by HVM guests as well.
A rather small part of it, if at all. And if HVM guests really use parts
of it, we can't disable it without breaking such guests running
(perhaps just temporarily) in 32-bit mode.
>>> As it is a configurable option, I intend to work in a direction which
>>> eventually makes it usable under x86.
>>>
>>> If there is a wish to move in an opposite direction, that should be a
>>> separate discussion made over a patch removing its entry from
>>> common/Kconfig.
>> Once again - from common code perspective this is a valid config
>> option to have. X86, however, unconditionally selects it, so there's
>> no point having such conditionals in x86 code.
>
> I don't agree with this reasoning. If it is a reasonable configuration
> for common code to use, it is a reasonable configuration for x86 code to
> use.
Depends: It's there in common code to skip the respective pieces of
code for architectures not needing it. It used to be a meaningful
option in x86 as long as there was a 32-bit hypervisor.
> Or do you disagree with my argument for why it should be a configurable
> option which can be turned off in an x86 build?
Well, I'd first of all have to check again how much of this really is
being used by HVM code. Hmm, looks like it's more than I did
remember. But with all CONFIG_COMPAT uses gone from x86
code (minus the ones you introduced not very long ago), I don't
see why you want to re-introduce a few more now. If we really
want the option of disabling that code, a concerted effort should
be made to re-add the conditionals wherever needed. Otherwise
we'll end up with code where readers would legitimately ask why
the conditionals are there in some places, but missing in many
others. And no, I wouldn't view a patch like the one here as the
beginning of a transitional period (as expressed before, I should
have noticed and objected to the re-introduction of such #ifdef-s
in your PV hypercall changes).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 12:37 ` Jan Beulich
@ 2017-05-03 18:29 ` Andrew Cooper
2017-05-04 7:27 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 18:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 03/05/17 13:37, Jan Beulich wrote:
>>>> On 03.05.17 at 14:18, <andrew.cooper3@citrix.com> wrote:
>> On 03/05/17 13:02, Jan Beulich wrote:
>>>>>> On 03.05.17 at 13:38, <andrew.cooper3@citrix.com> wrote:
>>>> On 03/05/17 12:26, Wei Liu wrote:
>>>>> On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
>>>>>>>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>>>>>>> --- /dev/null
>>>>>>> +++ b/xen/arch/x86/pv/traps.c
>>>>>>> @@ -0,0 +1,44 @@
>>>>>>>
>> +/***************************************************************************
>>>> ***
>>>>>>> + * arch/x86/pv/traps.c
>>>>>>> + *
>>>>>>> + * PV low level entry points.
>>>>>>> + *
>>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>>> + * it under the terms of the GNU General Public License as published by
>>>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>>>> + * (at your option) any later version.
>>>>>>> + *
>>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>>>> + * GNU General Public License for more details.
>>>>>>> + *
>>>>>>> + * You should have received a copy of the GNU General Public License
>>>>>>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>>>>>> + *
>>>>>>> + * Copyright (c) 2017 Citrix Systems Ltd.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <xen/hypercall.h>
>>>>>>> +
>>>>>>> +#include <asm/apic.h>
>>>>>>> +
>>>>>>> +#ifdef CONFIG_COMPAT
>>>>>> As expressed before, I disagree to the re-introduction of such
>>>>>> conditionals in x86 code.
>>>>>>
>>>>> I'm curious to know how the COMPAT interface is treated long term.
>>>>>
>>>>> I guess you're of the opinion that we should always have them enabled?
>>>> There is a valid usecase to disable CONFIG_COMPAT, seeing as sufficient
>>>> PVH interfaces exist to start APs straight in 64bit mode, as it provides
>>>> a meaningful reduction in hypervisor attack surface.
>>> What does PVH have to do with 32-bit PV compat guest support?
>> Nothing. I am unsure as to why do you think it does?
>>
>> The CONFIG_COMPAT infrastructure by HVM guests as well.
> A rather small part of it, if at all. And if HVM guests really use parts
> of it, we can't disable it without breaking such guests running
> (perhaps just temporarily) in 32-bit mode.
>
>>>> As it is a configurable option, I intend to work in a direction which
>>>> eventually makes it usable under x86.
>>>>
>>>> If there is a wish to move in an opposite direction, that should be a
>>>> separate discussion made over a patch removing its entry from
>>>> common/Kconfig.
>>> Once again - from common code perspective this is a valid config
>>> option to have. X86, however, unconditionally selects it, so there's
>>> no point having such conditionals in x86 code.
>> I don't agree with this reasoning. If it is a reasonable configuration
>> for common code to use, it is a reasonable configuration for x86 code to
>> use.
> Depends: It's there in common code to skip the respective pieces of
> code for architectures not needing it. It used to be a meaningful
> option in x86 as long as there was a 32-bit hypervisor.
>
>> Or do you disagree with my argument for why it should be a configurable
>> option which can be turned off in an x86 build?
> Well, I'd first of all have to check again how much of this really is
> being used by HVM code. Hmm, looks like it's more than I did
> remember. But with all CONFIG_COMPAT uses gone from x86
> code (minus the ones you introduced not very long ago), I don't
> see why you want to re-introduce a few more now. If we really
> want the option of disabling that code, a concerted effort should
> be made to re-add the conditionals wherever needed. Otherwise
> we'll end up with code where readers would legitimately ask why
> the conditionals are there in some places, but missing in many
> others. And no, I wouldn't view a patch like the one here as the
> beginning of a transitional period (as expressed before, I should
> have noticed and objected to the re-introduction of such #ifdef-s
> in your PV hypercall changes).
The reason c/s de82feebf2c uses CONFIG_COMPAT is because of
struct mc_state {
unsigned long flags;
union {
struct multicall_entry call;
#ifdef CONFIG_COMPAT
struct compat_multicall_entry compat_call;
#endif
};
};
being defined using CONFIG_COMPAT. It didn't even cross my mind that it
might have been ok to allow the code to fail to compile if CONFIG_COMPAT
was not selected. I therefore wrote code which matched the structure
definition it was using.
Irrespective of the history which lead to this point, the important
question is whether we want to allow compiling x86 without CONFIG_COMPAT.
If the eventual decision is yes, then new code should specifically be
introduced as being CONFIG_COMPAT-clean, because decisions like that
affect how to structure the code in the first place, and therefore be
far cleaner changes than trying to retrofit CONFIG_COMPAT in the future.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 18:29 ` Andrew Cooper
@ 2017-05-04 7:27 ` Jan Beulich
2017-05-04 9:27 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-04 7:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 03.05.17 at 20:29, <andrew.cooper3@citrix.com> wrote:
> Irrespective of the history which lead to this point, the important
> question is whether we want to allow compiling x86 without CONFIG_COMPAT.
>
> If the eventual decision is yes, then new code should specifically be
> introduced as being CONFIG_COMPAT-clean, because decisions like that
> affect how to structure the code in the first place, and therefore be
> far cleaner changes than trying to retrofit CONFIG_COMPAT in the future.
Well, okay, I can agree with this. So what would an x86-Xen with
COMPAT off support? 64-bit guests only, regardless of type? As
said before, I don't view it as a reasonable setup to allow only
64-bit HVM guests, so unless there is an intention to have a
configuration where only PVH guests are supported (i.e. neither
PV nor traditional HVM), I don't think allowing the option to be
turned off makes sense on x86.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-04 7:27 ` Jan Beulich
@ 2017-05-04 9:27 ` Andrew Cooper
2017-05-04 9:36 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-04 9:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 04/05/17 08:27, Jan Beulich wrote:
>>>> On 03.05.17 at 20:29, <andrew.cooper3@citrix.com> wrote:
>> Irrespective of the history which lead to this point, the important
>> question is whether we want to allow compiling x86 without CONFIG_COMPAT.
>>
>> If the eventual decision is yes, then new code should specifically be
>> introduced as being CONFIG_COMPAT-clean, because decisions like that
>> affect how to structure the code in the first place, and therefore be
>> far cleaner changes than trying to retrofit CONFIG_COMPAT in the future.
> Well, okay, I can agree with this. So what would an x86-Xen with
> COMPAT off support? 64-bit guests only, regardless of type?
Yes. I intend this to mean 64bit guests of any type.
> As said before, I don't view it as a reasonable setup to allow only
> 64-bit HVM guests, so unless there is an intention to have a
> configuration where only PVH guests are supported (i.e. neither
> PV nor traditional HVM), I don't think allowing the option to be
> turned off makes sense on x86.
HVM and PVH aren't very different, but there will be a small amount of
work required to get plain HVM booting like this, because of hvmloader
currently being 32bit only. I do see (potentially) value in allowing
64bit-only traditional HVM guests, e.g. with a 64bit OVMF firmware
(although this is already turning fairly PVH in nature).
PVH on the other hand is already capable of booting and running properly
were CONFIG_COMPAT to be disabled today.
The selection of CONFIG_PV, CONFIG_HVM and CONFIG_COMPAT will eventually
define the matrix of which guests are supported in this build of the
hypervisor, based on which chunks of functionality they compile out.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-04 9:27 ` Andrew Cooper
@ 2017-05-04 9:36 ` Jan Beulich
2017-05-04 9:57 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-04 9:36 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 04.05.17 at 11:27, <andrew.cooper3@citrix.com> wrote:
> On 04/05/17 08:27, Jan Beulich wrote:
>> As said before, I don't view it as a reasonable setup to allow only
>> 64-bit HVM guests, so unless there is an intention to have a
>> configuration where only PVH guests are supported (i.e. neither
>> PV nor traditional HVM), I don't think allowing the option to be
>> turned off makes sense on x86.
>
> HVM and PVH aren't very different, but there will be a small amount of
> work required to get plain HVM booting like this, because of hvmloader
> currently being 32bit only. I do see (potentially) value in allowing
> 64bit-only traditional HVM guests, e.g. with a 64bit OVMF firmware
> (although this is already turning fairly PVH in nature).
So that would then be yet another construct with no raw hardware
equivalent. I'd prefer if we strove for improving our compatibility with
raw hardware (as we do in various other places, you being one of
the primary advocates to do so) instead of inventing new custom
environments. Furthermore, while I can see the benefit of e.g. not
allowing PV guests for security reasons, I'm having a hard time
imagining some security benefit from disallowing a HVM guest to run
in 32-bit mode. But perhaps you've thought of something already...
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-04 9:36 ` Jan Beulich
@ 2017-05-04 9:57 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-04 9:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 04/05/17 10:36, Jan Beulich wrote:
>>>> On 04.05.17 at 11:27, <andrew.cooper3@citrix.com> wrote:
>> On 04/05/17 08:27, Jan Beulich wrote:
>>> As said before, I don't view it as a reasonable setup to allow only
>>> 64-bit HVM guests, so unless there is an intention to have a
>>> configuration where only PVH guests are supported (i.e. neither
>>> PV nor traditional HVM), I don't think allowing the option to be
>>> turned off makes sense on x86.
>> HVM and PVH aren't very different, but there will be a small amount of
>> work required to get plain HVM booting like this, because of hvmloader
>> currently being 32bit only. I do see (potentially) value in allowing
>> 64bit-only traditional HVM guests, e.g. with a 64bit OVMF firmware
>> (although this is already turning fairly PVH in nature).
> So that would then be yet another construct with no raw hardware
> equivalent. I'd prefer if we strove for improving our compatibility with
> raw hardware (as we do in various other places, you being one of
> the primary advocates to do so) instead of inventing new custom
> environments. Furthermore, while I can see the benefit of e.g. not
> allowing PV guests for security reasons, I'm having a hard time
> imagining some security benefit from disallowing a HVM guest to run
> in 32-bit mode. But perhaps you've thought of something already...
Hmm. It might be nice to be able to compile out 32bit PV without
excluding 32bit HVM.
The security gain for COMPAT HVM guests is fractional, compared to PV,
but is non-zero; we have had infinite loops in the compat translation
code before.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 11:26 ` Wei Liu
2017-05-03 11:38 ` Andrew Cooper
@ 2017-05-03 12:00 ` Jan Beulich
1 sibling, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 12:00 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Xen-devel
>>> On 03.05.17 at 13:26, <wei.liu2@citrix.com> wrote:
> On Wed, May 03, 2017 at 03:02:25AM -0600, Jan Beulich wrote:
>> >>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
>> > --- /dev/null
>> > +++ b/xen/arch/x86/pv/traps.c
>> > @@ -0,0 +1,44 @@
>> >
> +/***************************************************************************
> ***
>> > + * arch/x86/pv/traps.c
>> > + *
>> > + * PV low level entry points.
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License
>> > + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>> > + *
>> > + * Copyright (c) 2017 Citrix Systems Ltd.
>> > + */
>> > +
>> > +#include <xen/hypercall.h>
>> > +
>> > +#include <asm/apic.h>
>> > +
>> > +#ifdef CONFIG_COMPAT
>>
>> As expressed before, I disagree to the re-introduction of such
>> conditionals in x86 code.
>>
>
> I'm curious to know how the COMPAT interface is treated long term.
>
> I guess you're of the opinion that we should always have them enabled?
Yes, that's why all of its uses in x86 code had been removed
(until Andrew recently re-introduced some).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-03 9:02 ` Jan Beulich
2017-05-03 11:26 ` Wei Liu
@ 2017-05-04 10:01 ` Andrew Cooper
2017-05-04 10:16 ` Andrew Cooper
2017-05-04 10:22 ` Jan Beulich
1 sibling, 2 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-04 10:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
This is more readable, maintainable, and livepatchable.
This involves declaring check_for_unexpected_msi(), untrusted_msi and
pv_hypercall() suitably for use by C. While making these changes,
untrusted_msi is switched over to being a C99 bool.
No behavioural change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
v2:
* Drop ifdefary for now
---
xen/arch/x86/pv/Makefile | 2 ++
xen/arch/x86/pv/traps.c | 42 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/x86_64/compat/entry.S | 9 +-------
xen/drivers/passthrough/vtd/iommu.c | 4 ++--
xen/include/asm-x86/apic.h | 1 +
xen/include/asm-x86/hypercall.h | 2 ++
xen/include/asm-x86/iommu.h | 2 ++
7 files changed, 52 insertions(+), 10 deletions(-)
create mode 100644 xen/arch/x86/pv/traps.c
diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
index ea94599..8a295d0 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -1,2 +1,4 @@
obj-y += hypercall.o
+obj-y += traps.o
+
obj-bin-y += dom0_build.init.o
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
new file mode 100644
index 0000000..51125a8
--- /dev/null
+++ b/xen/arch/x86/pv/traps.c
@@ -0,0 +1,42 @@
+/******************************************************************************
+ * arch/x86/pv/traps.c
+ *
+ * PV low level entry points.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2017 Citrix Systems Ltd.
+ */
+
+#include <xen/hypercall.h>
+
+#include <asm/apic.h>
+
+void do_entry_int82(struct cpu_user_regs *regs)
+{
+ if ( unlikely(untrusted_msi) )
+ check_for_unexpected_msi((uint8_t)regs->entry_vector);
+
+ pv_hypercall(regs);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index abf1094..90bda09 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -17,17 +17,10 @@ ENTRY(entry_int82)
SAVE_VOLATILE type=HYPERCALL_VECTOR compat=1
CR4_PV32_RESTORE
- cmpb $0,untrusted_msi(%rip)
-UNLIKELY_START(ne, msi_check)
- movl $HYPERCALL_VECTOR,%edi
- call check_for_unexpected_msi
- LOAD_C_CLOBBERED compat=1 ax=0
-UNLIKELY_END(msi_check)
-
GET_CURRENT(bx)
mov %rsp, %rdi
- call pv_hypercall
+ call do_entry_int82
/* %rbx: struct vcpu */
ENTRY(compat_test_all_events)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index a5c61c6..19328f6 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -48,7 +48,7 @@ struct mapped_rmrr {
};
/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
-bool_t __read_mostly untrusted_msi;
+bool __read_mostly untrusted_msi;
int nr_iommus;
@@ -2334,7 +2334,7 @@ static int reassign_device_ownership(
* by the root complex unless interrupt remapping is enabled.
*/
if ( (target != hardware_domain) && !iommu_intremap )
- untrusted_msi = 1;
+ untrusted_msi = true;
/*
* If the device belongs to the hardware domain, and it has RMRR, don't
diff --git a/xen/include/asm-x86/apic.h b/xen/include/asm-x86/apic.h
index 2f1398df..9952039 100644
--- a/xen/include/asm-x86/apic.h
+++ b/xen/include/asm-x86/apic.h
@@ -213,6 +213,7 @@ extern int lapic_suspend(void);
extern int lapic_resume(void);
extern void record_boot_APIC_mode(void);
extern enum apic_mode current_local_apic_mode(void);
+extern void check_for_unexpected_msi(unsigned int vector);
extern int check_nmi_watchdog (void);
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index c59aa69..cfbcefe 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -25,6 +25,8 @@ typedef struct {
extern const hypercall_args_t hypercall_args_table[NR_hypercalls];
+void pv_hypercall(struct cpu_user_regs *regs);
+
/*
* Both do_mmuext_op() and do_mmu_update():
* We steal the m.s.b. of the @count parameter to indicate whether this
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 0431233..14ad048 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -92,6 +92,8 @@ bool_t iommu_supports_eim(void);
int iommu_enable_x2apic_IR(void);
void iommu_disable_x2apic_IR(void);
+extern bool untrusted_msi;
+
int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
const uint8_t gvec);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-04 10:01 ` [PATCH v2 " Andrew Cooper
@ 2017-05-04 10:16 ` Andrew Cooper
2017-05-04 10:28 ` Jan Beulich
2017-05-04 10:22 ` Jan Beulich
1 sibling, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-04 10:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Jan Beulich
On 04/05/17 11:01, Andrew Cooper wrote:
> This is more readable, maintainable, and livepatchable.
>
> This involves declaring check_for_unexpected_msi(), untrusted_msi and
> pv_hypercall() suitably for use by C. While making these changes,
> untrusted_msi is switched over to being a C99 bool.
>
> No behavioural change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Actually, in addition, I think the following change should be folded in.
andrewcoop@andrewcoop:/local/xen.git/xen$ git diff
diff --git a/xen/arch/x86/x86_64/compat/entry.S
b/xen/arch/x86/x86_64/compat/entry.S
index 90bda09..51b1c257 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -21,6 +21,7 @@ ENTRY(entry_int82)
mov %rsp, %rdi
call do_entry_int82
+ jmp compat_test_all_events
/* %rbx: struct vcpu */
ENTRY(compat_test_all_events)
This only currently works because the ALIGN hidden in ENTRY() happens to
be safe to execute through, but we shouldn't rely on this, and a single
predicted jump is probably faster to execute than a line of single-byte
nops.
Thoughts?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-04 10:16 ` Andrew Cooper
@ 2017-05-04 10:28 ` Jan Beulich
2017-05-04 11:09 ` Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-04 10:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 04.05.17 at 12:16, <andrew.cooper3@citrix.com> wrote:
> On 04/05/17 11:01, Andrew Cooper wrote:
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -21,6 +21,7 @@ ENTRY(entry_int82)
>
> mov %rsp, %rdi
> call do_entry_int82
> + jmp compat_test_all_events
>
> /* %rbx: struct vcpu */
> ENTRY(compat_test_all_events)
>
>
> This only currently works because the ALIGN hidden in ENTRY() happens to
> be safe to execute through, but we shouldn't rely on this,
Why not? ALIGN is the way it is for a reason.
> and a single predicted jump is probably faster to execute than
> a line of single-byte nops.
I'm not sure about this, but it's possible. A question is whether we
really should force 0x90 fillers in ALIGN, rather than having the
assembler use multi-bytes NOPs when suitable.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-04 10:28 ` Jan Beulich
@ 2017-05-04 11:09 ` Andrew Cooper
0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-04 11:09 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 04/05/17 11:28, Jan Beulich wrote:
>>>> On 04.05.17 at 12:16, <andrew.cooper3@citrix.com> wrote:
>> On 04/05/17 11:01, Andrew Cooper wrote:
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -21,6 +21,7 @@ ENTRY(entry_int82)
>>
>> mov %rsp, %rdi
>> call do_entry_int82
>> + jmp compat_test_all_events
>>
>> /* %rbx: struct vcpu */
>> ENTRY(compat_test_all_events)
>>
>>
>> This only currently works because the ALIGN hidden in ENTRY() happens to
>> be safe to execute through, but we shouldn't rely on this,
> Why not? ALIGN is the way it is for a reason.
It still doesn't strike me as a safe or sensible thing to rely on, and
will become forbidden if/when we decide to borrow some of the objtool
improvements from Linux.
>
>> and a single predicted jump is probably faster to execute than
>> a line of single-byte nops.
> I'm not sure about this, but it's possible. A question is whether we
> really should force 0x90 fillers in ALIGN, rather than having the
> assembler use multi-bytes NOPs when suitable.
I've been experimenting and it appears that some ALIGNs are reduced to
multibyte nops, while others aren't. I can't find any consistency.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C
2017-05-04 10:01 ` [PATCH v2 " Andrew Cooper
2017-05-04 10:16 ` Andrew Cooper
@ 2017-05-04 10:22 ` Jan Beulich
1 sibling, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2017-05-04 10:22 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 04.05.17 at 12:01, <andrew.cooper3@citrix.com> wrote:
> This is more readable, maintainable, and livepatchable.
>
> This involves declaring check_for_unexpected_msi(), untrusted_msi and
> pv_hypercall() suitably for use by C. While making these changes,
> untrusted_msi is switched over to being a C99 bool.
>
> No behavioural change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
without the subsequently suggested adjustment; I'll reply there.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 6/7] x86/asm: Fold LOAD_C_CLOBBERED into RESTORE_ALL
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
` (4 preceding siblings ...)
2017-05-02 18:05 ` [PATCH 5/7] x86/traps: Lift all non-entrypoint logic in entry_int82() up into C Andrew Cooper
@ 2017-05-02 18:05 ` Andrew Cooper
2017-05-03 9:08 ` Jan Beulich
2017-05-03 9:48 ` Wei Liu
2017-05-02 18:05 ` [PATCH 7/7] x86/asm: Clobber %r{8..15} on exit to 32bit PV guests Andrew Cooper
2017-05-04 11:11 ` [RFC for 4.9] [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
7 siblings, 2 replies; 48+ messages in thread
From: Andrew Cooper @ 2017-05-02 18:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
With its sole other user removed, fold LOAD_C_CLOBBERED into RESTORE_ALL to
reduce the cognitive load of trying to work out which registers get modified.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/include/asm-x86/asm_defns.h | 27 ++++++---------------------
1 file changed, 6 insertions(+), 21 deletions(-)
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index f1c6fa1..11306d1 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -321,40 +321,25 @@ static always_inline void stac(void)
.endif
/*
- * Reload registers not preserved by C code from frame.
- *
- * @compat: R8-R11 don't need reloading
+ * Restore all previously saved registers.
*
- * For the way it is used in RESTORE_ALL, this macro must preserve EFLAGS.ZF.
+ * @adj: extra stack pointer adjustment to be folded into the adjustment done
+ * anyway at the end of the macro
+ * @compat: R8-R15 don't need reloading
*/
-.macro LOAD_C_CLOBBERED compat=0 ax=1
+.macro RESTORE_ALL adj=0 compat=0
.if !\compat
+ testl $TRAP_regs_dirty,UREGS_entry_vector(%rsp)
movq UREGS_r11(%rsp),%r11
movq UREGS_r10(%rsp),%r10
movq UREGS_r9(%rsp),%r9
movq UREGS_r8(%rsp),%r8
.endif
-.if \ax
LOAD_ONE_REG(ax, \compat)
-.endif
LOAD_ONE_REG(cx, \compat)
LOAD_ONE_REG(dx, \compat)
LOAD_ONE_REG(si, \compat)
LOAD_ONE_REG(di, \compat)
-.endm
-
-/*
- * Restore all previously saved registers.
- *
- * @adj: extra stack pointer adjustment to be folded into the adjustment done
- * anyway at the end of the macro
- * @compat: R8-R15 don't need reloading
- */
-.macro RESTORE_ALL adj=0 compat=0
-.if !\compat
- testl $TRAP_regs_dirty,UREGS_entry_vector(%rsp)
-.endif
- LOAD_C_CLOBBERED \compat
.if !\compat
jz 987f
movq UREGS_r15(%rsp),%r15
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 6/7] x86/asm: Fold LOAD_C_CLOBBERED into RESTORE_ALL
2017-05-02 18:05 ` [PATCH 6/7] x86/asm: Fold LOAD_C_CLOBBERED into RESTORE_ALL Andrew Cooper
@ 2017-05-03 9:08 ` Jan Beulich
2017-05-03 9:48 ` Wei Liu
1 sibling, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 9:08 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> With its sole other user removed, fold LOAD_C_CLOBBERED into RESTORE_ALL to
> reduce the cognitive load of trying to work out which registers get
> modified.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 6/7] x86/asm: Fold LOAD_C_CLOBBERED into RESTORE_ALL
2017-05-02 18:05 ` [PATCH 6/7] x86/asm: Fold LOAD_C_CLOBBERED into RESTORE_ALL Andrew Cooper
2017-05-03 9:08 ` Jan Beulich
@ 2017-05-03 9:48 ` Wei Liu
1 sibling, 0 replies; 48+ messages in thread
From: Wei Liu @ 2017-05-03 9:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel
On Tue, May 02, 2017 at 07:05:25PM +0100, Andrew Cooper wrote:
> With its sole other user removed, fold LOAD_C_CLOBBERED into RESTORE_ALL to
> reduce the cognitive load of trying to work out which registers get modified.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 7/7] x86/asm: Clobber %r{8..15} on exit to 32bit PV guests
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
` (5 preceding siblings ...)
2017-05-02 18:05 ` [PATCH 6/7] x86/asm: Fold LOAD_C_CLOBBERED into RESTORE_ALL Andrew Cooper
@ 2017-05-02 18:05 ` Andrew Cooper
2017-05-03 9:13 ` Jan Beulich
2017-05-04 11:11 ` [RFC for 4.9] [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
7 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-02 18:05 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
In the presence of bugs such as XSA-214 where a 32bit PV guest can get its
hands on a long mode segment, this change prevents register content leaking
between domains.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/include/asm-x86/asm_defns.h | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 11306d1..4b5891f 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -325,7 +325,8 @@ static always_inline void stac(void)
*
* @adj: extra stack pointer adjustment to be folded into the adjustment done
* anyway at the end of the macro
- * @compat: R8-R15 don't need reloading
+ * @compat: R8-R15 don't need reloading, but they are clobbered for added
+ * safety against information leaks.
*/
.macro RESTORE_ALL adj=0 compat=0
.if !\compat
@@ -366,6 +367,16 @@ static always_inline void stac(void)
LOAD_ONE_REG(bp, \compat)
LOAD_ONE_REG(bx, \compat)
subq $-(UREGS_error_code-UREGS_r15+\adj), %rsp
+.if \compat
+ xor %r8d, %r8d
+ xor %r9d, %r9d
+ xor %r10d, %r10d
+ xor %r11d, %r11d
+ xor %r12d, %r12d
+ xor %r13d, %r13d
+ xor %r14d, %r14d
+ xor %r15d, %r15d
+.endif
.endm
#endif
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 7/7] x86/asm: Clobber %r{8..15} on exit to 32bit PV guests
2017-05-02 18:05 ` [PATCH 7/7] x86/asm: Clobber %r{8..15} on exit to 32bit PV guests Andrew Cooper
@ 2017-05-03 9:13 ` Jan Beulich
2017-05-03 17:51 ` [PATCH v2 " Andrew Cooper
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-05-03 9:13 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 02.05.17 at 20:05, <andrew.cooper3@citrix.com> wrote:
> @@ -366,6 +367,16 @@ static always_inline void stac(void)
> LOAD_ONE_REG(bp, \compat)
> LOAD_ONE_REG(bx, \compat)
> subq $-(UREGS_error_code-UREGS_r15+\adj), %rsp
> +.if \compat
> + xor %r8d, %r8d
> + xor %r9d, %r9d
> + xor %r10d, %r10d
> + xor %r11d, %r11d
> + xor %r12d, %r12d
> + xor %r13d, %r13d
> + xor %r14d, %r14d
> + xor %r15d, %r15d
> +.endif
> .endm
Rather than having yet another .if, I think this would better be the
.else to one of the existing ones. Possibly even two .else-s, one
each for the R8-R11 and R12-R15 register ranges.
Also the d suffixes on the register names are pretty pointless here,
as a REX prefix is going to be needed anyway.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v2 7/7] x86/asm: Clobber %r{8..15} on exit to 32bit PV guests
2017-05-03 9:13 ` Jan Beulich
@ 2017-05-03 17:51 ` Andrew Cooper
2017-05-04 8:50 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-03 17:51 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
In the presence of bugs such as XSA-214 where a 32bit PV guest can get its
hands on a long mode segment, this change prevents register content leaking
between domains.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
v2:
* Move to being in .else clauses
---
xen/include/asm-x86/asm_defns.h | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 11306d1..388fc93 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -325,7 +325,8 @@ static always_inline void stac(void)
*
* @adj: extra stack pointer adjustment to be folded into the adjustment done
* anyway at the end of the macro
- * @compat: R8-R15 don't need reloading
+ * @compat: R8-R15 don't need reloading, but they are clobbered for added
+ * safety against information leaks.
*/
.macro RESTORE_ALL adj=0 compat=0
.if !\compat
@@ -334,6 +335,11 @@ static always_inline void stac(void)
movq UREGS_r10(%rsp),%r10
movq UREGS_r9(%rsp),%r9
movq UREGS_r8(%rsp),%r8
+.else
+ xor %r11, %r11
+ xor %r10, %r10
+ xor %r9, %r9
+ xor %r8, %r8
.endif
LOAD_ONE_REG(ax, \compat)
LOAD_ONE_REG(cx, \compat)
@@ -361,6 +367,11 @@ static always_inline void stac(void)
789: BUG /* Corruption of partial register state. */
.subsection 0
#endif
+.else
+ xor %r15, %r15
+ xor %r14, %r14
+ xor %r13, %r13
+ xor %r12, %r12
.endif
987:
LOAD_ONE_REG(bp, \compat)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 7/7] x86/asm: Clobber %r{8..15} on exit to 32bit PV guests
2017-05-03 17:51 ` [PATCH v2 " Andrew Cooper
@ 2017-05-04 8:50 ` Jan Beulich
0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2017-05-04 8:50 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 03.05.17 at 19:51, <andrew.cooper3@citrix.com> wrote:
> In the presence of bugs such as XSA-214 where a 32bit PV guest can get its
> hands on a long mode segment, this change prevents register content leaking
> between domains.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC for 4.9] [PATCH 0/7] XSAs 213-315 followups
2017-05-02 18:05 [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
` (6 preceding siblings ...)
2017-05-02 18:05 ` [PATCH 7/7] x86/asm: Clobber %r{8..15} on exit to 32bit PV guests Andrew Cooper
@ 2017-05-04 11:11 ` Andrew Cooper
2017-05-04 12:52 ` Julien Grall
7 siblings, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2017-05-04 11:11 UTC (permalink / raw)
To: Xen-devel, Julien Grall; +Cc: George Dunlap, Tim Deegan, Jan Beulich
On 02/05/17 19:05, Andrew Cooper wrote:
> Various improvements based on observations while investigating and fixing the
> aformentioned XSAs. All are candidate patches for 4.9 at this point, with
> patches 2, 3 and 7 being the important ones from an attack-surface-mitigation
> point of view.
>
> This is the subset of my intented series, but I haven't had time to finish the
> rest of the work I wanted to do, which clearly makes it post 4.9 work at this
> point.
Julien: I have v2 of this series now fully reviewed/acked.
I'd like to request them for inclusion into 4.9 at this point (unless
there are any last minute objections from other maintainers).
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [RFC for 4.9] [PATCH 0/7] XSAs 213-315 followups
2017-05-04 11:11 ` [RFC for 4.9] [PATCH 0/7] XSAs 213-315 followups Andrew Cooper
@ 2017-05-04 12:52 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-05-04 12:52 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Tim Deegan, Jan Beulich
Hi Andrew,
On 04/05/17 12:11, Andrew Cooper wrote:
> On 02/05/17 19:05, Andrew Cooper wrote:
>> Various improvements based on observations while investigating and fixing the
>> aformentioned XSAs. All are candidate patches for 4.9 at this point, with
>> patches 2, 3 and 7 being the important ones from an attack-surface-mitigation
>> point of view.
>>
>> This is the subset of my intented series, but I haven't had time to finish the
>> rest of the work I wanted to do, which clearly makes it post 4.9 work at this
>> point.
>
> Julien: I have v2 of this series now fully reviewed/acked.
>
> I'd like to request them for inclusion into 4.9 at this point (unless
> there are any last minute objections from other maintainers).
I agree that mitigating the attack-surface is important for Xen 4.9:
Release-acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread