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