Re: [PATCH v10 10/16] KVM: x86: Introduce KVM_GET_SHARED_PAGES_LIST ioctl

[Tom Lendacky]

On 2/3/21 6:39 PM, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@xxxxxxx>
>
> The ioctl is used to retrieve a guest's shared pages list.

...

>   
> +int svm_get_shared_pages_list(struct kvm *kvm,
> +			      struct kvm_shared_pages_list *list)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct shared_region_array_entry *array;
> +	struct shared_region *pos;
> +	int ret, nents = 0;
> +	unsigned long sz;
> +
> +	if (!sev_guest(kvm))
> +		return -ENOTTY;
> +
> +	if (!list->size)
> +		return -EINVAL;
> +
> +	if (!sev->shared_pages_list_count)
> +		return put_user(0, list->pnents);
> +
> +	sz = sev->shared_pages_list_count * sizeof(struct shared_region_array_entry);
> +	if (sz > list->size)
> +		return -E2BIG;
> +
> +	array = kmalloc(sz, GFP_KERNEL);
> +	if (!array)
> +		return -ENOMEM;
> +
> +	mutex_lock(&kvm->lock);

I think this lock needs to be taken before the memory size is calculated. 
If the list is expanded after obtaining the size and before taking the 
lock, you will run off the end of the array.

Thanks,
Tom

> +	list_for_each_entry(pos, &sev->shared_pages_list, list) {
> +		array[nents].gfn_start = pos->gfn_start;
> +		array[nents++].gfn_end = pos->gfn_end;
> +	}
> +	mutex_unlock(&kvm->lock);
> +
> +	ret = -EFAULT;
> +	if (copy_to_user(list->buffer, array, sz))
> +		goto out;
> +	if (put_user(nents, list->pnents))
> +		goto out;
> +	ret = 0;
> +out:
> +	kfree(array);
> +	return ret;
> +}
> +