Hi,
This is just for the SEV (Secure Encrypted Virtualization) part of KVM.
It converts the get_user_pages_fast() call, after fixing a couple of
small bugs in the vicinity.
Note that I have only compile-tested these two patches, so any run-time
testing coverage would be greatly appreciated.
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Vitaly Kuznetsov <[email protected]>
Cc: Wanpeng Li <[email protected]>
Cc: Jim Mattson <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: [email protected]
Cc: [email protected]
John Hubbard (2):
KVM: SVM: fix svn_pin_memory()'s use of get_user_pages_fast()
KVM: SVM: convert get_user_pages() --> pin_user_pages()
arch/x86/kvm/svm/sev.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
base-commit: 9cb1fd0efd195590b828b9b865421ad345a4a145
--
2.26.2
There are two problems in svn_pin_memory():
1) The return value of get_user_pages_fast() is stored in an
unsigned long, although the declared return value is of type int.
This will not cause any symptoms, but it is misleading.
Fix this by changing the type of npinned to "int".
2) The number of pages passed into get_user_pages_fast() is stored
in an unsigned long, even though get_user_pages_fast() accepts an
int. This means that it is possible to silently overflow the number
of pages.
Fix this by adding a WARN_ON_ONCE() and an early error return. The
npages variable is left as an unsigned long for convenience in
checking for overflow.
Fixes: 89c505809052 ("KVM: SVM: Add support for KVM_SEV_LAUNCH_UPDATE_DATA command")
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Vitaly Kuznetsov <[email protected]>
Cc: Wanpeng Li <[email protected]>
Cc: Jim Mattson <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Hubbard <[email protected]>
---
arch/x86/kvm/svm/sev.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 89f7f3aebd31..9693db1af57c 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -313,7 +313,8 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
int write)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- unsigned long npages, npinned, size;
+ unsigned long npages, size;
+ int npinned;
unsigned long locked, lock_limit;
struct page **pages;
unsigned long first, last;
@@ -333,6 +334,9 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
return NULL;
}
+ if (WARN_ON_ONCE(npages > INT_MAX))
+ return NULL;
+
/* Avoid using vmalloc for smaller buffers. */
size = npages * sizeof(struct page *);
if (size > PAGE_SIZE)
--
2.26.2
John Hubbard <[email protected]> writes:
> There are two problems in svn_pin_memory():
>
> 1) The return value of get_user_pages_fast() is stored in an
> unsigned long, although the declared return value is of type int.
> This will not cause any symptoms, but it is misleading.
> Fix this by changing the type of npinned to "int".
>
> 2) The number of pages passed into get_user_pages_fast() is stored
> in an unsigned long, even though get_user_pages_fast() accepts an
> int. This means that it is possible to silently overflow the number
> of pages.
>
> Fix this by adding a WARN_ON_ONCE() and an early error return. The
> npages variable is left as an unsigned long for convenience in
> checking for overflow.
>
> Fixes: 89c505809052 ("KVM: SVM: Add support for KVM_SEV_LAUNCH_UPDATE_DATA command")
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Cc: Vitaly Kuznetsov <[email protected]>
> Cc: Wanpeng Li <[email protected]>
> Cc: Jim Mattson <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: John Hubbard <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 89f7f3aebd31..9693db1af57c 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -313,7 +313,8 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
> int write)
> {
> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> - unsigned long npages, npinned, size;
> + unsigned long npages, size;
> + int npinned;
> unsigned long locked, lock_limit;
> struct page **pages;
> unsigned long first, last;
> @@ -333,6 +334,9 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
> return NULL;
> }
>
> + if (WARN_ON_ONCE(npages > INT_MAX))
> + return NULL;
> +
I bit unrelated to this patch, but callers of sev_pin_memory() treat
NULL differently:
sev_launch_secret()/svm_register_enc_region() return -ENOMEM
sev_dbg_crypt() returns -EFAULT
Should we switch to ERR_PTR() to preserve the error?
> /* Avoid using vmalloc for smaller buffers. */
> size = npages * sizeof(struct page *);
> if (size > PAGE_SIZE)
Reviewed-by: Vitaly Kuznetsov <[email protected]>
--
Vitaly
On 2020-05-26 00:33, Vitaly Kuznetsov wrote:
...
> I bit unrelated to this patch, but callers of sev_pin_memory() treat
> NULL differently:
>
> sev_launch_secret()/svm_register_enc_region() return -ENOMEM
> sev_dbg_crypt() returns -EFAULT
>
> Should we switch to ERR_PTR() to preserve the error?
>
>> /* Avoid using vmalloc for smaller buffers. */
>> size = npages * sizeof(struct page *);
>> if (size > PAGE_SIZE)
>
> Reviewed-by: Vitaly Kuznetsov <[email protected]>
>
Thanks for the review, Vitaly. If anyone is able to do any run-time
testing of this patch and also patch 2/2, then I think a maintainer
would be more willing to pick them up.
Any testing help there is greatly appreciated!
thanks,
--
John Hubbard
NVIDIA