linux-kernel - Re: [PATCH] KVM: SVM: use vmsave/vmload for saving/restoring additional host state

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:   Thu, 10 Dec 2020 11:23:19 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Michael Roth <michael.roth@....com>
Cc:     kvm list <kvm@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        X86 ML <x86@...nel.org>, "H . Peter Anvin" <hpa@...or.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [PATCH] KVM: SVM: use vmsave/vmload for saving/restoring
 additional host state

On Thu, Dec 10, 2020 at 9:52 AM Michael Roth <michael.roth@....com> wrote:
>   MSR_STAR, MSR_LSTAR, MSR_CSTAR,
>   MSR_SYSCALL_MASK, MSR_KERNEL_GS_BASE,
>   MSR_IA32_SYSENTER_CS,
>   MSR_IA32_SYSENTER_ESP,
>   MSR_IA32_SYSENTER_EIP,
>   MSR_FS_BASE, MSR_GS_BASE

Can you get rid of all the old FS/GS manipulation at the same time?

> +       for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++) {
> +               rdmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
> +       }
> +
> +       asm volatile(__ex("vmsave")
> +                    : : "a" (page_to_pfn(sd->save_area) << PAGE_SHIFT)
> +                    : "memory");
> +       /*
> +        * Host FS/GS segment registers might be restored soon after
> +        * vmexit, prior to vmload of host save area. Even though this
> +        * state is now saved in the host's save area, we cannot use
> +        * per-cpu accesses until these registers are restored, so we
> +        * store a copy in the VCPU struct to make sure they are
> +        * accessible.
> +        */
>  #ifdef CONFIG_X86_64
> -       rdmsrl(MSR_GS_BASE, to_svm(vcpu)->host.gs_base);
> +       svm->host.gs_base = hostsa->gs.base;
>  #endif

For example, this comment makes no sense to me.  Just let VMLOAD
restore FS/GS and be done with it.  Don't copy those gs_base and
gs.base fields -- just delete them please.  (Or are they needed for
nested virt for some reason?  If so, please document that.)

> -       savesegment(fs, svm->host.fs);
> -       savesegment(gs, svm->host.gs);
> -       svm->host.ldt = kvm_read_ldt();
> -
> -       for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
> -               rdmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
> +       svm->host.fs = hostsa->fs.selector;
> +       svm->host.gs = hostsa->gs.selector;

This too.  Why is the host code thinking about selectors at all?

> -       kvm_load_ldt(svm->host.ldt);

I have a patch that deletes this, too.  Don't worry about the conflict
-- I'll sort it out.

> @@ -120,7 +115,6 @@ struct vcpu_svm {
>         struct {
>                 u16 fs;
>                 u16 gs;
> -               u16 ldt;
>                 u64 gs_base;
>         } host;

Shouldn't you be about to delete fs, gs, and gs_base too?