linux-kernel - Re: [Xen-devel] [PATCH v2 4/9] xen/pvh: Bootstrap PVH guest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3c592662-700c-22ec-d847-9d7504d5f909@citrix.com>
Date:   Fri, 3 Feb 2017 18:05:58 +0000
From:   Andrew Cooper <andrew.cooper3@...rix.com>
To:     Juergen Gross <jgross@...e.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>
CC:     <xen-devel@...ts.xenproject.org>, <linux-kernel@...r.kernel.org>,
        <roger.pau@...rix.com>
Subject: Re: [Xen-devel] [PATCH v2 4/9] xen/pvh: Bootstrap PVH guest

On 03/02/17 16:40, Juergen Gross wrote:
> On 03/02/17 17:20, Boris Ostrovsky wrote:
>>>> +
>>>> +	__HEAD
>>>> +
>>>> +/* Entry point for PVH guests. */
>>> Could you add some comments about register conetnts at entry?
>> Reference to Xen's docs/misc/hvmlite.markdown would be sifficient?
> I think the corresponding lines should be copied to this source
> file. It is inconvenient to have to get the Xen repostory for
> this information.
>
>>>> +gdt:
>>>> +	.word	gdt_end - gdt
>>>> +	.long	_pa(gdt)
>>> This is a rather strange construct: the NULL descriptor of the
>>> GDT being used as space for lgdt operand.
>>>
>>>> +	.word	0
>>>> +	.quad	0x0000000000000000 /* NULL descriptor */
>>> And this comment is wrong: the NULL descriptor is at "gdt:".
>> I'll change it to:
>>
>> gdt:
>>         .word   gdt_end - gdt_start
>>         .long   _pa(gdt_start)
>>         .word   0
>> gdt_start:
>>         .quad   0x0000000000000000 /* NULL descriptor */
>>         .quad   0x0000000000000000 /* reserved */
> Much better. :-)
>
>> #ifdef CONFIG_X86_64
>>         .quad   0x00af9a000000ffff /* __KERNEL_CS */
>> #else
>>         .quad   0x00cf9a000000ffff /* __KERNEL_CS */
>> #endif
>>         .quad   0x00cf92000000ffff /* __KERNEL_DS */
>> gdt_end:
>>
>>
>>>> +#ifdef CONFIG_X86_64
>>>> +	.quad	0x00af9a000000ffff /* __KERNEL_CS */
>>> Mind adding comments about the semantics of those constants?
>>> Or use GDT_ENTRY() macro?
>>>
>>>> +#else
>>>> +	.quad	0x00cf9a000000ffff /* __KERNEL_CS */
>>>> +#endif
>>>> +	.quad	0x00cf92000000ffff /* __KERNEL_DS */
>>>> +gdt_end:
>>>> +
>>>> +	.bss
>>>> +	.balign 4
>>>> +early_stack:
>>>> +	.fill 16, 1, 0
>>> Is the stack size large enough? With a hypercall being executed in
>>> xen_prepare_pvh() I doubt this will be okay.
>> What do you think it should be then?
> I didn't check the disassembly, but even if it is okay right now
> the needed stack size will depend on the compiler used. I'd rather
> use a larger size (e.g. 256 bytes).
>
> Maybe its even possible to reuse initial_stack, but I haven't
> verified that.

Hypercalls in HVM guests don't use the stack at all in the hypercall
page, and while this is unlikely to ever change, we make no guarentee to
maintain this property.  (64bit PV guests use 2 words of stack in the
hypercall page.)

However, you must `call` at the hypercall page entry stub which uses 1
word, and the compiler needs to perform register scheduling for all 6
hypercall arguments which might involve spilling them to the stack.

2 words of stack doesn't seem large enough, irrespective of hypercalls.

~Andrew