| 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
| ||
|
Date: Tue, 2 Aug 2022 08:36:06 -0600
From: Peter Gonda <pgonda@...gle.com>
To: Andrew Jones <andrew.jones@...ux.dev>
Cc: kvm list <kvm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Marc Orr <marcorr@...gle.com>,
Sean Christopherson <seanjc@...gle.com>,
Michael Roth <michael.roth@....com>,
"Lendacky, Thomas" <thomas.lendacky@....com>,
Joerg Roedel <joro@...tes.org>,
Mingwei Zhang <mizhang@...gle.com>,
Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [V2 09/11] KVM: selftests: Make ucall work with encrypted guests
()
On Tue, Aug 2, 2022 at 8:26 AM Andrew Jones <andrew.jones@...ux.dev> wrote:
>
> On Tue, Aug 02, 2022 at 07:51:40AM -0600, Peter Gonda wrote:
> > On Tue, Aug 2, 2022 at 3:49 AM Andrew Jones <andrew.jones@...ux.dev> wrote:
> > >
> > > On Mon, Aug 01, 2022 at 01:11:07PM -0700, Peter Gonda wrote:
> > > > Add support for encrypted, SEV, guests in the ucall framework. If
> > > > encryption is enabled set up a pool of ucall structs in the guests'
> > > > shared memory region. This was suggested in the thread on "[RFC PATCH
> > > > 00/10] KVM: selftests: Add support for test-selectable ucall
> > > > implementations". Using a listed as suggested there doesn't work well
> > > > because the list is setup using HVAs not GVAs so use a bitmap + array
> > > > solution instead to get the same pool result.
> > > >
> > > > Suggested-by: Sean Christopherson <seanjc@...gle.com>
> > > > Signed-off-by: Peter Gonda <pgonda@...gle.com>
> > > > ---
> > > > .../selftests/kvm/include/kvm_util_base.h | 3 +
> > > > .../selftests/kvm/include/ucall_common.h | 14 +--
> > > > .../testing/selftests/kvm/lib/ucall_common.c | 112 +++++++++++++++++-
> > > > 3 files changed, 115 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > > index 8ce9e5be70a3..ad4abc6be1ab 100644
> > > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > > @@ -102,6 +102,9 @@ struct kvm_vm {
> > > > int stats_fd;
> > > > struct kvm_stats_header stats_header;
> > > > struct kvm_stats_desc *stats_desc;
> > > > +
> > > > + bool use_ucall_list;
> > >
> > > use_ucall_pool
> > >
> > > > + struct list_head ucall_list;
> > > > };
> >
> > Will do. I also need to remove this |ucall_list| member.
> >
> > > >
> > > >
> > > > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> > > > index c1bc8e33ef3f..a96220ac6024 100644
> > > > --- a/tools/testing/selftests/kvm/include/ucall_common.h
> > > > +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> > > > @@ -22,6 +22,10 @@ enum {
> > > > struct ucall {
> > > > uint64_t cmd;
> > > > uint64_t args[UCALL_MAX_ARGS];
> > > > +
> > > > + /* For encrypted guests. */
> > >
> > > Please, no 'encrypted' words in ucall files. ucalls shouldn't care about
> > > guest types. Indeed, the summary of this patch could even drop the word
> > > 'encrypted'. This patch is adding support for ucall pools, which is
> > > motivated by the need to support encrypted guests (the motivation should
> > > go in the commit message, but otherwise the patch should be ucall specific
> > > and guest type agnostic)
> > >
> > > > + uint64_t idx;
> > >
> > > We don't need 'idx' because 'hva' will always be at the
> > > idx * sizeof(struct ucall) offset of ucall_hdr->ucalls, which means
> > > we can always calculate it,
> > >
> > > static inline size_t uc_pool_idx(struct ucall *uc)
> > > {
> > > return uc->hva - ucall_hdr->ucalls;
> > > }
> >
> > Good call, I didn't think of that.
> >
> > >
> > > > + struct ucall *hva;
> > > > };
> > > >
> > > > void ucall_arch_init(struct kvm_vm *vm, void *arg);
> > > > @@ -32,15 +36,9 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
> > > > void ucall(uint64_t cmd, int nargs, ...);
> > > > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> > > >
> > > > -static inline void ucall_init(struct kvm_vm *vm, void *arg)
> > > > -{
> > > > - ucall_arch_init(vm, arg);
> > > > -}
> > > > +void ucall_init(struct kvm_vm *vm, void *arg);
> > > >
> > > > -static inline void ucall_uninit(struct kvm_vm *vm)
> > > > -{
> > > > - ucall_arch_uninit(vm);
> > > > -}
> > > > +void ucall_uninit(struct kvm_vm *vm);
> > > >
> > > > #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \
> > > > ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
> > > > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> > > > index a060252bab40..feb0173179ec 100644
> > > > --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> > > > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> > > > @@ -1,22 +1,122 @@
> > > > // SPDX-License-Identifier: GPL-2.0-only
> > > > #include "kvm_util.h"
> > > > +#include "linux/types.h"
> > > > +#include "linux/bitmap.h"
> > > > +#include "linux/atomic.h"
> > > > +
> > > > +struct ucall_header {
> > > > + DECLARE_BITMAP(in_use, KVM_MAX_VCPUS);
> > > > + struct ucall ucalls[KVM_MAX_VCPUS];
> > > > +};
> > > > +
> > > > +static bool use_ucall_list;
> > >
> > > use_ucall_pool
> > >
> > > > +static struct ucall_header *ucall_hdr;
> > >
> > > ucall_pool
> >
> > Will update naming here.
> >
> > >
> > > > +
> > > > +void ucall_init(struct kvm_vm *vm, void *arg)
> > > > +{
> > > > + struct ucall *uc;
> > > > + struct ucall_header *hdr;
> > > > + vm_vaddr_t vaddr;
> > > > + int i;
> > > > +
> > > > + use_ucall_list = vm->use_ucall_list;
> > > > + sync_global_to_guest(vm, use_ucall_list);
> > > > + if (!use_ucall_list)
> > > > + goto out;
> > > > +
> > > > + TEST_ASSERT(!ucall_hdr,
> > > > + "Only a single encrypted guest at a time for ucalls.");
> > >
> > > single VM at a time
> > >
> > > And I'd leave that on a single line. checkpatch allows up to 100 chars and
> > > I'm included to use all 110 chars of it.
> >
> > Sounds good.
> >
> > >
> > > > + vaddr = vm_vaddr_alloc_shared(vm, sizeof(*hdr), vm->page_size);
> > > > + hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr);
> > > > + memset(hdr, 0, sizeof(*hdr));
> > > > +
> > > > + for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> > > > + uc = &hdr->ucalls[i];
> > > > + uc->hva = uc;
> > > > + uc->idx = i;
> > > > + }
> > > > +
> > > > + ucall_hdr = (struct ucall_header *)vaddr;
> > > > + sync_global_to_guest(vm, ucall_hdr);
> > > > +
> > > > +out:
> > > > + ucall_arch_init(vm, arg);
> > > > +}
> > > > +
> > > > +void ucall_uninit(struct kvm_vm *vm)
> > > > +{
> > > > + use_ucall_list = false;
> > > > + ucall_hdr = NULL;
> > >
> > > It's unlikely we'd ever change the ucall setup on a running VM,
> > > but we should sync these changes to the guest for good measure.
> >
> > Hmm I'll need to have some notion of encrypted guests here then. Since
> > the guests page tables also get encrypted we can no longer get the
> > gva2gpa translations so sync_global_to_guest() cannot be done due to
> > the addr_ga2hva(). So is it OK if this call references encryption like
> > below?
> >
> > use_ucall_list = false;
> > ucall_hdr = NULL;
> > if (!vm->memencrypt.encrypted) {
> > sync_global_to_guest(vm, use_ucall_list);
> > sync_global_to_guest(vm, ucall_hdr);
> > }
>
> Yes, I guess that's the best we can do.
>
> If sync_global_to_guest() becomes useless once a guest is encrypted then
> maybe it should assert !vm->memencrypt.encrypted.
In the last patch I have edited addr_gva2gpa like below to assert.
This should cover sync_global_to_guest() and other similar cases.
static inline vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
{
+ TEST_ASSERT(!vm->memcrypt.encrypted,
+ "Encrypted guests have their page tables encrypted
so gva2* conversions are not possible.");
return addr_arch_gva2gpa(vm, gva);
}
Speaking of that though I should move that to 10/11 instead of 11/11.
>
> >
> > >
> > > > +
> > > > + ucall_arch_uninit(vm);
> > > > +}
> > > > +
> > > > +static struct ucall *ucall_alloc(void)
> > > > +{
> > > > + struct ucall *uc = NULL;
> > > > + int i;
> > > > +
> > > > + if (!use_ucall_list)
> > > > + goto out;
> > > > +
> > > > + for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> > > > + if (atomic_test_and_set_bit(i, ucall_hdr->in_use))
> > > > + continue;
> > > > +
> > > > + uc = &ucall_hdr->ucalls[i];
> > > > + }
> > >
> > > This is not what I suggested in the last revision and it's still wrong.
> > > Here, you're still looping through all of the pool and returning the last
> > > one.
> > >
> > > What I suggested was
> > >
> > > for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> > > if (!atomic_test_and_set_bit(i, ucall_hdr->in_use)) {
> > > uc = &ucall_hdr->ucalls[i];
> > > break;
> > > }
> > > }
> > >
> > > Notice how we leave the loop early when we find a free uc.
> >
> > Ah my mistake, I didn't get this fix into this series
> >
> > >
> > > We should also zero the contents of the uc buffer before returning.
> >
> > Ack will do.
> >
> > >
> > > > +
> > > > +out:
> > > > + return uc;
> > > > +}
> > > > +
> > > > +static void ucall_free(struct ucall *uc)
> > > > +{
> > > > + if (!use_ucall_list)
> > > > + return;
> > > > +
> > > > + clear_bit(uc->idx, ucall_hdr->in_use);
> > >
> > > This seems to be the one and only use of idx which is another argument for
> > > dropping the variable and just calculating it instead.
> >
> > Will do.
> >
> > >
> > > > +}
> > > > +
> > > > +static vm_vaddr_t get_ucall_addr(struct ucall *uc)
> > > > +{
> > > > + if (use_ucall_list)
> > > > + return (vm_vaddr_t)uc->hva;
> > >
> > > A comment explaining that this hva has already been synchronized
> > > with the guest would be good. Or maybe a different name for the
> > > pointer than hva, one which conveys that it's a pointer that works
> > > as both an hva and gva would be better.
> >
> > I don't think this pointer does work as a gva though since its created
> > by just taking &uc during host execution. I can add a comment that
> > mentions this is a hva is already translated for the host and not for
> > guest use other than communicating with the host. I thought 'hva' was
> > a good name here since it is literally the hva of this ucall struct,
> > what name would you prefer?
>
> My mistake, I forgot how we initialized uc->hva. 'hva' is a good name.
>
> >
> > >
> > >
> > > > +
> > > > + return (vm_vaddr_t)uc;
> > > > +}
> > > >
> > > > void ucall(uint64_t cmd, int nargs, ...)
> > > > {
> > > > - struct ucall uc = {
> > > > - .cmd = cmd,
> > > > - };
> > >
> > > This zeros all members except cmd.
> > >
> > > > + struct ucall *uc;
> > > > + struct ucall tmp;
> > >
> > > And this makes tmp full of stack garbage, so in the non uc-pool case we
> > > no longer have an equivalent uc. Please initialize tmp the same way uc
> > > was initialized.
> >
> > Ack, I'll get this fixed.
> >
> > >
> > > > va_list va;
> > > > int i;
> > > >
> > > > + uc = ucall_alloc();
> > > > + if (!uc)
> > > > + uc = &tmp;
> > > > +
> > > > + uc->cmd = cmd;
> > > > +
> > > > nargs = min(nargs, UCALL_MAX_ARGS);
> > > >
> > > > va_start(va, nargs);
> > > > for (i = 0; i < nargs; ++i)
> > > > - uc.args[i] = va_arg(va, uint64_t);
> > > > + uc->args[i] = va_arg(va, uint64_t);
> > > > va_end(va);
> > > >
> > > > - ucall_arch_do_ucall((vm_vaddr_t)&uc);
> > > > + ucall_arch_do_ucall(get_ucall_addr(uc));
> > >
> > > We don't need get_ucall_addr(). Just do the if-else right here
> > >
> > > if (use_ucall_list)
> > > ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> > > else
> > > ucall_arch_do_ucall((vm_vaddr_t)uc);
> >
> > Will do.
> >
> > >
> > > > +
> > > > + ucall_free(uc);
> > > > +}
> > > > +
> > > > +static void *get_ucall_hva(struct kvm_vm *vm, uint64_t uc)
> > > > +{
> > > > + if (vm->use_ucall_list)
> > > > + return (void *)uc;
> > > > +
> > > > + return addr_gva2hva(vm, uc);
> > > > }
> > > >
> > > > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > > > @@ -27,7 +127,7 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > > > if (!uc)
> > > > uc = &ucall;
> > > >
> > > > - addr = addr_gva2hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
> > > > + addr = get_ucall_hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
> > >
> > > Wait, I thought we wanted ucall_arch_get_ucall() to return hvas?
> > > It would make more sense if it did, since it's called from the
> > > host.
> >
> > As you noted in [V2 07/11] I updated ucall_arch_get_ucall() to return
> > the gva. I figured this was just more boilerplate which could be
> > consolidated given all archs right now just use the addr_gva2hva()
> > helper to do this translation. This change also allowed for the
> > "use_ucall_pool" to be completely contained in ucall_common.c instead
> > of spilling into each arch specific file. If there is a preference to
> > have the arch specific calls translate from gva to hva I can do that,
> > I think I'll need to have them all check if "use_ucall_pool" is set
> > though. Thoughts?
>
> I just checked what I wrote for the last version of this patch,
> "[RFC V1 08/10] KVM: selftests: Make ucall work with encrypted guests".
> It was
>
> > ...we're only updating x86's ucall_arch_get_ucall() to return gvas.
> > What about the other architectures? Anyway, I'd rather we don't
> > change ucall_arch_get_ucall() to return gvas. They should continue
> > returning hvas and any trickery needed to translate a pool uc to
> > an hva should be put inside ucall_arch_get_ucall().
>
> (It's comforting to see I was consistent, at least wrt the last review.)
>
> So, I would keep returning hvas and put the 'if use_ucall_pool' inside
> the ucall_arch_get_ucall() functions.
Makes sense. I'll have ucall_arch_get_ucall() return hvas and deal
with the use_pool trickery. Sorry for not following that for this
version I had just thought consolidating that logic would be cleaner.
Thanks for thorough review Drew!
>
> Thanks,
> drew
>
> >
> > We can still update ucall_arch_get_ucall() to return a pointer type so
> > that we can return NULL instead of 0.
> >
> > >
> > > > if (addr) {
> > > > memcpy(uc, addr, sizeof(*uc));
> > > > vcpu_run_complete_io(vcpu);
> > > > --
> > > > 2.37.1.455.g008518b4e5-goog
> > > >
> > >
> > > Thanks,
> > > drew
Powered by blists - more mailing lists