lkml.org 
[lkml]   [2015]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 1/3] x86/ldt: Make modify_ldt synchronous
    From
    Date
    On 22/07/2015 01:28, Andy Lutomirski wrote:
    > On Tue, Jul 21, 2015 at 5:21 PM, Andrew Cooper
    > <andrew.cooper3@citrix.com> wrote:
    >> On 22/07/2015 01:07, Andy Lutomirski wrote:
    >>> On Tue, Jul 21, 2015 at 4:38 PM, Andrew Cooper
    >>> <andrew.cooper3@citrix.com> wrote:
    >>>> On 21/07/2015 22:53, Boris Ostrovsky wrote:
    >>>>> On 07/21/2015 03:59 PM, Andy Lutomirski wrote:
    >>>>>> --- a/arch/x86/include/asm/mmu_context.h
    >>>>>> +++ b/arch/x86/include/asm/mmu_context.h
    >>>>>> @@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
    >>>>>> *mm) {}
    >>>>>> #endif
    >>>>>> /*
    >>>>>> + * ldt_structs can be allocated, used, and freed, but they are never
    >>>>>> + * modified while live.
    >>>>>> + */
    >>>>>> +struct ldt_struct {
    >>>>>> + int size;
    >>>>>> + int __pad; /* keep the descriptors naturally aligned. */
    >>>>>> + struct desc_struct entries[];
    >>>>>> +};
    >>>>>
    >>>>> This breaks Xen which expects LDT to be page-aligned. Not sure why.
    >>>>>
    >>>>> Jan, Andrew?
    >>>> PV guests are not permitted to have writeable mappings to the frames
    >>>> making up the GDT and LDT, so it cannot make unaudited changes to
    >>>> loadable descriptors. In particular, for a 32bit PV guest, it is only
    >>>> the segment limit which protects Xen from the ring1 guest kernel.
    >>>>
    >>>> A lot of this code hasn't been touched in years, and it certainly
    >>>> predates me. The alignment requirement appears to come from the virtual
    >>>> region Xen uses to map the guests GDT and LDT. Strict alignment is
    >>>> required for the GDT so Xen's descriptors starting at 0xe0xx are
    >>>> correct, but the LDT alignment seems to be a side effect of similar
    >>>> codepaths.
    >>>>
    >>>> For an LDT smaller than 8192 entries, I can't see any specific reason
    >>>> for enforcing alignment, other than "that's the way it has always been".
    >>>>
    >>>> However, the guest would still have to relinquish write access to all
    >>>> frames which make up the LDT, which looks to be a bit of an issue given
    >>>> the snippet above.
    >>> Does the LDT itself need to be aligned or just the address passed to
    >>> paravirt_alloc_ldt?
    >> The address which Xen receives needs to be aligned.
    >>
    >> It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt
    >> it is passed is page aligned, and passes it straight through.
    > xen_alloc_ldt is just fiddling with protection though, I think. Isn't
    > it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a
    > pointer to the ldt_struct.

    So it is. It is the linear_addr in xen_set_ldt() which Xen currently
    audits to be page aligned.

    >>>> This will allow ldt_struct itself to be page aligned, and for the size
    >>>> field to sit across the base/limit field of what would logically be
    >>>> selector 0x0008 There would be some issues accessing size. To load
    >>>> frames as an LDT, a guest must drop all refs to the page so that its
    >>>> type may be changed from writeable to segdesc. After that, an
    >>>> update_descriptor hypercall can be used to change size, and I believe
    >>>> the guest may subsequently recreate read-only mappings to the frames in
    >>>> question (although frankly it is getting late so you will want to double
    >>>> check all of this).
    >>>>
    >>>> Anyhow, this looks like an issue which should be fixed up with slightly
    >>>> more PVOps, rather than enforcing a Xen view of the world on native Linux.
    >>>>
    >>> I could presumably make the allocation the other way around so the
    >>> size is at the end. I could even use two separate allocations if
    >>> needed.
    >> I suspect two separate allocations would be the better solution, as it
    >> means that the size field doesn't need to be subject to funny page
    >> permissions.
    > True. OTOH we never write to the size field after allocating the thing.

    Right, but even reading it is going to cause problems if one of the
    paravirt ops can't re-establish ro mappings.

    ~Andrew


    \
     
     \ /
      Last update: 2015-07-22 03:01    [W:3.389 / U:0.940 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site