linux-kernel - Re: [RFC PATCH 2/8] KVM: Add KVM_MAP_MEMORY vcpu ioctl to pre-populate guest memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 7 Mar 2024 12:45:16 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>
CC: "federico.parola@...ito.it" <federico.parola@...ito.it>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "dmatlack@...gle.com"
	<dmatlack@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "michael.roth@....com" <michael.roth@....com>,
	"seanjc@...gle.com" <seanjc@...gle.com>
Subject: Re: [RFC PATCH 2/8] KVM: Add KVM_MAP_MEMORY vcpu ioctl to
 pre-populate guest memory


>  
> +int kvm_arch_vcpu_pre_map_memory(struct kvm_vcpu *vcpu);

No explanation of why this is needed, and why it only takes @vcpu as input w/o
having the @mapping.
 
> +int kvm_arch_vcpu_map_memory(struct kvm_vcpu *vcpu,
> +			     struct kvm_memory_mapping *mapping);
> +
> 

[...]

> +static int kvm_vcpu_map_memory(struct kvm_vcpu *vcpu,
> +			       struct kvm_memory_mapping *mapping)
> +{
> +	bool added = false;
> +	int idx, r = 0;
> +
> +	if (mapping->flags & ~(KVM_MEMORY_MAPPING_FLAG_WRITE |
> +			       KVM_MEMORY_MAPPING_FLAG_EXEC |
> +			       KVM_MEMORY_MAPPING_FLAG_USER |
> +			       KVM_MEMORY_MAPPING_FLAG_PRIVATE))
> +		return -EINVAL;
> +	if ((mapping->flags & KVM_MEMORY_MAPPING_FLAG_PRIVATE) &&
> +	    !kvm_arch_has_private_mem(vcpu->kvm))
> +		return -EINVAL;
> +
> +	/* Sanity check */
> +	if (!IS_ALIGNED(mapping->source, PAGE_SIZE) ||
> +	    !mapping->nr_pages ||
> +	    mapping->base_gfn + mapping->nr_pages <= mapping->base_gfn)
> +		return -EINVAL;
> +
> +	vcpu_load(vcpu);
> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> +	r = kvm_arch_vcpu_pre_map_memory(vcpu);
> +	if (r)
> +		return r;

Returning w/o unloading the vcpu and releasing the SRCU.

> +
> +	while (mapping->nr_pages) {
> +		if (signal_pending(current)) {
> +			r = -ERESTARTSYS;
> +			break;
> +		}
> +
> +		if (need_resched())
> +			cond_resched();

need_resched() is not needed.

And normally I think we just put it at the end of the loop.

> +
> +		r = kvm_arch_vcpu_map_memory(vcpu, mapping);
> +		if (r)
> +			break;
> +
> +		added = true;
> +	}
> +
> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +	vcpu_put(vcpu);
> +
> +	if (added && mapping->nr_pages > 0)
> +		r = -EAGAIN;

Why do we need @added?

I assume the kvm_arch_vcpu_map_memory() can internally update the mapping-
>nr_pages but still return -E<WHATEVER>.  So when that happens in the first call
of kvm_arch_vcpu_map_memory(), @added won't get chance to turn to true.

> +
> +	return r;
> +}
> +
>  static long kvm_vcpu_ioctl(struct file *filp,
>  			   unsigned int ioctl, unsigned long arg)
>  {
> @@ -4620,6 +4683,17 @@ static long kvm_vcpu_ioctl(struct file *filp,
>  		r = kvm_vcpu_ioctl_get_stats_fd(vcpu);
>  		break;
>  	}
> +	case KVM_MAP_MEMORY: {
> +		struct kvm_memory_mapping mapping;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&mapping, argp, sizeof(mapping)))
> +			break;
> +		r = kvm_vcpu_map_memory(vcpu, &mapping);
> +		if (copy_to_user(argp, &mapping, sizeof(mapping)))
> +			r = -EFAULT;
> +		break;
> +	}
>  	default:
>  		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
>  	}