linux-kernel - Re: [PATCH v2 08/13] KVM: selftests: add SEV boot tests

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]

Date:   Tue, 21 Dec 2021 11:26:04 -0600
From:   Michael Roth <michael.roth@....com>
To:     Mingwei Zhang <mizhang@...gle.com>
CC:     <linux-kselftest@...r.kernel.org>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <x86@...nel.org>,
        Nathan Tempelman <natet@...gle.com>,
        Marc Orr <marcorr@...gle.com>,
        Steve Rutherford <srutherford@...gle.com>,
        Sean Christopherson <seanjc@...gle.com>,
        "Brijesh Singh" <brijesh.singh@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        "Varad Gautam" <varad.gautam@...e.com>,
        Shuah Khan <shuah@...nel.org>,
        "Vitaly Kuznetsov" <vkuznets@...hat.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        "Ricardo Koller" <ricarkol@...gle.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>,
        "H . Peter Anvin" <hpa@...or.com>,
        Krish Sadhukhan <krish.sadhukhan@...cle.com>,
        Peter Gonda <pgonda@...gle.com>
Subject: Re: [PATCH v2 08/13] KVM: selftests: add SEV boot tests

On Tue, Dec 21, 2021 at 09:40:36AM -0600, Michael Roth wrote:
> On Mon, Dec 20, 2021 at 01:49:15AM +0000, Mingwei Zhang wrote:
> > On Thu, Dec 16, 2021, Michael Roth wrote:
> > > +}
> > > +
> > > +static void
> > > +test_common(struct kvm_vm *vm, struct ucall *uc,
> > > +		  uint8_t *shared_buf, uint8_t *private_buf)
> > > +{
> > > +	bool success;
> > > +
> > > +	/* Initial guest check-in. */
> > > +	vcpu_run(vm, VCPU_ID);
> > > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 100);
> > > +
> > > +	/* Ensure initial private pages are intact/encrypted. */
> > > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> > > +	TEST_ASSERT(!success, "Initial guest memory not encrypted!");
> > > +
> > > +	vcpu_run(vm, VCPU_ID);
> > > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 101);
> > > +
> > > +	/* Ensure host userspace can't read newly-written encrypted data. */
> > > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
> > 
> > I am not sure if it is safe here. Since the cache coherency is not there
> > for neither SEV or SEV-ES. Reading confidential memory from host side
> > will generate cache lines that is not coherent with the guest. So might
> > be better to add clfush here?
> 
> On the guest side, the cachelines are tagged based on ASID, so in this case
> the guest would populate it's own cachelines when it writes new data to
> private buf.
> 
> On a host without SME coherency bit, there is a possibility that whatever
> data the host had previously written to private_buf with C=0/ASID=0, prior
> to the guest writing to it, might still be present in the cache, but for
> this test that's okay since the guest has purposely written new data to
> confirm that the host does not see the new data. What data the host
> *actually* sees, stale cache data vs. new reads of guest private memory
> with C=0 (e.g. ciphertext) are both okay as far as the test is concerned.
> clflush() would probably make sense here, but if failure to do so
> somehow results in the above assumptions not holding, and the test ends
> up seeing the newly-written data, we definitely want this test to fail
> loudly, so leaving out the clflush() to cover that corner case seems
> like a good idea.

Actually it might be good to check both of those cases, e.g.:

  //check private buf (possibly with stale cache for sme_coherency=0)
  clflush()
  //check private buf again (with fresh read of guest memory)

I'll take a look at that.

-Mike