2020-05-30 14:38:45

by Denis Efremov (Oracle)

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

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 <[email protected]>
---
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

2020-05-30 16:02:42

by Joe Perches

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

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 <[email protected]>

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...

2020-05-30 17:31:09

by Denis Efremov (Oracle)

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

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 <[email protected]>
>
> 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...
>

2020-06-01 08:50:55

by Paolo Bonzini

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

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 <[email protected]>
>> 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

2020-06-03 10:15:55

by Denis Efremov (Oracle)

Subject: [PATCH] KVM: Use vmemdup_user()

Replace opencoded alloc and copy with vmemdup_user().

Signed-off-by: Denis Efremov <[email protected]>
---
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

2020-06-04 22:24:36

by Paolo Bonzini

Subject: Re: [PATCH] KVM: Use vmemdup_user()

On 03/06/20 12:11, Denis Efremov wrote:
> Replace opencoded alloc and copy with vmemdup_user().
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> 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

2021-06-18 03:09:33

by Jim Mattson

Subject: Re: [PATCH] KVM: Use vmemdup_user()

On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <[email protected]> wrote:
>
> Replace opencoded alloc and copy with vmemdup_user().
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> 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.

2021-06-18 08:04:08

by Michal Hocko

Subject: Re: [PATCH] KVM: Use vmemdup_user()

On Thu 17-06-21 17:25:04, Jim Mattson wrote:
> On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <[email protected]> wrote:
> >
> > Replace opencoded alloc and copy with vmemdup_user().
> >
> > Signed-off-by: Denis Efremov <[email protected]>
> > ---
> > 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

2021-06-18 19:21:34

by Jim Mattson

Subject: Re: [PATCH] KVM: Use vmemdup_user()

On Thu, Jun 17, 2021 at 11:00 PM Michal Hocko <[email protected]> wrote:
>
> On Thu 17-06-21 17:25:04, Jim Mattson wrote:
> > On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <[email protected]> wrote:
> > >
> > > Replace opencoded alloc and copy with vmemdup_user().
> > >
> > > Signed-off-by: Denis Efremov <[email protected]>
> > > ---
> > > 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?

2021-06-18 19:25:15

by Michal Hocko

Subject: Re: [PATCH] KVM: Use vmemdup_user()

On Fri 18-06-21 09:53:53, Jim Mattson wrote:
> On Thu, Jun 17, 2021 at 11:00 PM Michal Hocko <[email protected]> wrote:
> >
> > On Thu 17-06-21 17:25:04, Jim Mattson wrote:
> > > On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <[email protected]> wrote:
> > > >
> > > > Replace opencoded alloc and copy with vmemdup_user().
> > > >
> > > > Signed-off-by: Denis Efremov <[email protected]>
> > > > ---
> > > > 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

2021-06-18 19:26:42

by Paolo Bonzini

Subject: Re: [PATCH] KVM: Use vmemdup_user()

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