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
