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