| Message ID | 20191113160523.16130-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n | expand |
On 13/11/19 17:05, Marc Zyngier wrote: > 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. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/kvm_main.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 543024c7a87f..1243e48dc717 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -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); > Queued, thanks. Paolo
On 13.11.19 17:05, Marc Zyngier wrote: > 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. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/kvm_main.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 543024c7a87f..1243e48dc717 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -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? > + .open = kvm_no_compat_open > #endif > static int hardware_enable_all(void); > static void hardware_disable_all(void); >
On Wed, 13 Nov 2019 at 18:44, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 13.11.19 17:05, Marc Zyngier wrote: > > 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. > Do we still need compat_ioctl if 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
On Wed, 13 Nov 2019 21:23:07 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Wed, 13 Nov 2019 at 18:44, Christian Borntraeger > <borntraeger@de.ibm.com> wrote: > > On 13.11.19 17:05, Marc Zyngier wrote: > > > 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. > > > Do we still need compat_ioctl if 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.) This was exactly my train of thoughts, which I should have made clear in the commit log. Thanks Peter for reading my mind! ;-) M.
On 14.11.19 09:15, Marc Zyngier wrote: > On Wed, 13 Nov 2019 21:23:07 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On Wed, 13 Nov 2019 at 18:44, Christian Borntraeger >> <borntraeger@de.ibm.com> wrote: >>> On 13.11.19 17:05, Marc Zyngier wrote: >>>> 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. >> >>> Do we still need compat_ioctl if 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.) > > 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 :-)
On 14/11/19 09:20, Christian Borntraeger wrote: > > > On 14.11.19 09:15, Marc Zyngier wrote: >> On Wed, 13 Nov 2019 21:23:07 +0000 >> Peter Maydell <peter.maydell@linaro.org> wrote: >> >>> On Wed, 13 Nov 2019 at 18:44, Christian Borntraeger >>> <borntraeger@de.ibm.com> wrote: >>>> On 13.11.19 17:05, Marc Zyngier wrote: >>>>> 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. >>> >>>> Do we still need compat_ioctl if 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.) >> >> 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
On 2019-11-14 12:12, Paolo Bonzini wrote: > On 14/11/19 09:20, Christian Borntraeger wrote: >> >> >> On 14.11.19 09:15, Marc Zyngier wrote: >>> On Wed, 13 Nov 2019 21:23:07 +0000 >>> Peter Maydell <peter.maydell@linaro.org> wrote: >>> >>>> On Wed, 13 Nov 2019 at 18:44, Christian Borntraeger >>>> <borntraeger@de.ibm.com> wrote: >>>>> On 13.11.19 17:05, Marc Zyngier wrote: >>>>>> 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. >>>> >>>>> Do we still need compat_ioctl if 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.) >>> >>> 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 <maz@kernel.org> 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. Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/kvm_main.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 1243e48dc717..722f2b1d4672 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -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; }
On Thu, 14 Nov 2019 at 13:22, Marc Zyngier <maz@kernel.org> wrote: > From 34bfc68752253c3da3e59072b137d1a4a85bc005 Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > 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 "rationale". (Isn't English spelling wonderful?) > no_compat open and ioctl callbacks to fend off compat tasks. > > Signed-off-by: Marc Zyngier <maz@kernel.org> thanks -- PMM
On 14/11/19 14:22, Marc Zyngier wrote: > > From 34bfc68752253c3da3e59072b137d1a4a85bc005 Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > 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. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/kvm_main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 1243e48dc717..722f2b1d4672 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -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; } > > -- > 2.20.1 Queued, thanks! Paolo
On 14/11/19 14:28, Peter Maydell wrote: > On Thu, 14 Nov 2019 at 13:22, Marc Zyngier <maz@kernel.org> wrote: >> From 34bfc68752253c3da3e59072b137d1a4a85bc005 Mon Sep 17 00:00:00 2001 >> From: Marc Zyngier <maz@kernel.org> >> 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 > > "rationale". (Isn't English spelling wonderful?) Oops, noted this only after sending the pull request. Thanks, Paolo >> no_compat open and ioctl callbacks to fend off compat tasks. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> > > thanks > -- PMM >
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 543024c7a87f..1243e48dc717 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -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);
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. Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/kvm_main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)