Re: [PATCH v12 11/20] KVM: xen: allow shared_info to be mapped by fixed HVA

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

• 	}