[PATCH] KVM: Use previously computed array_size()

[PATCH] KVM: Use previously computed array_size()

[Denis Efremov]

array_size() is used in alloc calls to compute the allocation
size. Next, "raw" multiplication is used to compute the size
for copy_from_user(). The patch removes duplicated computation
by saving the size in a var. No security concerns, just a small
optimization.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 arch/x86/kvm/cpuid.c | 9 ++++-----
 virt/kvm/kvm_main.c  | 8 ++++----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 901cd1fdecd9..3363b7531af1 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -184,14 +184,13 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 		goto out;
 	r = -ENOMEM;
 	if (cpuid->nent) {
-		cpuid_entries =
-			vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
-					   cpuid->nent));
+		const size_t size = array_size(sizeof(struct kvm_cpuid_entry),
+					       cpuid->nent);
+		cpuid_entries = vmalloc(size);
 		if (!cpuid_entries)
 			goto out;
 		r = -EFAULT;
-		if (copy_from_user(cpuid_entries, entries,
-				   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
+		if (copy_from_user(cpuid_entries, entries, size))
 			goto out;
 	}
 	for (i = 0; i < cpuid->nent; i++) {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 731c1e517716..001e1929e01c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3722,15 +3722,15 @@ static long kvm_vm_ioctl(struct file *filp,
 		if (routing.flags)
 			goto out;
 		if (routing.nr) {
+			const size_t size = array_size(sizeof(*entries),
+						       routing.nr);
 			r = -ENOMEM;
-			entries = vmalloc(array_size(sizeof(*entries),
-						     routing.nr));
+			entries = vmalloc(size);
 			if (!entries)
 				goto out;
 			r = -EFAULT;
 			urouting = argp;
-			if (copy_from_user(entries, urouting->entries,
-					   routing.nr * sizeof(*entries)))
+			if (copy_from_user(entries, urouting->entries, size))
 				goto out_free_irq_routing;
 		}
 		r = kvm_set_irq_routing(kvm, entries, routing.nr,
-- 
2.26.2

Re: [PATCH] KVM: Use previously computed array_size()

[Joe Perches]

On Sat, 2020-05-30 at 17:35 +0300, Denis Efremov wrote:
> array_size() is used in alloc calls to compute the allocation
> size. Next, "raw" multiplication is used to compute the size
> for copy_from_user(). The patch removes duplicated computation
> by saving the size in a var. No security concerns, just a small
> optimization.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>

Perhaps use vmemdup_user?

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
[]
> @@ -184,14 +184,13 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  		goto out;
>  	r = -ENOMEM;
>  	if (cpuid->nent) {
> -		cpuid_entries =
> -			vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
> -					   cpuid->nent));
> +		const size_t size = array_size(sizeof(struct kvm_cpuid_entry),
> +					       cpuid->nent);
> +		cpuid_entries = vmalloc(size);
>  		if (!cpuid_entries)
>  			goto out;
>  		r = -EFAULT;
> -		if (copy_from_user(cpuid_entries, entries,
> -				   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
> +		if (copy_from_user(cpuid_entries, entries, size))

		cpuid_entries = vmemdup_user(entries,
					     array_size(sizeof(struct kvm_cpuid_entry), cpuid->nent));
		if (IS_ERR(cpuid_entries))
			...

etc...

Re: [PATCH] KVM: Use previously computed array_size()

[Denis Efremov]

On 5/30/20 6:58 PM, Joe Perches wrote:
> On Sat, 2020-05-30 at 17:35 +0300, Denis Efremov wrote:
>> array_size() is used in alloc calls to compute the allocation
>> size. Next, "raw" multiplication is used to compute the size
>> for copy_from_user(). The patch removes duplicated computation
>> by saving the size in a var. No security concerns, just a small
>> optimization.
>>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
> 
> Perhaps use vmemdup_user?

vmemdup_user() uses kvmalloc internally. I think it will also require
changing vfree to kvfree.

> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> []
>> @@ -184,14 +184,13 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>  		goto out;
>>  	r = -ENOMEM;
>>  	if (cpuid->nent) {
>> -		cpuid_entries =
>> -			vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
>> -					   cpuid->nent));
>> +		const size_t size = array_size(sizeof(struct kvm_cpuid_entry),
>> +					       cpuid->nent);
>> +		cpuid_entries = vmalloc(size);
>>  		if (!cpuid_entries)
>>  			goto out;
>>  		r = -EFAULT;
>> -		if (copy_from_user(cpuid_entries, entries,
>> -				   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
>> +		if (copy_from_user(cpuid_entries, entries, size))
> 
> 		cpuid_entries = vmemdup_user(entries,
> 					     array_size(sizeof(struct kvm_cpuid_entry), cpuid->nent));
> 		if (IS_ERR(cpuid_entries))
> 			...
> 
> etc...
> 

Re: [PATCH] KVM: Use previously computed array_size()

[Paolo Bonzini]

On 30/05/20 19:28, Denis Efremov wrote:
>> On Sat, 2020-05-30 at 17:35 +0300, Denis Efremov wrote:
>>> array_size() is used in alloc calls to compute the allocation
>>> size. Next, "raw" multiplication is used to compute the size
>>> for copy_from_user(). The patch removes duplicated computation
>>> by saving the size in a var. No security concerns, just a small
>>> optimization.
>>>
>>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> Perhaps use vmemdup_user?
> vmemdup_user() uses kvmalloc internally. I think it will also require
> changing vfree to kvfree.
> 

Yes, it would be a good idea.

Paolo

[PATCH] KVM: Use vmemdup_user()

[Denis Efremov]

Replace opencoded alloc and copy with vmemdup_user().

Signed-off-by: Denis Efremov <efremov@linux.com>
---
Looks like these are the only places in KVM that are suitable for
vmemdup_user().

 arch/x86/kvm/cpuid.c | 17 +++++++----------
 virt/kvm/kvm_main.c  | 19 ++++++++-----------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 901cd1fdecd9..27438a2bdb62 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	r = -E2BIG;
 	if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
 		goto out;
-	r = -ENOMEM;
 	if (cpuid->nent) {
-		cpuid_entries =
-			vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
-					   cpuid->nent));
-		if (!cpuid_entries)
-			goto out;
-		r = -EFAULT;
-		if (copy_from_user(cpuid_entries, entries,
-				   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
+		cpuid_entries = vmemdup_user(entries,
+					     array_size(sizeof(struct kvm_cpuid_entry),
+							cpuid->nent));
+		if (IS_ERR(cpuid_entries)) {
+			r = PTR_ERR(cpuid_entries);
 			goto out;
+		}
 	}
 	for (i = 0; i < cpuid->nent; i++) {
 		vcpu->arch.cpuid_entries[i].function = cpuid_entries[i].function;
@@ -212,8 +209,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	kvm_x86_ops.cpuid_update(vcpu);
 	r = kvm_update_cpuid(vcpu);
 
+	kvfree(cpuid_entries);
 out:
-	vfree(cpuid_entries);
 	return r;
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 731c1e517716..46a3743e95ff 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3722,21 +3722,18 @@ static long kvm_vm_ioctl(struct file *filp,
 		if (routing.flags)
 			goto out;
 		if (routing.nr) {
-			r = -ENOMEM;
-			entries = vmalloc(array_size(sizeof(*entries),
-						     routing.nr));
-			if (!entries)
-				goto out;
-			r = -EFAULT;
 			urouting = argp;
-			if (copy_from_user(entries, urouting->entries,
-					   routing.nr * sizeof(*entries)))
-				goto out_free_irq_routing;
+			entries = vmemdup_user(urouting->entries,
+					       array_size(sizeof(*entries),
+							  routing.nr));
+			if (IS_ERR(entries)) {
+				r = PTR_ERR(entries);
+				goto out;
+			}
 		}
 		r = kvm_set_irq_routing(kvm, entries, routing.nr,
 					routing.flags);
-out_free_irq_routing:
-		vfree(entries);
+		kvfree(entries);
 		break;
 	}
 #endif /* CONFIG_HAVE_KVM_IRQ_ROUTING */
-- 
2.26.2

Re: [PATCH] KVM: Use vmemdup_user()

[Paolo Bonzini]

On 03/06/20 12:11, Denis Efremov wrote:
> Replace opencoded alloc and copy with vmemdup_user().
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> Looks like these are the only places in KVM that are suitable for
> vmemdup_user().
> 
>  arch/x86/kvm/cpuid.c | 17 +++++++----------
>  virt/kvm/kvm_main.c  | 19 ++++++++-----------
>  2 files changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 901cd1fdecd9..27438a2bdb62 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  	r = -E2BIG;
>  	if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
>  		goto out;
> -	r = -ENOMEM;
>  	if (cpuid->nent) {
> -		cpuid_entries =
> -			vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
> -					   cpuid->nent));
> -		if (!cpuid_entries)
> -			goto out;
> -		r = -EFAULT;
> -		if (copy_from_user(cpuid_entries, entries,
> -				   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
> +		cpuid_entries = vmemdup_user(entries,
> +					     array_size(sizeof(struct kvm_cpuid_entry),
> +							cpuid->nent));
> +		if (IS_ERR(cpuid_entries)) {
> +			r = PTR_ERR(cpuid_entries);
>  			goto out;
> +		}
>  	}
>  	for (i = 0; i < cpuid->nent; i++) {
>  		vcpu->arch.cpuid_entries[i].function = cpuid_entries[i].function;
> @@ -212,8 +209,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  	kvm_x86_ops.cpuid_update(vcpu);
>  	r = kvm_update_cpuid(vcpu);
>  
> +	kvfree(cpuid_entries);
>  out:
> -	vfree(cpuid_entries);
>  	return r;
>  }
>  
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 731c1e517716..46a3743e95ff 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3722,21 +3722,18 @@ static long kvm_vm_ioctl(struct file *filp,
>  		if (routing.flags)
>  			goto out;
>  		if (routing.nr) {
> -			r = -ENOMEM;
> -			entries = vmalloc(array_size(sizeof(*entries),
> -						     routing.nr));
> -			if (!entries)
> -				goto out;
> -			r = -EFAULT;
>  			urouting = argp;
> -			if (copy_from_user(entries, urouting->entries,
> -					   routing.nr * sizeof(*entries)))
> -				goto out_free_irq_routing;
> +			entries = vmemdup_user(urouting->entries,
> +					       array_size(sizeof(*entries),
> +							  routing.nr));
> +			if (IS_ERR(entries)) {
> +				r = PTR_ERR(entries);
> +				goto out;
> +			}
>  		}
>  		r = kvm_set_irq_routing(kvm, entries, routing.nr,
>  					routing.flags);
> -out_free_irq_routing:
> -		vfree(entries);
> +		kvfree(entries);
>  		break;
>  	}
>  #endif /* CONFIG_HAVE_KVM_IRQ_ROUTING */
> 

Queued, thanks.

Paolo

Re: [PATCH] KVM: Use vmemdup_user()

[Jim Mattson]

On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote:
>
> Replace opencoded alloc and copy with vmemdup_user().
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> Looks like these are the only places in KVM that are suitable for
> vmemdup_user().
>
>  arch/x86/kvm/cpuid.c | 17 +++++++----------
>  virt/kvm/kvm_main.c  | 19 ++++++++-----------
>  2 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 901cd1fdecd9..27438a2bdb62 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>         r = -E2BIG;
>         if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
>                 goto out;
> -       r = -ENOMEM;
>         if (cpuid->nent) {
> -               cpuid_entries =
> -                       vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
> -                                          cpuid->nent));
> -               if (!cpuid_entries)
> -                       goto out;
> -               r = -EFAULT;
> -               if (copy_from_user(cpuid_entries, entries,
> -                                  cpuid->nent * sizeof(struct kvm_cpuid_entry)))
> +               cpuid_entries = vmemdup_user(entries,
> +                                            array_size(sizeof(struct kvm_cpuid_entry),
> +                                                       cpuid->nent));

Does this break memcg accounting? I ask, because I'm really not sure.

Re: [PATCH] KVM: Use vmemdup_user()

[Michal Hocko]

On Thu 17-06-21 17:25:04, Jim Mattson wrote:
> On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote:
> >
> > Replace opencoded alloc and copy with vmemdup_user().
> >
> > Signed-off-by: Denis Efremov <efremov@linux.com>
> > ---
> > Looks like these are the only places in KVM that are suitable for
> > vmemdup_user().
> >
> >  arch/x86/kvm/cpuid.c | 17 +++++++----------
> >  virt/kvm/kvm_main.c  | 19 ++++++++-----------
> >  2 files changed, 15 insertions(+), 21 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 901cd1fdecd9..27438a2bdb62 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> >         r = -E2BIG;
> >         if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
> >                 goto out;
> > -       r = -ENOMEM;
> >         if (cpuid->nent) {
> > -               cpuid_entries =
> > -                       vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
> > -                                          cpuid->nent));
> > -               if (!cpuid_entries)
> > -                       goto out;
> > -               r = -EFAULT;
> > -               if (copy_from_user(cpuid_entries, entries,
> > -                                  cpuid->nent * sizeof(struct kvm_cpuid_entry)))
> > +               cpuid_entries = vmemdup_user(entries,
> > +                                            array_size(sizeof(struct kvm_cpuid_entry),
> > +                                                       cpuid->nent));
> 
> Does this break memcg accounting? I ask, because I'm really not sure.

What do you mean by that? The original code uses plain vmalloc so the
allocation is not memcg accounted (please note that __GFP_ACCOUNT needs
to be specified explicitly). vmemdup_user is the same in that regards.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] KVM: Use vmemdup_user()

[Jim Mattson]

On Thu, Jun 17, 2021 at 11:00 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 17-06-21 17:25:04, Jim Mattson wrote:
> > On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote:
> > >
> > > Replace opencoded alloc and copy with vmemdup_user().
> > >
> > > Signed-off-by: Denis Efremov <efremov@linux.com>
> > > ---
> > > Looks like these are the only places in KVM that are suitable for
> > > vmemdup_user().
> > >
> > >  arch/x86/kvm/cpuid.c | 17 +++++++----------
> > >  virt/kvm/kvm_main.c  | 19 ++++++++-----------
> > >  2 files changed, 15 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 901cd1fdecd9..27438a2bdb62 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> > >         r = -E2BIG;
> > >         if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
> > >                 goto out;
> > > -       r = -ENOMEM;
> > >         if (cpuid->nent) {
> > > -               cpuid_entries =
> > > -                       vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
> > > -                                          cpuid->nent));
> > > -               if (!cpuid_entries)
> > > -                       goto out;
> > > -               r = -EFAULT;
> > > -               if (copy_from_user(cpuid_entries, entries,
> > > -                                  cpuid->nent * sizeof(struct kvm_cpuid_entry)))
> > > +               cpuid_entries = vmemdup_user(entries,
> > > +                                            array_size(sizeof(struct kvm_cpuid_entry),
> > > +                                                       cpuid->nent));
> >
> > Does this break memcg accounting? I ask, because I'm really not sure.
>
> What do you mean by that? The original code uses plain vmalloc so the
> allocation is not memcg accounted (please note that __GFP_ACCOUNT needs
> to be specified explicitly). vmemdup_user is the same in that regards.

I asked, because I wasn't sure if plain vmalloc was accounted or not.

In any case, these allocations *should* be accounted, shouldn't they?

Re: [PATCH] KVM: Use vmemdup_user()

[Michal Hocko]

On Fri 18-06-21 09:53:53, Jim Mattson wrote:
> On Thu, Jun 17, 2021 at 11:00 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Thu 17-06-21 17:25:04, Jim Mattson wrote:
> > > On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote:
> > > >
> > > > Replace opencoded alloc and copy with vmemdup_user().
> > > >
> > > > Signed-off-by: Denis Efremov <efremov@linux.com>
> > > > ---
> > > > Looks like these are the only places in KVM that are suitable for
> > > > vmemdup_user().
> > > >
> > > >  arch/x86/kvm/cpuid.c | 17 +++++++----------
> > > >  virt/kvm/kvm_main.c  | 19 ++++++++-----------
> > > >  2 files changed, 15 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > > index 901cd1fdecd9..27438a2bdb62 100644
> > > > --- a/arch/x86/kvm/cpuid.c
> > > > +++ b/arch/x86/kvm/cpuid.c
> > > > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> > > >         r = -E2BIG;
> > > >         if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
> > > >                 goto out;
> > > > -       r = -ENOMEM;
> > > >         if (cpuid->nent) {
> > > > -               cpuid_entries =
> > > > -                       vmalloc(array_size(sizeof(struct kvm_cpuid_entry),
> > > > -                                          cpuid->nent));
> > > > -               if (!cpuid_entries)
> > > > -                       goto out;
> > > > -               r = -EFAULT;
> > > > -               if (copy_from_user(cpuid_entries, entries,
> > > > -                                  cpuid->nent * sizeof(struct kvm_cpuid_entry)))
> > > > +               cpuid_entries = vmemdup_user(entries,
> > > > +                                            array_size(sizeof(struct kvm_cpuid_entry),
> > > > +                                                       cpuid->nent));
> > >
> > > Does this break memcg accounting? I ask, because I'm really not sure.
> >
> > What do you mean by that? The original code uses plain vmalloc so the
> > allocation is not memcg accounted (please note that __GFP_ACCOUNT needs
> > to be specified explicitly). vmemdup_user is the same in that regards.
> 
> I asked, because I wasn't sure if plain vmalloc was accounted or not.
> 
> In any case, these allocations *should* be accounted, shouldn't they?

This is more of a question to maintainers. Are these objects easy to
request by userspace without any bounds?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] KVM: Use vmemdup_user()

[Paolo Bonzini]

On 18/06/21 19:04, Michal Hocko wrote:
> On Fri 18-06-21 09:53:53, Jim Mattson wrote:
>> In any case, these allocations *should* be accounted, shouldn't they?
> 
> This is more of a question to maintainers. Are these objects easy to
> request by userspace without any bounds?

This particular one need not be accounted because the allocation only 
lasts for the duration of the ioctl. The allocation below in 
kvm_vcpu_ioctl_set_cpuid

      e2 = kvmalloc_array(cpuid->nent, sizeof(*e2), GFP_KERNEL_ACCOUNT);

is long term and is already accounted for.

kvm_vcpu_ioctl_set_cpuid2 should also use kvmalloc_array and 
GFP_KERNEL_ACCOUNT.  However, it wasn't doing so before this patch went 
in, either.

Paolo