linux-kernel - Re: [PATCH v6 4/4] x86/xen: Add "nopv" support for HVM 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]

Date:   Wed, 10 Jul 2019 09:21:08 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Zhenzhong Duan <zhenzhong.duan@...cle.com>,
        linux-kernel@...r.kernel.org
Cc:     xen-devel@...ts.xenproject.org, jgross@...e.com,
        sstabellini@...nel.org, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de
Subject: Re: [PATCH v6 4/4] x86/xen: Add "nopv" support for HVM guest

On 7/9/19 10:07 PM, Zhenzhong Duan wrote:
> On 2019/7/9 22:54, Boris Ostrovsky wrote:
>> On 7/9/19 12:20 AM, Zhenzhong Duan wrote:
>>> -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = {
>>> +static uint32_t __init xen_platform_hvm(void)
>>> +{
>>> +       uint32_t xen_domain = xen_cpuid_base();
>>> +       struct x86_hyper_init *h = &x86_hyper_xen_hvm.init;
>>> +
>>> +       if (xen_pv_domain())
>>> +               return 0;
>>> +
>>> +       if (xen_pvh_domain() && nopv) {
>>> +               /* Guest booting via the Xen-PVH boot entry goes
>>> here */
>>> +               pr_info("\"nopv\" parameter is ignored in PVH
>>> guest\n");
>>> +               nopv = false;
>>> +       } else if (nopv && xen_domain) {
>>> +               /*
>>> +                * Guest booting via normal boot entry (like via
>>> grub2) goes
>>> +                * here.
>>> +                *
>>> +                * Use interface functions for bare hardware if nopv,
>>> +                * xen_hvm_guest_late_init is an exception as we
>>> need to
>>> +                * detect PVH and panic there.
>>> +                */
>>> +               memcpy(h, (void *)&x86_init.hyper,
>>> sizeof(x86_init.hyper));
>>
>> And this worked? I'd think it would fail since h points to RO section.
> Yes, I have below changes in the patch.
>
> -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = {
> +struct hypervisor_x86 x86_hyper_xen_hvm __initdata = {


But hypervisors[] stays__initconst so that I thought could be a problem.
But apparently it's not.

>
>>
>>
>>> +               memcpy(&x86_hyper_xen_hvm.runtime, (void
>>> *)&x86_platform.hyper,
>>> +                      sizeof(x86_platform.hyper));
>>> +               h->guest_late_init = xen_hvm_guest_late_init;
>>
>> To me this still doesn't look right --- you are making assumptions about
>> x86_platform/x86_init.hyper and I don't think you can assume they have
>> not been set to point to another hypervisor, for example.
>
> You mean copy_array() calls in init_hypervisor_platform()? But that
> happens after
>
> detect_hypervisor_vendor() shoose out the prefered hypervisor. In
> detect stage,
>
> x86_platform/x86_init.hyper has default value for bare hardware, or I
> missed something?


Right. My point though is that this ordering is opaque to
hypervisor-specific code and can change. The same is true about making
assumptions about x86_platform/x86_init.hyper values.

-boris