[PATCH 3.18,4.4] kvm: fix kvm_ioctl_create_device() reference counting (CVE-2019-6974)

[PATCH 3.18,4.4] kvm: fix kvm_ioctl_create_device() reference counting (CVE-2019-6974)

[Jann Horn]

commit cfa39381173d5f969daf43582c95ad679189cbc9 upstream.

kvm_ioctl_create_device() does the following:

1. creates a device that holds a reference to the VM object (with a borrowed
   reference, the VM's refcount has not been bumped yet)
2. initializes the device
3. transfers the reference to the device to the caller's file descriptor table
4. calls kvm_get_kvm() to turn the borrowed reference to the VM into a real
   reference

The ownership transfer in step 3 must not happen before the reference to the VM
becomes a proper, non-borrowed reference, which only happens in step 4.
After step 3, an attacker can close the file descriptor and drop the borrowed
reference, which can cause the refcount of the kvm object to drop to zero.

This means that we need to grab a reference for the device before
anon_inode_getfd(), otherwise the VM can disappear from under us.

Fixes: 852b6d57dc7f ("kvm: add device control API")
Cc: stable@kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 virt/kvm/kvm_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e4be695eb789..fce48d11ae07 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2711,14 +2711,15 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		return ret;
 	}
 
+	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);
 		ops->destroy(dev);
 		return ret;
 	}
 
 	list_add(&dev->vm_node, &kvm->devices);
-	kvm_get_kvm(kvm);
 	cd->fd = ret;
 	return 0;
 }
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH 3.18,4.4] kvm: fix kvm_ioctl_create_device() reference counting (CVE-2019-6974)g

[Sasha Levin]

On Mon, Feb 18, 2019 at 05:53:28PM +0100, Jann Horn wrote:
>commit cfa39381173d5f969daf43582c95ad679189cbc9 upstream.
>
>kvm_ioctl_create_device() does the following:
>
>1. creates a device that holds a reference to the VM object (with a borrowed
>   reference, the VM's refcount has not been bumped yet)
>2. initializes the device
>3. transfers the reference to the device to the caller's file descriptor table
>4. calls kvm_get_kvm() to turn the borrowed reference to the VM into a real
>   reference
>
>The ownership transfer in step 3 must not happen before the reference to the VM
>becomes a proper, non-borrowed reference, which only happens in step 4.
>After step 3, an attacker can close the file descriptor and drop the borrowed
>reference, which can cause the refcount of the kvm object to drop to zero.
>
>This means that we need to grab a reference for the device before
>anon_inode_getfd(), otherwise the VM can disappear from under us.
>
>Fixes: 852b6d57dc7f ("kvm: add device control API")
>Cc: stable@kernel.org
>Signed-off-by: Jann Horn <jannh@google.com>

Hi Jann,

You've dropped Paolo's S-O-B line, was it on purpose?

--
Thanks,
Sasha

Re: [PATCH 3.18,4.4] kvm: fix kvm_ioctl_create_device() reference counting (CVE-2019-6974)

[Greg KH]

On Mon, Feb 18, 2019 at 05:53:28PM +0100, Jann Horn wrote:
> commit cfa39381173d5f969daf43582c95ad679189cbc9 upstream.
> 
> kvm_ioctl_create_device() does the following:
> 
> 1. creates a device that holds a reference to the VM object (with a borrowed
>    reference, the VM's refcount has not been bumped yet)
> 2. initializes the device
> 3. transfers the reference to the device to the caller's file descriptor table
> 4. calls kvm_get_kvm() to turn the borrowed reference to the VM into a real
>    reference
> 
> The ownership transfer in step 3 must not happen before the reference to the VM
> becomes a proper, non-borrowed reference, which only happens in step 4.
> After step 3, an attacker can close the file descriptor and drop the borrowed
> reference, which can cause the refcount of the kvm object to drop to zero.
> 
> This means that we need to grab a reference for the device before
> anon_inode_getfd(), otherwise the VM can disappear from under us.
> 
> Fixes: 852b6d57dc7f ("kvm: add device control API")
> Cc: stable@kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  virt/kvm/kvm_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Now queued up, thanks.

greg k-h

Re: [PATCH 3.18,4.4] kvm: fix kvm_ioctl_create_device() reference counting (CVE-2019-6974)g

[Jann Horn]

On Thu, Feb 21, 2019 at 5:10 AM Sasha Levin <sashal@kernel.org> wrote:
>
> On Mon, Feb 18, 2019 at 05:53:28PM +0100, Jann Horn wrote:
> >commit cfa39381173d5f969daf43582c95ad679189cbc9 upstream.
> >
> >kvm_ioctl_create_device() does the following:
> >
> >1. creates a device that holds a reference to the VM object (with a borrowed
> >   reference, the VM's refcount has not been bumped yet)
> >2. initializes the device
> >3. transfers the reference to the device to the caller's file descriptor table
> >4. calls kvm_get_kvm() to turn the borrowed reference to the VM into a real
> >   reference
> >
> >The ownership transfer in step 3 must not happen before the reference to the VM
> >becomes a proper, non-borrowed reference, which only happens in step 4.
> >After step 3, an attacker can close the file descriptor and drop the borrowed
> >reference, which can cause the refcount of the kvm object to drop to zero.
> >
> >This means that we need to grab a reference for the device before
> >anon_inode_getfd(), otherwise the VM can disappear from under us.
> >
> >Fixes: 852b6d57dc7f ("kvm: add device control API")
> >Cc: stable@kernel.org
> >Signed-off-by: Jann Horn <jannh@google.com>
>
> Hi Jann,
>
> You've dropped Paolo's S-O-B line, was it on purpose?

The stable kernel rules don't explicitly say that I'm allowed to keep
other people's sign-off lines when manually submitting backports, so I
removed it to be on the safe side.

Re: [PATCH 3.18,4.4] kvm: fix kvm_ioctl_create_device() reference counting (CVE-2019-6974)g

[Paolo Bonzini]

On 21/02/19 14:03, Jann Horn wrote:
> On Thu, Feb 21, 2019 at 5:10 AM Sasha Levin <sashal@kernel.org> wrote:
>> Hi Jann,
>>
>> You've dropped Paolo's S-O-B line, was it on purpose?
> 
> The stable kernel rules don't explicitly say that I'm allowed to keep
> other people's sign-off lines when manually submitting backports, so I
> removed it to be on the safe side.

Yeah, since I wasn't really involved in the creation of this patch, and
I had not modified Jann's original submission in any way, I guess it's
okay either way.

Paolo