| 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
| ||
|
Message-ID: <1d020344-37e0-4386-a064-ddd0ca71d110@redhat.com>
Date: Fri, 10 Jan 2025 12:53:10 +0100
From: David Hildenbrand <david@...hat.com>
To: yangge1116@....com, pbonzini@...hat.com
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, seanjc@...gle.com,
21cnbao@...il.com, baolin.wang@...ux.alibaba.com, liuzixing@...on.cn
Subject: Re: [PATCH V2] KVM: SEV: fix wrong pinning of pages
On 10.01.25 12:42, yangge1116@....com wrote:
> From: yangge <yangge1116@....com>
>
> When pin_user_pages_fast() pins SEV guest memory without the
> FOLL_LONGTERM flag, the pages will not get migrated out of MIGRATE_CMA/
> ZONE_MOVABLE, violating these mechanisms to avoid fragmentation with
> unmovable pages, for example making CMA allocations fail.
>
> To address the aforementioned problem, we propose adding the
> FOLL_LONGTERM flag to the pin_user_pages_fast() function.
>
> Signed-off-by: yangge <yangge1116@....com>
> ---
>
> V2:
> - update code and commit message suggested by David
>
> arch/x86/kvm/svm/sev.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 943bd07..96f3b8e 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -630,6 +630,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
> unsigned long locked, lock_limit;
> struct page **pages;
> unsigned long first, last;
> + unsigned int flags = FOLL_LONGTERM;
> int ret;
>
> lockdep_assert_held(&kvm->lock);
> @@ -662,8 +663,10 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
> if (!pages)
> return ERR_PTR(-ENOMEM);
>
> + flags |= write ? FOLL_WRITE : 0;
> +
> /* Pin the user virtual address. */
> - npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : 0, pages);
> + npinned = pin_user_pages_fast(uaddr, npages, flags, pages);
> if (npinned != npages) {
> pr_err("SEV: Failure locking %lu pages.\n", npages);
> ret = -ENOMEM;
Sorry, looking into it in more detail, there are some paths that immediately unpin again,
and don't seem to have longterm semantics.
Could we do the following, so longterm pinning would be limited to the memory regions
that might stay pinned possible forever?
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 943bd074a5d37..4b0f03f0ea741 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -622,7 +622,7 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
unsigned long ulen, unsigned long *n,
- int write)
+ int flags)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
unsigned long npages, size;
@@ -663,7 +663,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
return ERR_PTR(-ENOMEM);
/* Pin the user virtual address. */
- npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : 0, pages);
+ npinned = pin_user_pages_fast(uaddr, npages, flags, pages);
if (npinned != npages) {
pr_err("SEV: Failure locking %lu pages.\n", npages);
ret = -ENOMEM;
@@ -751,7 +751,7 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
vaddr_end = vaddr + size;
/* Lock the user memory. */
- inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
+ inpages = sev_pin_memory(kvm, vaddr, size, &npages, FOLL_WRITE);
if (IS_ERR(inpages))
return PTR_ERR(inpages);
@@ -1250,7 +1250,8 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
if (IS_ERR(src_p))
return PTR_ERR(src_p);
- dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, PAGE_SIZE, &n, 1);
+ dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, PAGE_SIZE, &n,
+ FOLL_WRITE);
if (IS_ERR(dst_p)) {
sev_unpin_memory(kvm, src_p, n);
return PTR_ERR(dst_p);
@@ -1316,7 +1317,8 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
return -EFAULT;
- pages = sev_pin_memory(kvm, params.guest_uaddr, params.guest_len, &n, 1);
+ pages = sev_pin_memory(kvm, params.guest_uaddr, params.guest_len, &n,
+ FOLL_WRITE);
if (IS_ERR(pages))
return PTR_ERR(pages);
@@ -1798,7 +1800,7 @@ static int sev_receive_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
/* Pin guest memory */
guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
- PAGE_SIZE, &n, 1);
+ PAGE_SIZE, &n, FOLL_WRITE);
if (IS_ERR(guest_page)) {
ret = PTR_ERR(guest_page);
goto e_free_trans;
@@ -2696,7 +2698,8 @@ int sev_mem_enc_register_region(struct kvm *kvm,
return -ENOMEM;
mutex_lock(&kvm->lock);
- region->pages = sev_pin_memory(kvm, range->addr, range->size, ®ion->npages, 1);
+ region->pages = sev_pin_memory(kvm, range->addr, range->size, ®ion->npages,
+ FOLL_WRITE | FOLL_LONGTERM);
if (IS_ERR(region->pages)) {
ret = PTR_ERR(region->pages);
mutex_unlock(&kvm->lock);
Thoughts?
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists