KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n
PATCH 1/1 KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n
On a system without KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).
Take a more radical approach and return a -ENODEV to the compat task.
--- virt/kvm/kvm_main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
virt/kvm/kvm_main.c +7 -1
@@ -122,7 +122,13 @@ static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl,#else static long kvm_no_compat_ioctl (struct file *file,unsigned int ioctl,unsigned long arg) {return -EINVAL ; } -#define KVM_COMPAT (c) .compat_ioctl = kvm_no_compat_ioctl + +static int kvm_no_compat_open (struct inode *inode,struct file *file) +{ +return is_compat_task () ? -ENODEV :0 ; +} +#define KVM_COMPAT (c) .compat_ioctl = kvm_no_compat_ioctl, \ + .open = kvm_no_compat_open#endif static int hardware_enable_all (void );static void hardware_disable_all (void );
Signature · 1 line
2.20.1
Quoted 25 lines · Marc Zyngier
On a system without
KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).Take a more radical approach and return a
-ENODEVto the compat task.Reported-by Peter Maydell <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- virt/kvm/kvm_main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)virt/kvm/kvm_main.c +0
-1@@ -122,7 +122,13 @@ static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl,#else static long kvm_no_compat_ioctl (struct file *file,unsigned int ioctl,unsigned long arg) {return -EINVAL ; } -#define KVM_COMPAT (c) .compat_ioctl = kvm_no_compat_ioctl
virt/kvm/kvm_main.c static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl, · line 134
Quoted 10 lines
virt/kvm/kvm_main.c +7
-0+ +static int kvm_no_compat_open (struct inode *inode,struct file *file) +{ +return is_compat_task () ? -ENODEV :0 ; +} +#define KVM_COMPAT (c) .compat_ioctl = kvm_no_compat_ioctl, \ + .open = kvm_no_compat_open#endif static int hardware_enable_all (void );static void hardware_disable_all (void );
Queued, thanks.
Paolo
Quoted 20 lines · Marc Zyngier
On a system without
KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).Take a more radical approach and return a
-ENODEVto the compat task.Reported-by Peter Maydell <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- virt/kvm/kvm_main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)virt/kvm/kvm_main.c +0
-0
virt/kvm/kvm_main.c static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl, · line 130
Quoted 10 lines
virt/kvm/kvm_main.c +6
-1@@ -122,7 +122,13 @@ static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl,#else static long kvm_no_compat_ioctl (struct file *file,unsigned int ioctl,unsigned long arg) {return -EINVAL ; } -#define KVM_COMPAT (c) .compat_ioctl = kvm_no_compat_ioctl + +static int kvm_no_compat_open (struct inode *inode,struct file *file) +{ +return is_compat_task () ? -ENODEV :0 ; +} +#define KVM_COMPAT (c) .compat_ioctl = kvm_no_compat_ioctl, \
Do we still need compat_ioctl if open never succeeds?
Quoted 5 lines
virt/kvm/kvm_main.c +1
-0+ .open = kvm_no_compat_open#endif static int hardware_enable_all (void );static void hardware_disable_all (void );
Quoted 11 lines · Christian Borntraeger
Quoted 8 lines · Marc Zyngier
On a system without
KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).Take a more radical approach and return a
-ENODEVto the compat task.Do we still need
compat_ioctlif open never succeeds?
I wondered about that, but presumably you could use fd-passing, or just inheriting open fds across exec(), to open the fd in a 64-bit process and then hand it off to a 32-bit process to call the ioctl with. (That's probably only something you'd do if you were deliberately playing silly games, of course, but preventing silly games is useful as it makes it easier to reason about kernel behaviour.)
thanks
-- PMM
Quoted 14 lines · On Wed, 13 Nov 2019 21:23:07 +0000 Peter Maydell <[email protected]>…
Quoted 11 lines · Christian Borntraeger
Quoted 8 lines · Marc Zyngier
On a system without
KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).Take a more radical approach and return a
-ENODEVto the compat task.Do we still need
compat_ioctlif open never succeeds?
Quoted 8 lines
I wondered about that, but presumably you could use fd-passing, or just inheriting open fds across
exec(), to open the fd in a 64-bit process and then hand it off to a 32-bit process to call the ioctl with. (That's probably only something you'd do if you were deliberately playing silly games, of course, but preventing silly games is useful as it makes it easier to reason about kernel behaviour.)
This was exactly my train of thoughts, which I should have made clear in the commit log. Thanks Peter for reading my mind! ;-)
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 26 lines · Marc Zyngier
Quoted 14 lines · On Wed, 13 Nov 2019 21:23:07 +0000 Peter Maydell <[email protected]>…
Quoted 11 lines · Christian Borntraeger
Quoted 8 lines · Marc Zyngier
On a system without
KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).Take a more radical approach and return a
-ENODEVto the compat task.Do we still need
compat_ioctlif open never succeeds?Quoted 8 lines
I wondered about that, but presumably you could use fd-passing, or just inheriting open fds across
exec(), to open the fd in a 64-bit process and then hand it off to a 32-bit process to call the ioctl with. (That's probably only something you'd do if you were deliberately playing silly games, of course, but preventing silly games is useful as it makes it easier to reason about kernel behaviour.)
Quoted 2 lines
This was exactly my train of thoughts, which I should have made clear in the commit log. Thanks Peter for reading my mind! ;-)
Makes sense. Looks like this is already in kvm/master so we cannot improve the commit message easily any more. Hopefully we will not forget :-)
Paolo Bonzini
maintainerQuoted 29 lines · Christian Borntraeger
Quoted 26 lines · Marc Zyngier
Quoted 22 lines · On Wed, 13 Nov 2019 21:23:07 +0000 Peter Maydell <[email protected]>…
Quoted 11 lines · Christian Borntraeger
Quoted 8 lines · Marc Zyngier
On a system without
KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).Take a more radical approach and return a
-ENODEVto the compat task.Do we still need
compat_ioctlif open never succeeds?I wondered about that, but presumably you could use fd-passing, or just inheriting open fds across
exec(), to open the fd in a 64-bit process and then hand it off to a 32-bit process to call the ioctl with. (That's probably only something you'd do if you were deliberately playing silly games, of course, but preventing silly games is useful as it makes it easier to reason about kernel behaviour.)
Quoted 5 lines
Quoted 2 lines
This was exactly my train of thoughts, which I should have made clear in the commit log. Thanks Peter for reading my mind! ;-)
Makes sense. Looks like this is already in kvm/master so we cannot improve the commit message easily any more. Hopefully we will not forget :-)
A comment in the code would probably be better than the commit message, to not forget stuff like this. (Hint! :))
Paolo
Quoted 30 lines · Paolo Bonzini
Quoted 29 lines · Christian Borntraeger
Quoted 26 lines · Marc Zyngier
Quoted 22 lines · On Wed, 13 Nov 2019 21:23:07 +0000 Peter Maydell <[email protected]>…
Quoted 11 lines · Christian Borntraeger
Quoted 8 lines · Marc Zyngier
On a system without
KVM_COMPAT, we prevent IOCTLs from being issued by a compat task. Although this prevents most silly things from happening, it can still confuse a 32bit userspace that is able to open the kvm device (the qemu test suite seems to be pretty mad with this behaviour).Take a more radical approach and return a
-ENODEVto the compat task.Do we still need
compat_ioctlif open never succeeds?I wondered about that, but presumably you could use fd-passing, or just inheriting open fds across
exec(), to open the fd in a 64-bit process and then hand it off to a 32-bit process to call the ioctl with. (That's probably only something you'd do if you were deliberately playing silly games, of course, but preventing silly games is useful as it makes it easier to reason about kernel behaviour.)
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
This was exactly my train of thoughts, which I should have made clear in the commit log. Thanks Peter for reading my mind! ;-)
Makes sense. Looks like this is already in kvm/master so we cannot improve the commit message easily any more. Hopefully we will not forget :-)
A comment in the code would probably be better than the commit message, to not forget stuff like this. (Hint! :))
Hint received. How about this?
M.
From 34bfc68752253c3da3e59072b137d1a4a85bc005 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Thu, 14 Nov 2019 13:17:39 +0000
Subject: [PATCH] KVM: Add a comment describing the /dev/kvm no_compat handling
Add a comment explaining the rational behind having both no_compat open and ioctl callbacks to fend off compat tasks.
--- virt/kvm/kvm_main.c | 7 +++++++ 1 file changed, 7 insertions(+)
virt/kvm/kvm_main.c +7 -0
@@ -120,6 +120,13 @@ static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl, unsigned long arg);#define KVM_COMPAT (c) .compat_ioctl = (c)#else +/* +* For architectures that don't implement a compat infrastructure, +* adopt a double line of defense: +* - Prevent a compat task from opening /dev/kvm +* - If the open has been done by a 64bit task, and the KVM fd +* passed to a compat task, let the ioctls fail. +*/ static long kvm_no_compat_ioctl (struct file *file,unsigned int ioctl,unsigned long arg) {return -EINVAL ; }
Signature · 5 lines
2.20.1 -- Jazz is not dead. It just smells funny...
Quoted 7 lines · Marc Zyngier
From
34bfc68752253c3da3e59072b137d1a4a85bc005Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Thu, 14 Nov 2019 13:17:39 +0000
Subject: [PATCH] KVM: Add a comment describing the/dev/kvmno_compathandlingAdd a comment explaining the rational behind having both
"rationale". (Isn't English spelling wonderful?)
Quoted 3 lines
no_compatopen and ioctl callbacks to fend off compat tasks.Signed-off-by Marc Zyngier <[email protected]>
thanks
-- PMM
Paolo Bonzini
maintainerQuoted 10 lines · Peter Maydell
Quoted 7 lines · Marc Zyngier
From
34bfc68752253c3da3e59072b137d1a4a85bc005Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Thu, 14 Nov 2019 13:17:39 +0000
Subject: [PATCH] KVM: Add a comment describing the/dev/kvmno_compathandlingAdd a comment explaining the rational behind having both
"rationale". (Isn't English spelling wonderful?)
Oops, noted this only after sending the pull request.
Thanks,
Paolo
Quoted 7 lines
Quoted 3 lines
no_compatopen and ioctl callbacks to fend off compat tasks.Signed-off-by Marc Zyngier <[email protected]>thanks
-- PMM
Quoted 34 lines · Marc Zyngier
From
34bfc68752253c3da3e59072b137d1a4a85bc005Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Thu, 14 Nov 2019 13:17:39 +0000
Subject: [PATCH] KVM: Add a comment describing the/dev/kvmno_compathandlingAdd a comment explaining the rational behind having both
no_compatopen and ioctl callbacks to fend off compat tasks.Signed-off-by Marc Zyngier <[email protected]>---
virt/kvm/kvm_main.c | 7 +++++++1 file changed, 7 insertions(+)virt/kvm/kvm_main.c +7
-0@@ -120,6 +120,13 @@ static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl, unsigned long arg); #define KVM_COMPAT(c) .compat_ioctl = (c) #else +/* +* For architectures that don't implement a compat infrastructure, +* adopt a double line of defense: +* - Prevent a compat task from opening /dev/kvm +* - If the open has been done by a 64bit task, and the KVM fd +* passed to a compat task, let the ioctls fail. +*/ static long kvm_no_compat_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) { return -EINVAL; }
Quoted 2 lines
--
2.20.1
Queued, thanks!
Paolo