kvm: use anon_inode_getfd() with O_CLOEXEC flag

KVM uses anon_inode_get() to allocate file descriptors as part of some of its ioctls. But those ioctls are lacking a flag argument allowing userspace to choose options for the newly opened file descriptor.

In such case it's advised to use O_CLOEXEC by default so that userspace is allowed to choose, without race, if the file descriptor is going to be inherited across exec().

This patch set O_CLOEXEC flag on all file descriptors created with anon_inode_getfd() to not leak file descriptors across exec().

Signed-off-by Yann Droneaud <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 virt/kvm/kvm_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

virt/kvm/kvm_main.c +3 -3

index 89f74d1..d65cc0c 100644
@@ -1896,7 +1896,7 @@ static struct file_operations kvm_vcpu_fops = {
  */
 static int create_vcpu_fd(struct kvm_vcpu *vcpu)
 {
-	return anon_inode_getfd("kvm-vcpu", &kvm_vcpu_fops, vcpu, O_RDWR);
+	return anon_inode_getfd("kvm-vcpu", &kvm_vcpu_fops, vcpu, O_RDWR | O_CLOEXEC);
 }
 
 /*

@@ -2306,7 +2306,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		return ret;
 	}
 
-	ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR);
+	ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC);
 	if (ret < 0) {
 		ops->destroy(dev);
 		return ret;

@@ -2590,7 +2590,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 		return r;
 	}
 #endif
-	r = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
+	r = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, O_RDWR | O_CLOEXEC);
 	if (r < 0)
 		kvm_put_kvm(kvm);
 

KVM uses anon_inode_get() to allocate file descriptors as part of some of its ioctls. But those ioctls are lacking a flag argument allowing userspace to choose options for the newly opened file descriptor.

In such case it's advised to use O_CLOEXEC by default so that userspace is allowed to choose, without race, if the file descriptor is going to be inherited across exec().

This patch set O_CLOEXEC flag on all file descriptors created with anon_inode_getfd() to not leak file descriptors across exec().

Signed-off-by Yann Droneaud <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Quoted 12 lines

KVM uses anon_inode_get() to allocate file descriptors as part of some of its ioctls. But those ioctls are lacking a flag argument allowing userspace to choose options for the newly opened file descriptor.

In such case it's advised to use O_CLOEXEC by default so that userspace is allowed to choose, without race, if the file descriptor is going to be inherited across exec().

This patch set O_CLOEXEC flag on all file descriptors created with anon_inode_getfd() to not leak file descriptors across exec().

Quoted 2 lines

Signed-off-by Yann Droneaud <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Reviewed-by Alexander Graf <[email protected]>

Would it make sense to simply inherit the O_CLOEXEC flag from the parent kvm fd instead? That would give user space the power to keep fds across exec() if it wants to.

Il 25/08/2013 17:04, Alexander Graf ha scritto:

Quoted 10 lines · Yann Droneaud

Quoted 8 lines

This patch set O_CLOEXEC flag on all file descriptors created with anon_inode_getfd() to not leak file descriptors across exec().

Signed-off-by Yann Droneaud <[email protected]>

Link:
http://lkml.kernel.org/r/[email protected]

Quoted 5 lines

Reviewed-by Alexander Graf <[email protected]>

Would it make sense to simply inherit the O_CLOEXEC flag from the parent kvm fd instead? That would give user space the power to keep fds across exec() if it wants to.

Does it make sense to use non-O_CLOEXEC file descriptors with KVM at all? Besides fork() not being supported by KVM, as described in Documentation/virtual/kvm/api.txt, the VMAs of the parent process go away as soon as you exec(). I'm not sure how you can use the inherited file descriptor in a sensible way after exec().

Sounds a lot like InfiniBand subsystem behavor: IB file descriptors are of no use accross exec() since memory mappings tied to those fds won't be available in the new process:

https://lkml.org/lkml/2013/7/8/380
http://mid.gmane.org/[email protected]

Hi,

Following a patchset asking to change calls to get_unused_flag() [1] to use O_CLOEXEC, Alex Williamson [2][3] decided to change VFIO to use the flag.

Since it's a related subsystem to KVM, using O_CLOEXEC for file descriptors created by KVM might be applicable too.

I'm suggesting to change calls to anon_inode_getfd() to use O_CLOEXEC as default flag.

This patchset should be reviewed to not break existing userspace program.

BTW, if it's not applicable, I would suggest that new ioctls be added to KVM subsystem, those ioctls would have a "flag" field added to their arguments. Such "flag" would let userspace choose the open flag to use.
See for example other APIs using anon_inode_getfd() such as fanotify, inotify, signalfd and timerfd.

You might be interested to read:

- Secure File Descriptor Handling (Ulrich Drepper, 2008)
http://udrepper.livejournal.com/20407.html

- Excuse me son, but your code is leaking !!! (Dan Walsh, March 2012)
http://danwalsh.livejournal.com/53603.html

Regards.

[1] http://lkml.kernel.org/r/[email protected]
[2] http://lkml.kernel.org/r/[email protected]
[3] http://lkml.kernel.org/r/[email protected]

Yann Droneaud (2):
kvm: use anon_inode_getfd() with O_CLOEXEC flag
ppc: kvm: use anon_inode_getfd() with O_CLOEXEC flag

 arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
 arch/powerpc/kvm/book3s_64_vio.c    | 2 +-
 arch/powerpc/kvm/book3s_hv.c        | 2 +-
 virt/kvm/kvm_main.c                 | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

Signature · 1 line

1.8.3.1