Re: [PATCH v4 0/3] x86: modify_ldt improvement, test, and config option

From: Andy Lutomirski
Date: Mon Jul 27 2015 - 23:23:27 EST

On Mon, Jul 27, 2015 at 8:16 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On Mon, Jul 27, 2015 at 7:20 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>> On Mon, Jul 27, 2015 at 9:18 AM, Boris Ostrovsky
>> <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 07/27/2015 11:53 AM, Andy Lutomirski wrote:
>>>>
>>>> On Mon, Jul 27, 2015 at 8:36 AM, Boris Ostrovsky
>>>> <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>>
>>>>> On 07/25/2015 01:36 AM, Andy Lutomirski wrote:
>>>>>>
>>>>>> Here's v3. It fixes the "dazed and confused" issue, I hope. It's also
>>>>>> probably a good general attack surface reduction, and it replaces some
>>>>>> scary code with IMO less scary code.
>>>>>>
>>>>>> Also, servers and embedded systems should probably turn off modify_ldt.
>>>>>> This makes that possible.
>>>>>>
>>>>>> Xen people, can you take a look at this?
>>>>>>
>>>>>> Willy and Kees: I left the config option alone. The -tiny people will
>>>>>> like it, and we can always add a sysctl of some sort later.
>>>>>>
>>>>>> Changes from v3:
>>>>>> - Hopefully fixed Xen.
>>>>>
>>>>>
>>>>> 32b-on-32b fails in the same manner. (but non-zero LDT is taken care of)
>>>>>
>>>>>> - Fixed 32-bit test case on 32-bit native kernel.
>>>>>
>>>>>
>>>>> I am not sure I see what changed.
>>>>
>>>> I misplaced the fix in the wrong git commit, so I failed to sent it.
>>>> Oops.
>>>>
>>>> I just sent v4.1 of patch 3. Can you try that?
>>>
>>>
>>>
>>> I am hitting BUG() in Xen code (returning from a hypercall) when freeing LDT
>>> in destroy_context(). Interestingly though when I run the test in the
>>> debugger I get SIGILL (just like before) but no BUG().
>>>
>>> Let me get back to you on that later today.
>>>
>>>
>>
>> After forward-porting my virtio patches, I got this thing to run on
>> Xen. After several tries, I got:
>>
>> [ 53.985707] ------------[ cut here ]------------
>> [ 53.986314] kernel BUG at arch/x86/xen/enlighten.c:496!
>> [ 53.986677] invalid opcode: 0000 [#1] SMP
>> [ 53.986677] Modules linked in:
>> [ 53.986677] CPU: 0 PID: 1400 Comm: bash Not tainted 4.2.0-rc4+ #4
>> [ 53.986677] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org
>> 04/01/2014
>> [ 53.986677] task: c2376180 ti: c0874000 task.ti: c0874000
>> [ 53.986677] EIP: 0061:[<c10530f2>] EFLAGS: 00010282 CPU: 0
>> [ 53.986677] EIP is at set_aliased_prot+0xb2/0xc0
>> [ 53.986677] EAX: ffffffea EBX: cc3d1000 ECX: 0672e063 EDX: 80000000
>> [ 53.986677] ESI: 00000000 EDI: 80000000 EBP: c0875e94 ESP: c0875e74
>> [ 53.986677] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0069
>> [ 53.986677] CR0: 80050033 CR2: b77404d4 CR3: 020b6000 CR4: 00042660
>> [ 53.986677] Stack:
>> [ 53.986677] 80000000 0672e063 000021c0 cc3d1000 00000001 cc3d2000
>> 00000b4a 00000200
>> [ 53.986677] c0875ea8 c105312d c2317940 c2373a80 00000000 c0875eb4
>> c1062310 c01861c0
>> [ 53.986677] c0875ec0 c1062735 c01861c0 c0875ed4 c10a764e c7007a00
>> c2373a80 00000000
>> [ 53.986677] Call Trace:
>> [ 53.986677] [<c105312d>] xen_free_ldt+0x2d/0x40
>> [ 53.986677] [<c1062310>] free_ldt_struct.part.1+0x10/0x40
>> [ 53.986677] [<c1062735>] destroy_context+0x25/0x40
>> [ 53.986677] [<c10a764e>] __mmdrop+0x1e/0xc0
>> [ 53.986677] [<c10c9858>] finish_task_switch+0xd8/0x1a0
>> [ 53.986677] [<c1863736>] __schedule+0x316/0x950
>> [ 53.986677] [<c1863d96>] schedule+0x26/0x70
>> [ 53.986677] [<c10ac613>] do_wait+0x1b3/0x200
>> [ 53.986677] [<c10ac9d7>] SyS_waitpid+0x67/0xd0
>> [ 53.986677] [<c10aa820>] ? task_stopped_code+0x50/0x50
>> [ 53.986677] [<c186717a>] syscall_call+0x7/0x7
>> [ 53.986677] Code: e8 c1 e3 0c 81 eb 00 00 00 40 39 5d ec 74 11 8b
>> 4d e4 8b 55 e0 31 f6 e8 dd e0 fa ff 85 c0 75 0d 83 c4 14 5b 5e 5f 5d
>> c3 90 0f 0b <0f> 0b 0f 0b 8d 76 00 8d bc 27 00 00 00 00 85 d2 74 31 55
>> 89 e5
>> [ 53.986677] EIP: [<c10530f2>] set_aliased_prot+0xb2/0xc0 SS:ESP 0069:c0875e74
>> [ 54.010069] ---[ end trace 89ac35b29c1c59bb ]---
>>
>> Is that the error you're seeing?
>>
>> If I change xen_free_ldt to:
>>
>> static void xen_free_ldt(struct desc_struct *ldt, unsigned entries)
>> {
>> const unsigned entries_per_page = PAGE_SIZE / LDT_ENTRY_SIZE;
>> int i;
>>
>> vm_unmap_aliases();
>> xen_mc_flush();
>>
>> for(i = 0; i < entries; i += entries_per_page)
>> set_aliased_prot(ldt + i, PAGE_KERNEL);
>> }
>>
>> then it works. I don't know why this makes a difference.
>> (xen_mc_flush makes a little bit of sense to me. vm_unmap_aliases
>> doesn't.)
>>
>
> That fix makes sense if there's some way that the vmalloc area we're
> freeing has an extra alias somewhere, which is very much possible. On
> the other hand, I don't see how this happens without first doing an
> MMUEXT_SET_LDT with an unexpectedly aliased address, and I would have
> expected that to blow up and/or result in test case failures.
>
> But I'm still confused, because it seems like Xen will never populate
> the actual (hidden) LDT mapping unless the pages backing it are
> unaliased and well-formed, which make me wonder why this stuff ever
> worked. Wouldn't LDT access with pre-existing vmalloc aliases result
> in segfaults?
>
> The semantics seem to be very odd. xen_free_ldt with an aliased
> address might fail (and OOPS), but actual access to the LDT with an
> aliased address page faults.
>
> Also, using kzalloc for everything fixes the problem, which suggests
> that there really is something to my theory that the problem involves
> unexpected aliases.

The attachment fixes the problem for me, and I could easily believe
that the attachment is correct. I'd like to know why the code appears
to work without the xen_alloc_ldt change in there, though.

--Andy
commit b4772cf849f05d5ceab079b4e88497dd1b990acd
Author: Andy Lutomirski <luto@xxxxxxxxxx>
Date: Mon Jul 27 20:20:55 2015 -0700

x86/xen: Unmap aliases in xen_alloc_ldt and xen_free_ldt

The xen_free_ldt change fixes an OOPS in the new modify_ldt
implementation. I think the xen_alloc_ldt change should be
necessary, too, but I can't seem to trigger any failures without it,
which I find surprising.

Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 0b95c9b8283f..100a2e2294af 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -32,6 +32,7 @@
#include <linux/gfp.h>
#include <linux/memblock.h>
#include <linux/edd.h>
+#include <linux/vmalloc.h>

#include <xen/xen.h>
#include <xen/events.h>
@@ -512,6 +513,10 @@ static void xen_alloc_ldt(struct desc_struct *ldt, unsigned entries)

for(i = 0; i < entries; i += entries_per_page)
set_aliased_prot(ldt + i, PAGE_KERNEL_RO);
+
+ /* If there are stray aliases, the LDT won't work. */
+ if (is_vmalloc_addr(ldt))
+ vm_unmap_aliases();
}

static void xen_free_ldt(struct desc_struct *ldt, unsigned entries)
@@ -519,6 +524,13 @@ static void xen_free_ldt(struct desc_struct *ldt, unsigned entries)
const unsigned entries_per_page = PAGE_SIZE / LDT_ENTRY_SIZE;
int i;

+ /*
+ * If there are stray aliases, hypervisor will fail the hypercalls,
+ * causing us to OOPS.
+ */
+ if (is_vmalloc_addr(ldt))
+ vm_unmap_aliases();
+
for(i = 0; i < entries; i += entries_per_page)
set_aliased_prot(ldt + i, PAGE_KERNEL);
}
diff --git a/tools/testing/selftests/x86/ldt_gdt.c b/tools/testing/selftests/x86/ldt_gdt.c
index c27adfc9ae72..fba5bc133aa2 100644
--- a/tools/testing/selftests/x86/ldt_gdt.c
+++ b/tools/testing/selftests/x86/ldt_gdt.c
@@ -204,6 +204,10 @@ static void do_simple_tests(void)
};
install_valid(&desc, AR_DPL3 | AR_TYPE_XRCODE | AR_S | AR_P | AR_DB);

+ desc.entry_number = 8191;
+ install_valid(&desc, AR_DPL3 | AR_TYPE_XRCODE | AR_S | AR_P | AR_DB);
+ desc.entry_number = 0;
+
desc.limit_in_pages = 1;
install_valid(&desc, AR_DPL3 | AR_TYPE_XRCODE |
AR_S | AR_P | AR_DB | AR_G);