linux-kernel - Re: [V4 6/8] KVM: selftests: add library for creating/interacting with SEV guests

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: <CAMkAt6rP7KbgUqmK+aiooSLfvRrMsRmp99cL0YWKBwpOJZc82A@mail.gmail.com>
Date:   Thu, 27 Oct 2022 10:24:58 -0600
From:   Peter Gonda <pgonda@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        marcorr@...gle.com, michael.roth@....com, thomas.lendacky@....com,
        joro@...tes.org, mizhang@...gle.com, pbonzini@...hat.com,
        andrew.jones@...ux.dev
Subject: Re: [V4 6/8] KVM: selftests: add library for creating/interacting
 with SEV guests

On Wed, Oct 19, 2022 at 10:34 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Tue, Oct 18, 2022, Peter Gonda wrote:
> > On Mon, Oct 17, 2022 at 2:34 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > >
> > > On Mon, Oct 17, 2022, Peter Gonda wrote:
> > > > I think this means we don't need to add VM_MODE_PXXV48_4K_SEV since we
> > > > can set up the c-bit from inside of vm_sev_create_*(), thoughts?
> > >
> > > Configuring the C-bit inside vm_sev_create_*() won't work (at least not well).
> > > The C-bit needs to be known before kvm_vm_elf_load(), i.e. can't be handled after
> > > __vm_create(), and needs to be tracked inside the VM, i.e. can't be handled before
> > > __vm_create().
> > >
> > > The proposed kvm_init_vm_address_properties() seems like the best fit since the
> > > C-bit (and TDX's S-bit) is stolen from GPA space, i.e. directly affects the other
> > > values computed in that path.
> > >
> > > As for the kvm_vm_arch allocation ugliness, when we talked off-list I didn't
> > > consider the need to allocate in kvm_init_vm_address_properties().  That's quite
> > > gross, especially since the pointer will be larger than the thing being allocated.
> > >
> > > With that in mind, adding .../include/<arch>/kvm_util.h so that "struct kvm_vm_arch"
> > > can be defined and referenced directly doesn't seem so bad.  Having to stub in the
> > > struct for the other architectures is annoying, but not the end of the world.
> >
> > I'll make "struct kvm_vm_arch" a non pointer member, so adding
> > /include/<arch>/kvm_util.h files.
> >
> > But I think we do not need VM_MODE_PXXV48_4K_SEV, see:
>
> I really don't want to open code __vm_create() with a slight tweak.  E.g. the
> below code will be broken by Ricardo's series to add memslot0 is moved out of
> ____vm_create()[1], and kinda sorta be broken again by Vishal's series to add an
> arch hook to __vm_create()[2].
>
> AFAICT, there is no requirement that KVM_SEV_INIT be called before computing the
> C-Bit, the only requirement is that KVM_SEV_INIT is called before adding vCPUs.
>
> [1] https://lore.kernel.org/all/20221017195834.2295901-8-ricarkol@google.com
> [2] https://lore.kernel.org/all/YzsC4ibDqGh5qaP9@google.com

Oh I misunderstood your suggestion above.

I should make KVM_SEV_INIT happen from kvm_arch_vm_post_create().  Add
VM_MODE_PXXV48_4K_SEV for c-bit setting inside of
kvm_init_vm_address_properties().

Inside of vm_sev_create_with_one_vcpu() I use
__vm_create_with_vcpus(), then call KVM_SEV_LAUNCH_FINISH.

Is that correct?



>
> > struct kvm_vm *vm_sev_create_with_one_vcpu(uint32_t policy, void *guest_code,
> >                                            struct kvm_vcpu **cpu)
> > {
> >         enum vm_guest_mode mode = VM_MODE_PXXV48_4K;
> >         uint64_t nr_pages = vm_nr_pages_required(mode, 1, 0);
> >         struct kvm_vm *vm;
> >         uint8_t measurement[512];
> >         int i;
> >
> >         vm = ____vm_create(mode, nr_pages);
> >
> >         kvm_sev_ioctl(vm, KVM_SEV_INIT, NULL);
> >
> >         configure_sev_pte_masks(vm);
> >
> >         *cpu = vm_vcpu_add(vm, 0, guest_code);
> >         kvm_vm_elf_load(vm, program_invocation_name);
> >
> >         sev_vm_launch(vm, policy);
> >
> >         /* Dump the initial measurement. A test to actually verify it
> > would be nice. */
> >         sev_vm_launch_measure(vm, measurement);
> >         pr_info("guest measurement: ");
> >         for (i = 0; i < 32; ++i)
> >                 pr_info("%02x", measurement[i]);
> >         pr_info("\n");
> >
> >         sev_vm_launch_finish(vm);
> >
> >         return vm;
> > }