Messages in this thread | ![/](/images/icornerl.gif) | | From | Paul Durrant <> | Date | Wed, 7 Feb 2024 08:53:37 +0000 | Subject | Re: [PATCH v12 11/20] KVM: xen: allow shared_info to be mapped by fixed HVA |
| |
On 07/02/2024 04:10, Sean Christopherson wrote: > On Mon, Jan 15, 2024, Paul Durrant wrote: >> @@ -638,20 +637,32 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data) >> } >> break; >> >> - case KVM_XEN_ATTR_TYPE_SHARED_INFO: { >> + case KVM_XEN_ATTR_TYPE_SHARED_INFO: >> + case KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA: { >> int idx; >> >> mutex_lock(&kvm->arch.xen.xen_lock); >> >> idx = srcu_read_lock(&kvm->srcu); >> >> - if (data->u.shared_info.gfn == KVM_XEN_INVALID_GFN) { >> - kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache); >> - r = 0; >> + if (data->type == KVM_XEN_ATTR_TYPE_SHARED_INFO) { >> + if (data->u.shared_info.gfn == KVM_XEN_INVALID_GFN) { >> + kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache); >> + r = 0; >> + } else { >> + r = kvm_gpc_activate(&kvm->arch.xen.shinfo_cache, >> + gfn_to_gpa(data->u.shared_info.gfn), >> + PAGE_SIZE); >> + } >> } else { >> - r = kvm_gpc_activate(&kvm->arch.xen.shinfo_cache, >> - gfn_to_gpa(data->u.shared_info.gfn), >> - PAGE_SIZE); >> + if (data->u.shared_info.hva == 0) { > > I know I said I don't care about the KVM Xen ABI, but I still think using '0' as > "invalid" is ridiculous. >
I don't have any massive preference; we could define a KVM_XEN_INVALID_HVA too.
> More importantly, this code needs to check that HVA is a userspace pointer. > Because __kvm_set_memory_region() performs the address checks, KVM assumes any > hva that it gets out of a memslot, i.e. from a gfn, is a safe userspace address. > > kvm_is_error_hva() will catch most addresses, but I'm pretty sure there's still > a small window where userspace could use this to write kernel memory.
Ok. Good point. I'll add some appropriate checks.
Paul
> >> + kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache); >> + r = 0; >> + } else { >> + r = kvm_gpc_activate_hva(&kvm->arch.xen.shinfo_cache, >> + data->u.shared_info.hva, >> + PAGE_SIZE); >> + }
| ![\](/images/icornerr.gif) |