linux-kernel - Re: [PATCH v10 10/16] KVM: x86: Introduce KVM_GET_SHARED_PAGES

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 4 Feb 2021 10:14:37 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Ashish Kalra <Ashish.Kalra@....com>, pbonzini@...hat.com
Cc:     tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
        rkrcmar@...hat.com, joro@...tes.org, bp@...e.de, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        srutherford@...gle.com, seanjc@...gle.com,
        venu.busireddy@...cle.com, brijesh.singh@....com
Subject: Re: [PATCH v10 10/16] KVM: x86: Introduce KVM_GET_SHARED_PAGES_LIST
 ioctl

On 2/3/21 6:39 PM, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@....com>
> 
> 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;
> +}
> +