Add a new helper, kvm_put_kvm_no_destroy(), to handle putting a borrowed
reference[*] to the VM when installing a new file descriptor fails. KVM
expects the refcount to remain valid in this case, as the in-progress
ioctl() has an explicit reference to the VM. The primary motiviation
for the helper is to document that the 'kvm' pointer is still valid
after putting the borrowed reference, e.g. to document that doing
mutex(&kvm->lock) immediately after putting a ref to kvm isn't broken.
[*] When exposing a new object to userspace via a file descriptor, e.g.
a new vcpu, KVM grabs a reference to itself (the VM) prior to making
the object visible to userspace to avoid prematurely freeing the VM
in the scenario where userspace immediately closes file descriptor.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
arch/powerpc/kvm/book3s_64_vio.c | 2 +-
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 16 ++++++++++++++--
4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 9a75f0e1933b..68678e31c84c 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -2000,7 +2000,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct kvm_get_htab_fd *ghf)
ret = anon_inode_getfd("kvm-htab", &kvm_htab_fops, ctx, rwflag | O_CLOEXEC);
if (ret < 0) {
kfree(ctx);
- kvm_put_kvm(kvm);
+ kvm_put_kvm_no_destroy(kvm);
return ret;
}
diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
index 5834db0a54c6..883a66e76638 100644
--- a/arch/powerpc/kvm/book3s_64_vio.c
+++ b/arch/powerpc/kvm/book3s_64_vio.c
@@ -317,7 +317,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
if (ret >= 0)
list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
else
- kvm_put_kvm(kvm);
+ kvm_put_kvm_no_destroy(kvm);
mutex_unlock(&kvm->lock);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 719fc3e15ea4..90a2102605ef 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -622,6 +622,7 @@ void kvm_exit(void);
void kvm_get_kvm(struct kvm *kvm);
void kvm_put_kvm(struct kvm *kvm);
+void kvm_put_kvm_no_destroy(struct kvm *kvm);
static inline struct kvm_memslots *__kvm_memslots(struct kvm *kvm, int as_id)
{
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 67ef3f2e19e8..b8534c6b8cf6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -772,6 +772,18 @@ void kvm_put_kvm(struct kvm *kvm)
}
EXPORT_SYMBOL_GPL(kvm_put_kvm);
+/*
+ * Used to put a reference that was taken on behalf of an object associated
+ * with a user-visible file descriptor, e.g. a vcpu or device, if installation
+ * of the new file descriptor fails and the reference cannot be transferred to
+ * its final owner. In such cases, the caller is still actively using @kvm and
+ * will fail miserably if the refcount unexpectedly hits zero.
+ */
+void kvm_put_kvm_no_destroy(struct kvm *kvm)
+{
+ WARN_ON(refcount_dec_and_test(&kvm->users_count));
+}
+EXPORT_SYMBOL_GPL(kvm_put_kvm_no_destroy);
static int kvm_vm_release(struct inode *inode, struct file *filp)
{
@@ -2679,7 +2691,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
kvm_get_kvm(kvm);
r = create_vcpu_fd(vcpu);
if (r < 0) {
- kvm_put_kvm(kvm);
+ kvm_put_kvm_no_destroy(kvm);
goto unlock_vcpu_destroy;
}
@@ -3117,7 +3129,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
kvm_get_kvm(kvm);
ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC);
if (ret < 0) {
- kvm_put_kvm(kvm);
+ kvm_put_kvm_no_destroy(kvm);
mutex_lock(&kvm->lock);
list_del(&dev->vm_node);
mutex_unlock(&kvm->lock);
--
2.22.0
On 22/10/19 00:58, Sean Christopherson wrote:
> Add a new helper, kvm_put_kvm_no_destroy(), to handle putting a borrowed
> reference[*] to the VM when installing a new file descriptor fails. KVM
> expects the refcount to remain valid in this case, as the in-progress
> ioctl() has an explicit reference to the VM. The primary motiviation
> for the helper is to document that the 'kvm' pointer is still valid
> after putting the borrowed reference, e.g. to document that doing
> mutex(&kvm->lock) immediately after putting a ref to kvm isn't broken.
>
> [*] When exposing a new object to userspace via a file descriptor, e.g.
> a new vcpu, KVM grabs a reference to itself (the VM) prior to making
> the object visible to userspace to avoid prematurely freeing the VM
> in the scenario where userspace immediately closes file descriptor.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
> arch/powerpc/kvm/book3s_64_vio.c | 2 +-
> include/linux/kvm_host.h | 1 +
> virt/kvm/kvm_main.c | 16 ++++++++++++++--
> 4 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 9a75f0e1933b..68678e31c84c 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -2000,7 +2000,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct kvm_get_htab_fd *ghf)
> ret = anon_inode_getfd("kvm-htab", &kvm_htab_fops, ctx, rwflag | O_CLOEXEC);
> if (ret < 0) {
> kfree(ctx);
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
> return ret;
> }
>
> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> index 5834db0a54c6..883a66e76638 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -317,7 +317,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> if (ret >= 0)
> list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
> else
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
>
> mutex_unlock(&kvm->lock);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 719fc3e15ea4..90a2102605ef 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -622,6 +622,7 @@ void kvm_exit(void);
>
> void kvm_get_kvm(struct kvm *kvm);
> void kvm_put_kvm(struct kvm *kvm);
> +void kvm_put_kvm_no_destroy(struct kvm *kvm);
>
> static inline struct kvm_memslots *__kvm_memslots(struct kvm *kvm, int as_id)
> {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 67ef3f2e19e8..b8534c6b8cf6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -772,6 +772,18 @@ void kvm_put_kvm(struct kvm *kvm)
> }
> EXPORT_SYMBOL_GPL(kvm_put_kvm);
>
> +/*
> + * Used to put a reference that was taken on behalf of an object associated
> + * with a user-visible file descriptor, e.g. a vcpu or device, if installation
> + * of the new file descriptor fails and the reference cannot be transferred to
> + * its final owner. In such cases, the caller is still actively using @kvm and
> + * will fail miserably if the refcount unexpectedly hits zero.
> + */
> +void kvm_put_kvm_no_destroy(struct kvm *kvm)
> +{
> + WARN_ON(refcount_dec_and_test(&kvm->users_count));
> +}
> +EXPORT_SYMBOL_GPL(kvm_put_kvm_no_destroy);
>
> static int kvm_vm_release(struct inode *inode, struct file *filp)
> {
> @@ -2679,7 +2691,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> kvm_get_kvm(kvm);
> r = create_vcpu_fd(vcpu);
> if (r < 0) {
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
> goto unlock_vcpu_destroy;
> }
>
> @@ -3117,7 +3129,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
> kvm_get_kvm(kvm);
> ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC);
> if (ret < 0) {
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
> mutex_lock(&kvm->lock);
> list_del(&dev->vm_node);
> mutex_unlock(&kvm->lock);
>
Queued, thanks.
Paolo
On Tue, Nov 26, 2019 at 01:44:14PM -0300, Leonardo Bras wrote:
> On Mon, 2019-10-21 at 15:58 -0700, Sean Christopherson wrote:
...
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 67ef3f2e19e8..b8534c6b8cf6 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -772,6 +772,18 @@ void kvm_put_kvm(struct kvm *kvm)
> > }
> > EXPORT_SYMBOL_GPL(kvm_put_kvm);
> >
> > +/*
> > + * Used to put a reference that was taken on behalf of an object associated
> > + * with a user-visible file descriptor, e.g. a vcpu or device, if installation
> > + * of the new file descriptor fails and the reference cannot be transferred to
> > + * its final owner. In such cases, the caller is still actively using @kvm and
> > + * will fail miserably if the refcount unexpectedly hits zero.
> > + */
> > +void kvm_put_kvm_no_destroy(struct kvm *kvm)
> > +{
> > + WARN_ON(refcount_dec_and_test(&kvm->users_count));
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_put_kvm_no_destroy);
> >
> > static int kvm_vm_release(struct inode *inode, struct file *filp)
> > {
> > @@ -2679,7 +2691,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm
> > *kvm, u32 id)
> > kvm_get_kvm(kvm);
> > r = create_vcpu_fd(vcpu);
> > if (r < 0) {
> > - kvm_put_kvm(kvm);
> > + kvm_put_kvm_no_destroy(kvm);
> > goto unlock_vcpu_destroy;
> > }
> >
> > @@ -3117,7 +3129,7 @@ static int kvm_ioctl_create_device(struct kvm
> > *kvm,
> > kvm_get_kvm(kvm);
> > ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR
> > | O_CLOEXEC);
> > if (ret < 0) {
> > - kvm_put_kvm(kvm);
> > + kvm_put_kvm_no_destroy(kvm);
> > mutex_lock(&kvm->lock);
> > list_del(&dev->vm_node);
> > mutex_unlock(&kvm->lock);
>
> Hello,
>
> I see what are you solving here, but would not this behavior cause the
> refcount to reach negative values?
>
> If so, is not there a problem? I mean, in some archs (powerpc included)
> refcount_dec_and_test() will decrement and then test if the value is
> equal 0. If we ever reach a negative value, this will cause that memory
> to never be released.
>
> An example is that refcount_dec_and_test(), on other archs than x86,
> will call atomic_dec_and_test(), which on include/linux/atomic-
> fallback.h will do:
>
> return atomic_dec_return(v) == 0;
>
> To change this behavior, it would mean change the whole atomic_*_test
> behavior, or do a copy function in order to change this '== 0' to
> '<= 0'.
>
> Does it make sense? Do you need any help on this?
I don't think so. refcount_dec_and_test() will WARN on an underflow when
the kernel is built with CONFIG_REFCOUNT_FULL=y. I see no value in
duplicating those sanity checks in KVM.
This new helper and WARN is to explicitly catch @users_count unexpectedly
hitting zero, which is orthogonal to an underflow (although odds are good
that a bug that triggers the WARN in kvm_put_kvm_no_destroy() will also
lead to an underflow). Leaking the memory is deliberate as the alternative
is a guaranteed use-after-free, i.e. kvm_put_kvm_no_destroy() is intended
to be used when users_count is guaranteed to be valid after it is
decremented.
On Mon, 2019-10-21 at 15:58 -0700, Sean Christopherson wrote:
> Add a new helper, kvm_put_kvm_no_destroy(), to handle putting a
> borrowed
> reference[*] to the VM when installing a new file descriptor
> fails. KVM
> expects the refcount to remain valid in this case, as the in-progress
> ioctl() has an explicit reference to the VM. The primary motiviation
> for the helper is to document that the 'kvm' pointer is still valid
> after putting the borrowed reference, e.g. to document that doing
> mutex(&kvm->lock) immediately after putting a ref to kvm isn't
> broken.
>
> [*] When exposing a new object to userspace via a file descriptor,
> e.g.
> a new vcpu, KVM grabs a reference to itself (the VM) prior to
> making
> the object visible to userspace to avoid prematurely freeing the
> VM
> in the scenario where userspace immediately closes file
> descriptor.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
> arch/powerpc/kvm/book3s_64_vio.c | 2 +-
> include/linux/kvm_host.h | 1 +
> virt/kvm/kvm_main.c | 16 ++++++++++++++--
> 4 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 9a75f0e1933b..68678e31c84c 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -2000,7 +2000,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm,
> struct kvm_get_htab_fd *ghf)
> ret = anon_inode_getfd("kvm-htab", &kvm_htab_fops, ctx, rwflag
> | O_CLOEXEC);
> if (ret < 0) {
> kfree(ctx);
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
> return ret;
> }
>
> diff --git a/arch/powerpc/kvm/book3s_64_vio.c
> b/arch/powerpc/kvm/book3s_64_vio.c
> index 5834db0a54c6..883a66e76638 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -317,7 +317,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm
> *kvm,
> if (ret >= 0)
> list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
> else
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
>
> mutex_unlock(&kvm->lock);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 719fc3e15ea4..90a2102605ef 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -622,6 +622,7 @@ void kvm_exit(void);
>
> void kvm_get_kvm(struct kvm *kvm);
> void kvm_put_kvm(struct kvm *kvm);
> +void kvm_put_kvm_no_destroy(struct kvm *kvm);
>
> static inline struct kvm_memslots *__kvm_memslots(struct kvm *kvm,
> int as_id)
> {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 67ef3f2e19e8..b8534c6b8cf6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -772,6 +772,18 @@ void kvm_put_kvm(struct kvm *kvm)
> }
> EXPORT_SYMBOL_GPL(kvm_put_kvm);
>
> +/*
> + * Used to put a reference that was taken on behalf of an object
> associated
> + * with a user-visible file descriptor, e.g. a vcpu or device, if
> installation
> + * of the new file descriptor fails and the reference cannot be
> transferred to
> + * its final owner. In such cases, the caller is still actively
> using @kvm and
> + * will fail miserably if the refcount unexpectedly hits zero.
> + */
> +void kvm_put_kvm_no_destroy(struct kvm *kvm)
> +{
> + WARN_ON(refcount_dec_and_test(&kvm->users_count));
> +}
> +EXPORT_SYMBOL_GPL(kvm_put_kvm_no_destroy);
>
> static int kvm_vm_release(struct inode *inode, struct file *filp)
> {
> @@ -2679,7 +2691,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm
> *kvm, u32 id)
> kvm_get_kvm(kvm);
> r = create_vcpu_fd(vcpu);
> if (r < 0) {
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
> goto unlock_vcpu_destroy;
> }
>
> @@ -3117,7 +3129,7 @@ static int kvm_ioctl_create_device(struct kvm
> *kvm,
> kvm_get_kvm(kvm);
> ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR
> | O_CLOEXEC);
> if (ret < 0) {
> - kvm_put_kvm(kvm);
> + kvm_put_kvm_no_destroy(kvm);
> mutex_lock(&kvm->lock);
> list_del(&dev->vm_node);
> mutex_unlock(&kvm->lock);
Hello,
I see what are you solving here, but would not this behavior cause the
refcount to reach negative values?
If so, is not there a problem? I mean, in some archs (powerpc included)
refcount_dec_and_test() will decrement and then test if the value is
equal 0. If we ever reach a negative value, this will cause that memory
to never be released.
An example is that refcount_dec_and_test(), on other archs than x86,
will call atomic_dec_and_test(), which on include/linux/atomic-
fallback.h will do:
return atomic_dec_return(v) == 0;
To change this behavior, it would mean change the whole atomic_*_test
behavior, or do a copy function in order to change this '== 0' to
'<= 0'.
Does it make sense? Do you need any help on this?
Kind regards,
Leonardo Brás
On Tue, 2019-11-26 at 09:14 -0800, Sean Christopherson wrote:
> On Tue, Nov 26, 2019 at 01:44:14PM -0300, Leonardo Bras wrote:
> > On Mon, 2019-10-21 at 15:58 -0700, Sean Christopherson wrote:
>
> ...
>
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 67ef3f2e19e8..b8534c6b8cf6 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -772,6 +772,18 @@ void kvm_put_kvm(struct kvm *kvm)
> > > }
> > > EXPORT_SYMBOL_GPL(kvm_put_kvm);
> > >
> > > +/*
> > > + * Used to put a reference that was taken on behalf of an object
> > > associated
> > > + * with a user-visible file descriptor, e.g. a vcpu or device,
> > > if installation
> > > + * of the new file descriptor fails and the reference cannot be
> > > transferred to
> > > + * its final owner. In such cases, the caller is still actively
> > > using @kvm and
> > > + * will fail miserably if the refcount unexpectedly hits zero.
> > > + */
> > > +void kvm_put_kvm_no_destroy(struct kvm *kvm)
> > > +{
> > > + WARN_ON(refcount_dec_and_test(&kvm->users_count));
> > > +}
> > > +EXPORT_SYMBOL_GPL(kvm_put_kvm_no_destroy);
> > >
> > > static int kvm_vm_release(struct inode *inode, struct file
> > > *filp)
> > > {
> > > @@ -2679,7 +2691,7 @@ static int kvm_vm_ioctl_create_vcpu(struct
> > > kvm
> > > *kvm, u32 id)
> > > kvm_get_kvm(kvm);
> > > r = create_vcpu_fd(vcpu);
> > > if (r < 0) {
> > > - kvm_put_kvm(kvm);
> > > + kvm_put_kvm_no_destroy(kvm);
> > > goto unlock_vcpu_destroy;
> > > }
> > >
> > > @@ -3117,7 +3129,7 @@ static int kvm_ioctl_create_device(struct
> > > kvm
> > > *kvm,
> > > kvm_get_kvm(kvm);
> > > ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR
> > > > O_CLOEXEC);
> > > if (ret < 0) {
> > > - kvm_put_kvm(kvm);
> > > + kvm_put_kvm_no_destroy(kvm);
> > > mutex_lock(&kvm->lock);
> > > list_del(&dev->vm_node);
> > > mutex_unlock(&kvm->lock);
> >
> > Hello,
> >
> > I see what are you solving here, but would not this behavior cause
> > the
> > refcount to reach negative values?
> >
> > If so, is not there a problem? I mean, in some archs (powerpc
> > included)
> > refcount_dec_and_test() will decrement and then test if the value
> > is
> > equal 0. If we ever reach a negative value, this will cause that
> > memory
> > to never be released.
> >
> > An example is that refcount_dec_and_test(), on other archs than
> > x86,
> > will call atomic_dec_and_test(), which on include/linux/atomic-
> > fallback.h will do:
> >
> > return atomic_dec_return(v) == 0;
> >
> > To change this behavior, it would mean change the whole
> > atomic_*_test
> > behavior, or do a copy function in order to change this '== 0' to
> > '<= 0'.
> >
> > Does it make sense? Do you need any help on this?
>
> I don't think so. refcount_dec_and_test() will WARN on an underflow
> when
> the kernel is built with CONFIG_REFCOUNT_FULL=y. I see no value in
> duplicating those sanity checks in KVM.
>
> This new helper and WARN is to explicitly catch @users_count
> unexpectedly
> hitting zero, which is orthogonal to an underflow (although odds are
> good
> that a bug that triggers the WARN in kvm_put_kvm_no_destroy() will
> also
> lead to an underflow). Leaking the memory is deliberate as the
> alternative
> is a guaranteed use-after-free, i.e. kvm_put_kvm_no_destroy() is
> intended
> to be used when users_count is guaranteed to be valid after it is
> decremented.
I agree an use-after-free more problem than a memory leak, but I think
that there is a way to solve this without leaking the memory also.
One option would be reordering the kvm_put_kvm(), like in this patch:
https://lkml.org/lkml/2019/11/26/517
And the other would be creating a new atomic operation that checks if
the counter is less than zero:
atomic_dec_and_test_negative(atomic_t *v)
{
return atomic_dec_return(v) <= 0;
}
And apply it to generic refcount.
Do you think that would work?
Best regards,
Leonardo Bras
On Tue, 2019-11-26 at 09:14 -0800, Sean Christopherson wrote:
> On Tue, Nov 26, 2019 at 01:44:14PM -0300, Leonardo Bras wrote:
> > On Mon, 2019-10-21 at 15:58 -0700, Sean Christopherson wrote:
>
> ...
>
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 67ef3f2e19e8..b8534c6b8cf6 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -772,6 +772,18 @@ void kvm_put_kvm(struct kvm *kvm)
> > > }
> > > EXPORT_SYMBOL_GPL(kvm_put_kvm);
> > >
> > > +/*
> > > + * Used to put a reference that was taken on behalf of an object
> > > associated
> > > + * with a user-visible file descriptor, e.g. a vcpu or device,
> > > if installation
> > > + * of the new file descriptor fails and the reference cannot be
> > > transferred to
> > > + * its final owner. In such cases, the caller is still actively
> > > using @kvm and
> > > + * will fail miserably if the refcount unexpectedly hits zero.
> > > + */
> > > +void kvm_put_kvm_no_destroy(struct kvm *kvm)
> > > +{
> > > + WARN_ON(refcount_dec_and_test(&kvm->users_count));
> > > +}
> > > +EXPORT_SYMBOL_GPL(kvm_put_kvm_no_destroy);
> > >
> > > static int kvm_vm_release(struct inode *inode, struct file
> > > *filp)
> > > {
> > > @@ -2679,7 +2691,7 @@ static int kvm_vm_ioctl_create_vcpu(struct
> > > kvm
> > > *kvm, u32 id)
> > > kvm_get_kvm(kvm);
> > > r = create_vcpu_fd(vcpu);
> > > if (r < 0) {
> > > - kvm_put_kvm(kvm);
> > > + kvm_put_kvm_no_destroy(kvm);
> > > goto unlock_vcpu_destroy;
> > > }
> > >
> > > @@ -3117,7 +3129,7 @@ static int kvm_ioctl_create_device(struct
> > > kvm
> > > *kvm,
> > > kvm_get_kvm(kvm);
> > > ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR
> > > > O_CLOEXEC);
> > > if (ret < 0) {
> > > - kvm_put_kvm(kvm);
> > > + kvm_put_kvm_no_destroy(kvm);
> > > mutex_lock(&kvm->lock);
> > > list_del(&dev->vm_node);
> > > mutex_unlock(&kvm->lock);
> >
> > Hello,
> >
> > I see what are you solving here, but would not this behavior cause
> > the
> > refcount to reach negative values?
> >
> > If so, is not there a problem? I mean, in some archs (powerpc
> > included)
> > refcount_dec_and_test() will decrement and then test if the value
> > is
> > equal 0. If we ever reach a negative value, this will cause that
> > memory
> > to never be released.
> >
> > An example is that refcount_dec_and_test(), on other archs than
> > x86,
> > will call atomic_dec_and_test(), which on include/linux/atomic-
> > fallback.h will do:
> >
> > return atomic_dec_return(v) == 0;
> >
> > To change this behavior, it would mean change the whole
> > atomic_*_test
> > behavior, or do a copy function in order to change this '== 0' to
> > '<= 0'.
> >
> > Does it make sense? Do you need any help on this?
>
> I don't think so. refcount_dec_and_test() will WARN on an underflow
> when
> the kernel is built with CONFIG_REFCOUNT_FULL=y. I see no value in
> duplicating those sanity checks in KVM.
>
> This new helper and WARN is to explicitly catch @users_count
> unexpectedly
> hitting zero, which is orthogonal to an underflow (although odds are
> good
> that a bug that triggers the WARN in kvm_put_kvm_no_destroy() will
> also
> lead to an underflow). Leaking the memory is deliberate as the
> alternative
> is a guaranteed use-after-free, i.e. kvm_put_kvm_no_destroy() is
> intended
> to be used when users_count is guaranteed to be valid after it is
> decremented.
I agree an use-after-free more problem than a memory leak, but I think
that there is a way to solve this without leaking the memory also.
One option would be reordering the kvm_put_kvm(), like in this patch:
https://lkml.org/lkml/2019/11/26/517
And the other would be creating a new atomic operation that checks if
the counter is less than zero:
atomic_dec_and_test_negative(atomic_t *v)
{
return atomic_dec_return(v) <= 0;
}
And apply it to generic refcount.
Do you think that would work?
Best regards,
Leonardo Bras
On 26/11/19 18:53, Leonardo Bras wrote:
>
> I agree an use-after-free more problem than a memory leak, but I think
> that there is a way to solve this without leaking the memory also.
>
> One option would be reordering the kvm_put_kvm(), like in this patch:
> https://lkml.org/lkml/2019/11/26/517
It's a tradeoff between "fix one bug" and "mitigate all bugs of that
class", both are good things to do. Reordering the kvm_put_kvm() fixes
the bug. kvm_put_kvm_no_destroy() makes all bugs of that kind less
severe, but it doesn't try to fix them.
Paolo
On Wed, 2019-11-27 at 17:38 +0100, Paolo Bonzini wrote:
> On 26/11/19 18:53, Leonardo Bras wrote:
> > I agree an use-after-free more problem than a memory leak, but I
> > think
> > that there is a way to solve this without leaking the memory also.
> >
> > One option would be reordering the kvm_put_kvm(), like in this
> > patch:
> > https://lkml.org/lkml/2019/11/26/517
>
> It's a tradeoff between "fix one bug" and "mitigate all bugs of that
> class", both are good things to do. Reordering the kvm_put_kvm()
> fixes
> the bug. kvm_put_kvm_no_destroy() makes all bugs of that kind less
> severe, but it doesn't try to fix them.
>
> Paolo
>
I think I understand it better now, thanks Paolo and Sean.
By what I could undestand up to now, these functions that use borrowed
references can only be called while the reference (file descriptor)
exists.
So, suppose these threads, where:
- T1 uses a borrowed reference, and
- T2 is releasing the reference (close, release):
T1 | T2
kvm_get_kvm() |
... | kvm_put_kvm()
kvm_put_kvm_no_destroy() |
The above would not trigger a use-after-free bug, but will cause a
memory leak.
Is my above understanding right?
Best regards,
Leonardo
On 27/11/19 19:24, Leonardo Bras wrote:
> By what I could undestand up to now, these functions that use borrowed
> references can only be called while the reference (file descriptor)
> exists.
> So, suppose these threads, where:
> - T1 uses a borrowed reference, and
> - T2 is releasing the reference (close, release):
Nit: T2 is releasing the *last* reference (as implied by your reference
to close/release).
>
> T1 | T2
> kvm_get_kvm() |
> ... | kvm_put_kvm()
> kvm_put_kvm_no_destroy() |
>
> The above would not trigger a use-after-free bug, but will cause a
> memory leak. Is my above understanding right?
Yes, this is correct.
Paolo
On Wed, 2019-11-27 at 19:32 +0100, Paolo Bonzini wrote:
> On 27/11/19 19:24, Leonardo Bras wrote:
> > By what I could undestand up to now, these functions that use borrowed
> > references can only be called while the reference (file descriptor)
> > exists.
> > So, suppose these threads, where:
> > - T1 uses a borrowed reference, and
> > - T2 is releasing the reference (close, release):
>
> Nit: T2 is releasing the *last* reference (as implied by your reference
> to close/release).
Correct.
>
> > T1 | T2
> > kvm_get_kvm() |
> > ... | kvm_put_kvm()
> > kvm_put_kvm_no_destroy() |
> >
> > The above would not trigger a use-after-free bug, but will cause a
> > memory leak. Is my above understanding right?
>
> Yes, this is correct.
>
Then, what would not be a bug before (using kvm_put_kvm()) now is a
memory leak (using kvm_put_kvm_no_destroy()).
And it's the price to avoid use-after-free on other cases, which is a
worse bug. Ok, I get it.
> Paolo
On Tue, 2019-11-26 at 10:14 -0800, Sean Christopherson wrote:
> If one these kvm_put_kvm() calls did unexpectedly free @kvm (due to
> a bug somewhere else), KVM would still hit a use-after-free scenario
> as the caller still thinks @kvm is valid. Currently, this would
> only happen on a subsequent ioctl() on the caller's file descriptor
> (which holds a pointer to @kvm), as the callers of these functions
> don't directly dereference @kvm after the functions return. But,
> not deferencing @kvm isn't deliberate or functionally required, it's
> just how the code happens to be written.
So, testing if the kvm reference is valid before running ioctl would be
enough to avoid these bugs? Is it possible?
Humm, but if it frees kvm before running ->release(), would it mean the
VM is destroyed incorrectly, and will probably crash?
Thanks for the patience,
Leonardo
On Wed, Nov 27, 2019 at 04:25:55PM -0300, Leonardo Bras wrote:
> On Wed, 2019-11-27 at 19:32 +0100, Paolo Bonzini wrote:
> > On 27/11/19 19:24, Leonardo Bras wrote:
> > > By what I could undestand up to now, these functions that use borrowed
> > > references can only be called while the reference (file descriptor)
> > > exists.
> > > So, suppose these threads, where:
> > > - T1 uses a borrowed reference, and
> > > - T2 is releasing the reference (close, release):
> >
> > Nit: T2 is releasing the *last* reference (as implied by your reference
> > to close/release).
>
> Correct.
>
> >
> > > T1 | T2
> > > kvm_get_kvm() |
> > > ... | kvm_put_kvm()
> > > kvm_put_kvm_no_destroy() |
> > >
> > > The above would not trigger a use-after-free bug, but will cause a
> > > memory leak. Is my above understanding right?
> >
> > Yes, this is correct.
> >
>
> Then, what would not be a bug before (using kvm_put_kvm()) now is a
> memory leak (using kvm_put_kvm_no_destroy()).
No, using kvm_put_kvm_no_destroy() changes how a bug would manifest, as
you note below. Replacing kvm_put_kvm() with kvm_put_kvm_no_destroy()
when the refcount is _guaranteed_ to be >1 has no impact on correctness.
> And it's the price to avoid use-after-free on other cases, which is a
> worse bug. Ok, I get it.
>
> > Paolo
>
> On Tue, 2019-11-26 at 10:14 -0800, Sean Christopherson wrote:
> > If one these kvm_put_kvm() calls did unexpectedly free @kvm (due to
> > a bug somewhere else), KVM would still hit a use-after-free scenario
> > as the caller still thinks @kvm is valid. Currently, this would
> > only happen on a subsequent ioctl() on the caller's file descriptor
> > (which holds a pointer to @kvm), as the callers of these functions
> > don't directly dereference @kvm after the functions return. But,
> > not deferencing @kvm isn't deliberate or functionally required, it's
> > just how the code happens to be written.
>
> So, testing if the kvm reference is valid before running ioctl would be
> enough to avoid these bugs?
No, the only way to avoid use-after-free bugs of this nature is to not
screw up the refcounting :-) This funky "borrowed reference" pattern is
not very common. It's necessary here because KVM needs to take an extra
reference to itself on behalf of the child device before installing the
child's file descriptor, because once the fd is installed it can be
closed by userspace and free the child's reference. The error path,
which uses kvm_put_kvm_no_destroy(), is used if and only if installing
the fd fails, in which case the extra reference is deliberately thrown
away.
kvm_put_kvm_no_destroy() is asserting "N > 0" as a way to detect a
refcounting bug that wouldn't be detected (until later) by the normal
refcounting behavior, which asserts "N >= 0".
> Is it possible?
No. Similar to above, userspace gets a fd by doing open("/dev/kvm"), and
the semantics of KVM are such that each fd is a reference to KVM. From
userspace's perspective, having a valid fd *is* how it knows that it has
a valid KVM reference.
> Humm, but if it frees kvm before running ->release(), would it mean the
> VM is destroyed incorrectly, and will probably crash?
More than likely the host will crash due to corrupting memory. The guest
will crash too, but that's a secondary concern.
On Wed, 2019-11-27 at 11:47 -0800, Sean Christopherson wrote:
> On Wed, Nov 27, 2019 at 04:25:55PM -0300, Leonardo Bras wrote:
> > On Wed, 2019-11-27 at 19:32 +0100, Paolo Bonzini wrote:
> > > On 27/11/19 19:24, Leonardo Bras wrote:
> > > > By what I could undestand up to now, these functions that use borrowed
> > > > references can only be called while the reference (file descriptor)
> > > > exists.
> > > > So, suppose these threads, where:
> > > > - T1 uses a borrowed reference, and
> > > > - T2 is releasing the reference (close, release):
> > >
> > > Nit: T2 is releasing the *last* reference (as implied by your reference
> > > to close/release).
> >
> > Correct.
> >
> > > > T1 | T2
> > > > kvm_get_kvm() |
> > > > ... | kvm_put_kvm()
> > > > kvm_put_kvm_no_destroy() |
> > > >
> > > > The above would not trigger a use-after-free bug, but will cause a
> > > > memory leak. Is my above understanding right?
> > >
> > > Yes, this is correct.
> > >
> >
> > Then, what would not be a bug before (using kvm_put_kvm()) now is a
> > memory leak (using kvm_put_kvm_no_destroy()).
>
> No, using kvm_put_kvm_no_destroy() changes how a bug would manifest, as
> you note below. Replacing kvm_put_kvm() with kvm_put_kvm_no_destroy()
> when the refcount is _guaranteed_ to be >1 has no impact on correctness.
>
Humm, so what about the above example with T1 and T2?
> > And it's the price to avoid use-after-free on other cases, which is a
> > worse bug. Ok, I get it.
> >
> > > Paolo
> >
> > On Tue, 2019-11-26 at 10:14 -0800, Sean Christopherson wrote:
> > > If one these kvm_put_kvm() calls did unexpectedly free @kvm (due to
> > > a bug somewhere else), KVM would still hit a use-after-free scenario
> > > as the caller still thinks @kvm is valid. Currently, this would
> > > only happen on a subsequent ioctl() on the caller's file descriptor
> > > (which holds a pointer to @kvm), as the callers of these functions
> > > don't directly dereference @kvm after the functions return. But,
> > > not deferencing @kvm isn't deliberate or functionally required, it's
> > > just how the code happens to be written.
> >
> > So, testing if the kvm reference is valid before running ioctl would be
> > enough to avoid these bugs?
>
> No, the only way to avoid use-after-free bugs of this nature is to not
> screw up the refcounting :-) This funky "borrowed reference" pattern is
> not very common. It's necessary here because KVM needs to take an extra
> reference to itself on behalf of the child device before installing the
> child's file descriptor, because once the fd is installed it can be
> closed by userspace and free the child's reference. The error path,
> which uses kvm_put_kvm_no_destroy(), is used if and only if installing
> the fd fails, in which case the extra reference is deliberately thrown
> away.
>
> kvm_put_kvm_no_destroy() is asserting "N > 0" as a way to detect a
> refcounting bug that wouldn't be detected (until later) by the normal
> refcounting behavior, which asserts "N >= 0".
>
> > Is it possible?
>
> No. Similar to above, userspace gets a fd by doing open("/dev/kvm"), and
> the semantics of KVM are such that each fd is a reference to KVM. From
> userspace's perspective, having a valid fd *is* how it knows that it has
> a valid KVM reference.
>
> > Humm, but if it frees kvm before running ->release(), would it mean the
> > VM is destroyed incorrectly, and will probably crash?
>
> More than likely the host will crash due to corrupting memory. The guest
> will crash too, but that's a secondary concern.
Thanks for explaining, it's way more clear to me now how it works.
Best regards,
Leonardo Bras
On Wed, 2019-11-27 at 17:15 -0300, Leonardo Bras wrote:
> > > > > So, suppose these threads, where:
> > > > > - T1 uses a borrowed reference, and
> > > > > - T2 is releasing the reference (close, release):
> > > >
> > > > Nit: T2 is releasing the *last* reference (as implied by your reference
> > > > to close/release).
> > >
> > > Correct.
> > >
> > > > > T1 | T2
> > > > > kvm_get_kvm() |
> > > > > ... | kvm_put_kvm()
> > > > > kvm_put_kvm_no_destroy() |
> > > > >
> > > > > The above would not trigger a use-after-free bug, but will cause a
> > > > > memory leak. Is my above understanding right?
> > > >
> > > > Yes, this is correct.
> > > >
> > >
> > > Then, what would not be a bug before (using kvm_put_kvm()) now is a
> > > memory leak (using kvm_put_kvm_no_destroy()).
> >
Sorry, I missed some information on above example.
Suppose on that example that the reorder changes take place so that
kvm_put_kvm{,_no_destroy}() always happens after the last usage of kvm
(in the same syscall, let's say).
Before T1 and T2, refcount = 1;
If T1 uses kvm_put_kvm_no_destroy():
- T1 increases refcount (=2)
- T2 decreases refcount (=1)
- T1 decreases refcount, (=0) don't free kvm (memleak)
If T1 uses kvm_put_kvm():
- T1 increases refcount (= 2)
- T2 decreases refcount (= 1)
- T1 decreases refcount, (= 0) frees kvm.
So using kvm_put_kvm_no_destroy() would introduce a memleak where it
would have no bug.
> > No, using kvm_put_kvm_no_destroy() changes how a bug would manifest, as
> > you note below. Replacing kvm_put_kvm() with kvm_put_kvm_no_destroy()
> > when the refcount is _guaranteed_ to be >1 has no impact on correctness.
Yes, you are correct.
But on the above case, kvm_put_kvm{,_no_destroy}() would be called
with refcount == 1, and if reorder patch is applied, it would not cause
any use-after-free error, even on kvm_put_kvm() case.
Is the above correct?
Best regards,
Leonardo
On Wed, Nov 27, 2019 at 06:57:10PM -0300, Leonardo Bras wrote:
> On Wed, 2019-11-27 at 17:15 -0300, Leonardo Bras wrote:
> > > > > > So, suppose these threads, where:
> > > > > > - T1 uses a borrowed reference, and
> > > > > > - T2 is releasing the reference (close, release):
> > > > >
> > > > > Nit: T2 is releasing the *last* reference (as implied by your reference
> > > > > to close/release).
> > > >
> > > > Correct.
> > > >
> > > > > > T1 | T2
> > > > > > kvm_get_kvm() |
> > > > > > ... | kvm_put_kvm()
> > > > > > kvm_put_kvm_no_destroy() |
> > > > > >
> > > > > > The above would not trigger a use-after-free bug, but will cause a
> > > > > > memory leak. Is my above understanding right?
> > > > >
> > > > > Yes, this is correct.
> > > > >
> > > >
> > > > Then, what would not be a bug before (using kvm_put_kvm()) now is a
> > > > memory leak (using kvm_put_kvm_no_destroy()).
> > >
>
> Sorry, I missed some information on above example.
> Suppose on that example that the reorder changes take place so that
> kvm_put_kvm{,_no_destroy}() always happens after the last usage of kvm
> (in the same syscall, let's say).
That can't happen, because the ioctl() holds a reference to KVM via its
file descriptor for /dev/kvm, and ioctl() in turn prevents the fd from
being closed.
> Before T1 and T2, refcount = 1;
This is what's impossible. T1 must have an existing reference to get
into the ioctl(), and that reference cannot be dropped until the ioctl()
completes (and by completes I mean returns to userspace). Assuming no
other bugs, i.e. T2 has its own reference, then refcount >= 2.
> If T1 uses kvm_put_kvm_no_destroy():
> - T1 increases refcount (=2)
> - T2 decreases refcount (=1)
> - T1 decreases refcount, (=0) don't free kvm (memleak)
>
> If T1 uses kvm_put_kvm():
> - T1 increases refcount (= 2)
> - T2 decreases refcount (= 1)
> - T1 decreases refcount, (= 0) frees kvm.
>
> So using kvm_put_kvm_no_destroy() would introduce a memleak where it
> would have no bug.
>
> > > No, using kvm_put_kvm_no_destroy() changes how a bug would manifest, as
> > > you note below. Replacing kvm_put_kvm() with kvm_put_kvm_no_destroy()
> > > when the refcount is _guaranteed_ to be >1 has no impact on correctness.
>
> Yes, you are correct.
> But on the above case, kvm_put_kvm{,_no_destroy}() would be called
> with refcount == 1, and if reorder patch is applied, it would not cause
> any use-after-free error, even on kvm_put_kvm() case.
>
> Is the above correct?
No, see above.
On 27/11/19 22:57, Leonardo Bras wrote:
> But on the above case, kvm_put_kvm{,_no_destroy}() would be called
> with refcount == 1, and if reorder patch is applied, it would not cause
> any use-after-free error, even on kvm_put_kvm() case.
I think this is what you're missing: kvm_put_kvm_no_destroy() does not
protect against bugs in the code that uses it. It protect against bugs
_elsewhere_.
Therefore, kvm_put_kvm_no_destroy() is always a better choice when
applicable, because it turns bugs in _other parts of the code_ from
use-after-free to WARN+leak.
Paolo
On Thu, 2019-11-28 at 14:49 +0100, Paolo Bonzini wrote:
> On 27/11/19 22:57, Leonardo Bras wrote:
> > But on the above case, kvm_put_kvm{,_no_destroy}() would be called
> > with refcount == 1, and if reorder patch is applied, it would not cause
> > any use-after-free error, even on kvm_put_kvm() case.
>
> I think this is what you're missing: kvm_put_kvm_no_destroy() does not
> protect against bugs in the code that uses it. It protect against bugs
> _elsewhere_.
>
> Therefore, kvm_put_kvm_no_destroy() is always a better choice when
> applicable, because it turns bugs in _other parts of the code_ from
> use-after-free to WARN+leak.
>
> Paolo
>
Hello Paolo,
Thanks for explaining that! I think I got to understand it better now.
Best regards,
Leonardo
On Wed, 2019-11-27 at 17:00 -0800, Sean Christopherson wrote:
> > Sorry, I missed some information on above example.
> > Suppose on that example that the reorder changes take place so that
> > kvm_put_kvm{,_no_destroy}() always happens after the last usage of kvm
> > (in the same syscall, let's say).
>
> That can't happen, because the ioctl() holds a reference to KVM via its
> file descriptor for /dev/kvm, and ioctl() in turn prevents the fd from
> being closed.
>
> > Before T1 and T2, refcount = 1;
>
> This is what's impossible. T1 must have an existing reference to get
> into the ioctl(), and that reference cannot be dropped until the ioctl()
> completes (and by completes I mean returns to userspace). Assuming no
> other bugs, i.e. T2 has its own reference, then refcount >= 2.
>
Thanks for explaining, I think I get it now.
Best regards,
Leonardo Bras