linux-kernel - Re: [RFC] KVM: x86: Support KVM VMs sharing SEV context

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210305223647.GA2289@ashkalra_ubuntu_server>
Date:   Fri, 5 Mar 2021 22:36:47 +0000
From:   Ashish Kalra <ashish.kalra@....com>
To:     Steve Rutherford <srutherford@...gle.com>
Cc:     Tom Lendacky <thomas.lendacky@....com>,
        Nathan Tempelman <natet@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>, X86 ML <x86@...nel.org>,
        KVM list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Sean Christopherson <seanjc@...gle.com>,
        David Rientjes <rientjes@...gle.com>,
        Brijesh Singh <brijesh.singh@....com>
Subject: Re: [RFC] KVM: x86: Support KVM VMs sharing SEV context

On Thu, Feb 25, 2021 at 10:49:00AM -0800, Steve Rutherford wrote:
> On Thu, Feb 25, 2021 at 6:57 AM Tom Lendacky <thomas.lendacky@....com> wrote:
> > >> +int svm_vm_copy_asid_to(struct kvm *kvm, unsigned int mirror_kvm_fd)
> > >> +{
> > >> +       struct file *mirror_kvm_file;
> > >> +       struct kvm *mirror_kvm;
> > >> +       struct kvm_sev_info *mirror_kvm_sev;
> > >> +       unsigned int asid;
> > >> +       int ret;
> > >> +
> > >> +       if (!sev_guest(kvm))
> > >> +               return -ENOTTY;
> > >
> > > You definitely don't want this: this is the function that turns the vm
> > > into an SEV guest (marks SEV as active).
> >
> > The sev_guest() function does not set sev->active, it only checks it. The
> > sev_guest_init() function is where sev->active is set.
> Sorry, bad use of the english on my part: the "this" was referring to
> svm_vm_copy_asid_to. Right now, you could only pass this sev_guest
> check if you had already called sev_guest_init, which seems incorrect.
> >
> > >
> > > (Not an issue with this patch, but a broader issue) I believe
> > > sev_guest lacks the necessary acquire/release barriers on sev->active,
> >
> > The svm_mem_enc_op() takes the kvm lock and that is the only way into the
> > sev_guest_init() function where sev->active is set.
> There are a few places that check sev->active which don't have the kvm
> lock, which is not problematic if we add in a few compiler barriers
> (ala irqchip_split et al).

Probably, sev->active accesses can be made safe using READ_ONCE() &
WRITE_ONCE().

Thanks,
Ashish