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); + }
