| 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
| ||
|
Message-ID: <20210407140044.GA25499@ashkalra_ubuntu_server>
Date: Wed, 7 Apr 2021 14:01:01 +0000
From: Ashish Kalra <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com,
hpa@...or.com, joro@...tes.org, bp@...e.de,
thomas.lendacky@....com, x86@...nel.org, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, srutherford@...gle.com,
venu.busireddy@...cle.com, brijesh.singh@....com, will@...nel.org,
maz@...nel.org, qperret@...gle.com
Subject: Re: [PATCH v11 08/13] KVM: X86: Introduce KVM_HC_PAGE_ENC_STATUS
hypercall
On Tue, Apr 06, 2021 at 03:48:20PM +0000, Sean Christopherson wrote:
> On Mon, Apr 05, 2021, Ashish Kalra wrote:
> > From: Ashish Kalra <ashish.kalra@....com>
>
> ...
>
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 3768819693e5..78284ebbbee7 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1352,6 +1352,8 @@ struct kvm_x86_ops {
> > int (*complete_emulated_msr)(struct kvm_vcpu *vcpu, int err);
> >
> > void (*vcpu_deliver_sipi_vector)(struct kvm_vcpu *vcpu, u8 vector);
> > + int (*page_enc_status_hc)(struct kvm_vcpu *vcpu, unsigned long gpa,
> > + unsigned long sz, unsigned long mode);
> > };
> >
> > struct kvm_x86_nested_ops {
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index c9795a22e502..fb3a315e5827 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -1544,6 +1544,67 @@ static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > return ret;
> > }
> >
> > +static int sev_complete_userspace_page_enc_status_hc(struct kvm_vcpu *vcpu)
> > +{
> > + vcpu->run->exit_reason = 0;
> > + kvm_rax_write(vcpu, vcpu->run->dma_sharing.ret);
> > + ++vcpu->stat.hypercalls;
> > + return kvm_skip_emulated_instruction(vcpu);
> > +}
> > +
> > +int svm_page_enc_status_hc(struct kvm_vcpu *vcpu, unsigned long gpa,
> > + unsigned long npages, unsigned long enc)
> > +{
> > + kvm_pfn_t pfn_start, pfn_end;
> > + struct kvm *kvm = vcpu->kvm;
> > + gfn_t gfn_start, gfn_end;
> > +
> > + if (!sev_guest(kvm))
> > + return -EINVAL;
> > +
> > + if (!npages)
> > + return 0;
>
> Parth of me thinks passing a zero size should be an error not a nop. Either way
> works, just feels a bit weird to allow this to be a nop.
>
> > +
> > + gfn_start = gpa_to_gfn(gpa);
>
> This should check that @gpa is aligned.
>
> > + gfn_end = gfn_start + npages;
> > +
> > + /* out of bound access error check */
> > + if (gfn_end <= gfn_start)
> > + return -EINVAL;
> > +
> > + /* lets make sure that gpa exist in our memslot */
> > + pfn_start = gfn_to_pfn(kvm, gfn_start);
> > + pfn_end = gfn_to_pfn(kvm, gfn_end);
> > +
> > + if (is_error_noslot_pfn(pfn_start) && !is_noslot_pfn(pfn_start)) {
> > + /*
> > + * Allow guest MMIO range(s) to be added
> > + * to the shared pages list.
> > + */
> > + return -EINVAL;
> > + }
> > +
> > + if (is_error_noslot_pfn(pfn_end) && !is_noslot_pfn(pfn_end)) {
> > + /*
> > + * Allow guest MMIO range(s) to be added
> > + * to the shared pages list.
> > + */
> > + return -EINVAL;
> > + }
>
> I don't think KVM should do any checks beyond gfn_end <= gfn_start. Just punt
> to userspace and give userspace full say over what is/isn't legal.
>
> > +
> > + if (enc)
> > + vcpu->run->exit_reason = KVM_EXIT_DMA_UNSHARE;
> > + else
> > + vcpu->run->exit_reason = KVM_EXIT_DMA_SHARE;
>
> Use a single exit and pass "enc" via kvm_run. I also strongly dislike "DMA",
> there's no guarantee the guest is sharing memory for DMA.
>
> I think we can usurp KVM_EXIT_HYPERCALL for this? E.g.
>
> vcpu->run->exit_reason = KVM_EXIT_HYPERCALL;
> vcpu->run->hypercall.nr = KVM_HC_PAGE_ENC_STATUS;
> vcpu->run->hypercall.args[0] = gfn_start << PAGE_SHIFT;
> vcpu->run->hypercall.args[1] = npages * PAGE_SIZE;
> vcpu->run->hypercall.args[2] = enc;
> vcpu->run->hypercall.longmode = is_64_bit_mode(vcpu);
>
> > +
> > + vcpu->run->dma_sharing.addr = gfn_start;
>
> Addresses and pfns are not the same thing. If you're passing the size in bytes,
> then it's probably best to pass the gpa, not the gfn. Same for the params from
> the guest, they should be in the same "domain".
>
> > + vcpu->run->dma_sharing.len = npages * PAGE_SIZE;
> > + vcpu->arch.complete_userspace_io =
> > + sev_complete_userspace_page_enc_status_hc;
>
> I vote to drop the "userspace" part, it's already quite verbose.
>
> vcpu->arch.complete_userspace_io = sev_complete_page_enc_status_hc;
>
> > +
> > + return 0;
> > +}
> > +
>
> ..
>
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index f7d12fca397b..ef5c77d59651 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -8273,6 +8273,18 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > kvm_sched_yield(vcpu->kvm, a0);
> > ret = 0;
> > break;
> > + case KVM_HC_PAGE_ENC_STATUS: {
> > + int r;
> > +
> > + ret = -KVM_ENOSYS;
> > + if (kvm_x86_ops.page_enc_status_hc) {
> > + r = kvm_x86_ops.page_enc_status_hc(vcpu, a0, a1, a2);
>
> Use static_call().
>
> > + if (r >= 0)
> > + return r;
> > + ret = r;
> > + }
>
> Hmm, an alternative to adding a kvm_x86_ops hook would be to tag the VM as
> supporting/allowing the hypercall. That would clean up this code, ensure VMX
> and SVM don't end up creating a different userspace ABI, and make it easier to
> reuse the hypercall in the future (I'm still hopeful :-) ). E.g.
>
> case KVM_HC_PAGE_ENC_STATUS: {
> u64 gpa = a0, nr_bytes = a1;
>
> if (!vcpu->kvm->arch.page_enc_hc_enable)
> break;
>
> if (!PAGE_ALIGNED(gpa) || !PAGE_ALIGNED(nr_bytes) ||
> !nr_bytes || gpa + nr_bytes <= gpa)) {
> ret = -EINVAL;
> break;
> }
>
> vcpu->run->exit_reason = KVM_EXIT_HYPERCALL;
> vcpu->run->hypercall.nr = KVM_HC_PAGE_ENC_STATUS;
> vcpu->run->hypercall.args[0] = gpa;
> vcpu->run->hypercall.args[1] = nr_bytes;
> vcpu->run->hypercall.args[2] = enc;
> vcpu->run->hypercall.longmode = op_64_bit;
> vcpu->arch.complete_userspace_io = complete_page_enc_hc;
> return 0;
> }
I really like the above approach, but one thing that concerns me above
is the addition of architecture specific code, for example, the page
encryption stuff as part of the generic x86 KVM code ?
Especially bits like the page enc hypercall completion callback and
testing the architecture specific page encryption hypercall enabled
flag, probably the hypercall completion callback can be named
something as complete_userspace_hypercall(), as anyway what it does
inside the callback is all very generic.
The above naming may also go well with KVM_EXIT_HYPERCALL exitcode
interface.
Thanks,
Ashish
>
>
> > + break;
> > + }
> > default:
> > ret = -KVM_ENOSYS;
> > break;
Powered by blists - more mailing lists