[PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[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 -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);
-- 
2.20.1

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Paolo Bonzini]

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

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Christian Borntraeger]

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);
> 

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Peter Maydell]

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

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Christian Borntraeger]

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 :-)

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Paolo Bonzini]

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

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Marc Zyngier]

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; }

-- 
2.20.1


-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Peter Maydell]

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

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Paolo Bonzini]

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
> 

Re: [PATCH] KVM: Forbid /dev/kvm being opened by a compat task when CONFIG_KVM_COMPAT=n

[Paolo Bonzini]

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