Re: [Xen-devel] [PATCH v2 1/2] x86/xpti: really hide almost all of Xen image

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/03/18 17:04, Jan Beulich wrote:
>>>> On 02.03.18 at 17:53, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 02/03/18 14:34, Jan Beulich wrote:
>>> Note that the removed BUILD_BUG_ON()s don't get replaced by anything -
>>> there already is a suitable ASSERT() in xen.lds.S.
>> This isn't quite true.  You've changed the mechanism by which the stubs
>> get mapped (from entirely common, to per-pcpu), removing the need for
>> the BUILD_BUG_ON().
>>
>> The ASSERT() in xen.lds.S serves a different purpose, checking that the
>> sum total of stubs don't overlap with the compiled code.  (On this
>> note... do we perform the same check for livepatches?  I can't spot
>> anything.)
> What you say may be true for the one that was in
> setup_cpu_root_pgt(), but surely not the one I'm removing from
> alloc_stub_page(). But I can drop this if you prefer.

I think it might avoid some confusion.

>
>>> What should we do with the TSS? Currently together with it we expose
>>> almost a full page of other per-CPU data. A simple (but slightly
>>> hackish) option would be to use one of the two unused stack slots.
>> In 64bit, the TSS can be mapped read-only, because hardware never has
>> cause to write to it.
>>
>> I believe that Linux now uses a read-only TSS mapping to double as a
>> guard page for the trampoline stack, which is a less hacky way of
>> thinking about it.
>>
>> However, doing that in Xen would mean shattering the directmap
>> superpages in all cases, and we'd inherit the SVM triple fault case into
>> release builds.  A different alternative (and perhaps simpler to
>> backport) might be to have .bss.percpu.page_aligned and use that to hide
>> the surrounding data.
> Well, yes, that's obviously an option, but pretty wasteful. I'd then
> be tempted to at least do some sharing of the page similar to how
> the stubs of several CPUs share a single page.

For backport to older releases?

I think the extra almost 4k per pcpu isn't going to concern people (its
the least of their problems right now), and there is a very tangible
benefit of not leaking the other surrounding data.

>
>> Thinking about it, we've got the same problem with the TSS as the BSP
>> IDT, if the link order happens to cause init_tss to cross a page boundary.
> I don't think so, no - the structure is 128 bytes in size and 128
> byte aligned. When I created the original XPTI light patch I did
> specifically check.

This only happens by chance, because sizeof(struct tss_struct) ==
SMP_CACHE_BYTES

If we intend to rely on this behaviour, we want something like this:

diff --git a/xen/include/asm-x86/processor.h
b/xen/include/asm-x86/processor.h
index 9c70a98..fe647dc 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -385,7 +385,7 @@ static always_inline void __mwait(unsigned long eax,
unsigned long ecx)
 #define IOBMP_BYTES             8192
 #define IOBMP_INVALID_OFFSET    0x8000
 
-struct __packed __cacheline_aligned tss_struct {
+struct __packed tss_struct {
     uint32_t :32;
     uint64_t rsp0, rsp1, rsp2;
     uint64_t :64;
@@ -398,7 +398,7 @@ struct __packed __cacheline_aligned tss_struct {
     uint16_t :16, bitmap;
     /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
     uint8_t __cacheline_filler[24];
-};
+} __aligned(sizeof(struct tss_struct));
 
 #define IST_NONE 0UL
 #define IST_DF   1UL

except that C can't cope with this expression.  I wonder if there is an
alternate way with typedefs.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel