[PATCH 0/2] KVM: Fix !KVM_COMPAT ioctl behaviour

[PATCH 0/2] KVM: Fix !KVM_COMPAT ioctl behaviour

[Marc Zyngier]

Mark Rutland recently pointed out that a couple of issues with the way
we deal with compat tasks:

1) arm64 unexpectedly selects KVM_COMPAT, while offering no way for a
   32bit userspace to change any register

2) Even deselecting KVM_COMPAT, we end-up with compat_ioctl being NULL,
   and the VFS behaviour is thus to call the non-compat ioctl

(1) is completely unintentional and should be fixed. (2) is quite odd,
and probably just as unintentional. This series fixes it by installing
a compat_ioctl callback whose only job is to return -EINVAL.

Tested on arm64 with a 32bit kvmtool.

Marc Zyngier (2):
  KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT
  KVM: arm64: Prevent KVM_COMPAT from being selected

 virt/kvm/Kconfig    |  2 +-
 virt/kvm/kvm_main.c | 19 +++++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH 1/2] KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT

[Marc Zyngier]

The current behaviour of the compat ioctls is a bit odd.
We provide a compat_ioctl method when KVM_COMPAT is set, and NULL
otherwise. But NULL means that the normal, non-compat ioctl should
be used directly for compat tasks, and there is no way to actually
prevent a compat task from issueing KVM ioctls.

This patch changes this behaviour, by always registering a compat_ioctl
method, even if KVM_COMPAT is not selected. In that case, the callback
will always return -EINVAL.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/kvm_main.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ada21f47f22b..8b47507faab5 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -116,6 +116,11 @@ static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
 #ifdef CONFIG_KVM_COMPAT
 static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl,
 				  unsigned long arg);
+#define KVM_COMPAT(c)	.compat_ioctl	= (c)
+#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
 #endif
 static int hardware_enable_all(void);
 static void hardware_disable_all(void);
@@ -2396,11 +2401,9 @@ static int kvm_vcpu_release(struct inode *inode, struct file *filp)
 static struct file_operations kvm_vcpu_fops = {
 	.release        = kvm_vcpu_release,
 	.unlocked_ioctl = kvm_vcpu_ioctl,
-#ifdef CONFIG_KVM_COMPAT
-	.compat_ioctl   = kvm_vcpu_compat_ioctl,
-#endif
 	.mmap           = kvm_vcpu_mmap,
 	.llseek		= noop_llseek,
+	KVM_COMPAT(kvm_vcpu_compat_ioctl),
 };
 
 /*
@@ -2824,10 +2827,8 @@ static int kvm_device_release(struct inode *inode, struct file *filp)
 
 static const struct file_operations kvm_device_fops = {
 	.unlocked_ioctl = kvm_device_ioctl,
-#ifdef CONFIG_KVM_COMPAT
-	.compat_ioctl = kvm_device_ioctl,
-#endif
 	.release = kvm_device_release,
+	KVM_COMPAT(kvm_device_ioctl),
 };
 
 struct kvm_device *kvm_device_from_filp(struct file *filp)
@@ -3165,10 +3166,8 @@ static long kvm_vm_compat_ioctl(struct file *filp,
 static struct file_operations kvm_vm_fops = {
 	.release        = kvm_vm_release,
 	.unlocked_ioctl = kvm_vm_ioctl,
-#ifdef CONFIG_KVM_COMPAT
-	.compat_ioctl   = kvm_vm_compat_ioctl,
-#endif
 	.llseek		= noop_llseek,
+	KVM_COMPAT(kvm_vm_compat_ioctl),
 };
 
 static int kvm_dev_ioctl_create_vm(unsigned long type)
@@ -3259,8 +3258,8 @@ static long kvm_dev_ioctl(struct file *filp,
 
 static struct file_operations kvm_chardev_ops = {
 	.unlocked_ioctl = kvm_dev_ioctl,
-	.compat_ioctl   = kvm_dev_ioctl,
 	.llseek		= noop_llseek,
+	KVM_COMPAT(kvm_dev_ioctl),
 };
 
 static struct miscdevice kvm_dev = {
-- 
2.17.1

[PATCH 1/2] KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT

[Mark Rutland]

On Tue, Jun 19, 2018 at 10:42:50AM +0100, Marc Zyngier wrote:
> The current behaviour of the compat ioctls is a bit odd.
> We provide a compat_ioctl method when KVM_COMPAT is set, and NULL
> otherwise. But NULL means that the normal, non-compat ioctl should
> be used directly for compat tasks, and there is no way to actually
> prevent a compat task from issueing KVM ioctls.
> 
> This patch changes this behaviour, by always registering a compat_ioctl
> method, even if KVM_COMPAT is not selected. In that case, the callback
> will always return -EINVAL.
> 
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

I virt/kvm/Kconfig we have:

config KVM_COMPAT
       def_bool y
       depends on KVM && COMPAT && !S390

... and in arch/s390 we have COMPAT support, so does this potentially break
anything there?

Thanks,
Mark.

> ---
>  virt/kvm/kvm_main.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ada21f47f22b..8b47507faab5 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -116,6 +116,11 @@ static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
>  #ifdef CONFIG_KVM_COMPAT
>  static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl,
>  				  unsigned long arg);
> +#define KVM_COMPAT(c)	.compat_ioctl	= (c)
> +#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
>  #endif
>  static int hardware_enable_all(void);
>  static void hardware_disable_all(void);
> @@ -2396,11 +2401,9 @@ static int kvm_vcpu_release(struct inode *inode, struct file *filp)
>  static struct file_operations kvm_vcpu_fops = {
>  	.release        = kvm_vcpu_release,
>  	.unlocked_ioctl = kvm_vcpu_ioctl,
> -#ifdef CONFIG_KVM_COMPAT
> -	.compat_ioctl   = kvm_vcpu_compat_ioctl,
> -#endif
>  	.mmap           = kvm_vcpu_mmap,
>  	.llseek		= noop_llseek,
> +	KVM_COMPAT(kvm_vcpu_compat_ioctl),
>  };
>  
>  /*
> @@ -2824,10 +2827,8 @@ static int kvm_device_release(struct inode *inode, struct file *filp)
>  
>  static const struct file_operations kvm_device_fops = {
>  	.unlocked_ioctl = kvm_device_ioctl,
> -#ifdef CONFIG_KVM_COMPAT
> -	.compat_ioctl = kvm_device_ioctl,
> -#endif
>  	.release = kvm_device_release,
> +	KVM_COMPAT(kvm_device_ioctl),
>  };
>  
>  struct kvm_device *kvm_device_from_filp(struct file *filp)
> @@ -3165,10 +3166,8 @@ static long kvm_vm_compat_ioctl(struct file *filp,
>  static struct file_operations kvm_vm_fops = {
>  	.release        = kvm_vm_release,
>  	.unlocked_ioctl = kvm_vm_ioctl,
> -#ifdef CONFIG_KVM_COMPAT
> -	.compat_ioctl   = kvm_vm_compat_ioctl,
> -#endif
>  	.llseek		= noop_llseek,
> +	KVM_COMPAT(kvm_vm_compat_ioctl),
>  };
>  
>  static int kvm_dev_ioctl_create_vm(unsigned long type)
> @@ -3259,8 +3258,8 @@ static long kvm_dev_ioctl(struct file *filp,
>  
>  static struct file_operations kvm_chardev_ops = {
>  	.unlocked_ioctl = kvm_dev_ioctl,
> -	.compat_ioctl   = kvm_dev_ioctl,
>  	.llseek		= noop_llseek,
> +	KVM_COMPAT(kvm_dev_ioctl),
>  };
>  
>  static struct miscdevice kvm_dev = {
> -- 
> 2.17.1
> 

[PATCH 1/2] KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT

[Marc Zyngier]

On 19/06/18 11:01, Mark Rutland wrote:
> On Tue, Jun 19, 2018 at 10:42:50AM +0100, Marc Zyngier wrote:
>> The current behaviour of the compat ioctls is a bit odd.
>> We provide a compat_ioctl method when KVM_COMPAT is set, and NULL
>> otherwise. But NULL means that the normal, non-compat ioctl should
>> be used directly for compat tasks, and there is no way to actually
>> prevent a compat task from issueing KVM ioctls.
>>
>> This patch changes this behaviour, by always registering a compat_ioctl
>> method, even if KVM_COMPAT is not selected. In that case, the callback
>> will always return -EINVAL.
>>
>> Reported-by: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> I virt/kvm/Kconfig we have:
> 
> config KVM_COMPAT
>        def_bool y
>        depends on KVM && COMPAT && !S390
> 
> ... and in arch/s390 we have COMPAT support, so does this potentially break
> anything there?

It doesn't seem to (COMPAT seems to support the 31 bit stuff, which I
don't think ever had KVM support), but my s390-foo is quite basic.

Christian, could you help here?

Thanks,

	M.

> 
> Thanks,
> Mark.
> 
>> ---
>>  virt/kvm/kvm_main.c | 19 +++++++++----------
>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index ada21f47f22b..8b47507faab5 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -116,6 +116,11 @@ static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
>>  #ifdef CONFIG_KVM_COMPAT
>>  static long kvm_vcpu_compat_ioctl(struct file *file, unsigned int ioctl,
>>  				  unsigned long arg);
>> +#define KVM_COMPAT(c)	.compat_ioctl	= (c)
>> +#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
>>  #endif
>>  static int hardware_enable_all(void);
>>  static void hardware_disable_all(void);
>> @@ -2396,11 +2401,9 @@ static int kvm_vcpu_release(struct inode *inode, struct file *filp)
>>  static struct file_operations kvm_vcpu_fops = {
>>  	.release        = kvm_vcpu_release,
>>  	.unlocked_ioctl = kvm_vcpu_ioctl,
>> -#ifdef CONFIG_KVM_COMPAT
>> -	.compat_ioctl   = kvm_vcpu_compat_ioctl,
>> -#endif
>>  	.mmap           = kvm_vcpu_mmap,
>>  	.llseek		= noop_llseek,
>> +	KVM_COMPAT(kvm_vcpu_compat_ioctl),
>>  };
>>  
>>  /*
>> @@ -2824,10 +2827,8 @@ static int kvm_device_release(struct inode *inode, struct file *filp)
>>  
>>  static const struct file_operations kvm_device_fops = {
>>  	.unlocked_ioctl = kvm_device_ioctl,
>> -#ifdef CONFIG_KVM_COMPAT
>> -	.compat_ioctl = kvm_device_ioctl,
>> -#endif
>>  	.release = kvm_device_release,
>> +	KVM_COMPAT(kvm_device_ioctl),
>>  };
>>  
>>  struct kvm_device *kvm_device_from_filp(struct file *filp)
>> @@ -3165,10 +3166,8 @@ static long kvm_vm_compat_ioctl(struct file *filp,
>>  static struct file_operations kvm_vm_fops = {
>>  	.release        = kvm_vm_release,
>>  	.unlocked_ioctl = kvm_vm_ioctl,
>> -#ifdef CONFIG_KVM_COMPAT
>> -	.compat_ioctl   = kvm_vm_compat_ioctl,
>> -#endif
>>  	.llseek		= noop_llseek,
>> +	KVM_COMPAT(kvm_vm_compat_ioctl),
>>  };
>>  
>>  static int kvm_dev_ioctl_create_vm(unsigned long type)
>> @@ -3259,8 +3258,8 @@ static long kvm_dev_ioctl(struct file *filp,
>>  
>>  static struct file_operations kvm_chardev_ops = {
>>  	.unlocked_ioctl = kvm_dev_ioctl,
>> -	.compat_ioctl   = kvm_dev_ioctl,
>>  	.llseek		= noop_llseek,
>> +	KVM_COMPAT(kvm_dev_ioctl),
>>  };
>>  
>>  static struct miscdevice kvm_dev = {
>> -- 
>> 2.17.1
>>


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

[PATCH 1/2] KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT

[Christian Borntraeger]

On 06/19/2018 12:10 PM, Marc Zyngier wrote:
> On 19/06/18 11:01, Mark Rutland wrote:
>> On Tue, Jun 19, 2018 at 10:42:50AM +0100, Marc Zyngier wrote:
>>> The current behaviour of the compat ioctls is a bit odd.
>>> We provide a compat_ioctl method when KVM_COMPAT is set, and NULL
>>> otherwise. But NULL means that the normal, non-compat ioctl should
>>> be used directly for compat tasks, and there is no way to actually
>>> prevent a compat task from issueing KVM ioctls.
>>>
>>> This patch changes this behaviour, by always registering a compat_ioctl
>>> method, even if KVM_COMPAT is not selected. In that case, the callback
>>> will always return -EINVAL.
>>>
>>> Reported-by: Mark Rutland <mark.rutland@arm.com>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>> I virt/kvm/Kconfig we have:
>>
>> config KVM_COMPAT
>>        def_bool y
>>        depends on KVM && COMPAT && !S390
>>
>> ... and in arch/s390 we have COMPAT support, so does this potentially break
>> anything there?
> 
> It doesn't seem to (COMPAT seems to support the 31 bit stuff, which I
> don't think ever had KVM support), but my s390-foo is quite basic.
> 
> Christian, could you help here?

We do not support KVM for 31 bit. So the original goal of the KVM_COMPAT stuff
was to actually disable the compat thing for the KVM device driver. 

See

commit de8e5d744051568c8aad ("KVM: Disable compat ioctl for s390").

Now looking back, this patch actually only disabled the compat wrappers and
still allows an 31bit process to run the 64bit ioctls. In theory this should
cause no issues as the 64bit ioctl must fence off all invalid input anyway
but actually forbidding KVM for compat processes is even better.



So I think this patch actually is a good thing.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

if you want you could even add a
Fixes: tag.

[PATCH 1/2] KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT

[Marc Zyngier]

On 19/06/18 12:24, Christian Borntraeger wrote:
> 
> 
> On 06/19/2018 12:10 PM, Marc Zyngier wrote:
>> On 19/06/18 11:01, Mark Rutland wrote:
>>> On Tue, Jun 19, 2018 at 10:42:50AM +0100, Marc Zyngier wrote:
>>>> The current behaviour of the compat ioctls is a bit odd.
>>>> We provide a compat_ioctl method when KVM_COMPAT is set, and NULL
>>>> otherwise. But NULL means that the normal, non-compat ioctl should
>>>> be used directly for compat tasks, and there is no way to actually
>>>> prevent a compat task from issueing KVM ioctls.
>>>>
>>>> This patch changes this behaviour, by always registering a compat_ioctl
>>>> method, even if KVM_COMPAT is not selected. In that case, the callback
>>>> will always return -EINVAL.
>>>>
>>>> Reported-by: Mark Rutland <mark.rutland@arm.com>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>
>>> I virt/kvm/Kconfig we have:
>>>
>>> config KVM_COMPAT
>>>        def_bool y
>>>        depends on KVM && COMPAT && !S390
>>>
>>> ... and in arch/s390 we have COMPAT support, so does this potentially break
>>> anything there?
>>
>> It doesn't seem to (COMPAT seems to support the 31 bit stuff, which I
>> don't think ever had KVM support), but my s390-foo is quite basic.
>>
>> Christian, could you help here?
> 
> We do not support KVM for 31 bit. So the original goal of the KVM_COMPAT stuff
> was to actually disable the compat thing for the KVM device driver. 
> 
> See
> 
> commit de8e5d744051568c8aad ("KVM: Disable compat ioctl for s390").
> 
> Now looking back, this patch actually only disabled the compat wrappers and
> still allows an 31bit process to run the 64bit ioctls. In theory this should
> cause no issues as the 64bit ioctl must fence off all invalid input anyway
> but actually forbidding KVM for compat processes is even better.
> 
> 
> 
> So I think this patch actually is a good thing.
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> if you want you could even add a
> Fixes: tag.

Added, thanks.

Radim, Paolo: are you happy with me taking this through the kvmarm tree?

Thanks,

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

[PATCH 1/2] KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT

[Radim Krčmář]

2018-06-21 12:40+0100, Marc Zyngier:
> Radim, Paolo: are you happy with me taking this through the kvmarm tree?

Yes, please do.

Acked-by: Radim Kr?m?? <rkrcmar@redhat.com>

[PATCH 2/2] KVM: arm64: Prevent KVM_COMPAT from being selected

[Marc Zyngier]

There is very little point in trying to support the 32bit KVM/arm API
on arm64, and this was never an anticipated use case.

Let's make it clear by not selecting KVM_COMPAT.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 72143cfaf6ec..ea434ddc8499 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -47,7 +47,7 @@ config KVM_GENERIC_DIRTYLOG_READ_PROTECT
 
 config KVM_COMPAT
        def_bool y
-       depends on KVM && COMPAT && !S390
+       depends on KVM && COMPAT && !(S390 || ARM64)
 
 config HAVE_KVM_IRQ_BYPASS
        bool
-- 
2.17.1

[PATCH 2/2] KVM: arm64: Prevent KVM_COMPAT from being selected

[Mark Rutland]

On Tue, Jun 19, 2018 at 10:42:51AM +0100, Marc Zyngier wrote:
> There is very little point in trying to support the 32bit KVM/arm API
> on arm64, and this was never an anticipated use case.
> 
> Let's make it clear by not selecting KVM_COMPAT.

Do any other architectures have KVM && COMPAT, and expose an
incompatible ABI to COMPAT tasks?

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Given the state of arm/arm64, this makes sense to me. FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

> ---
>  virt/kvm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> index 72143cfaf6ec..ea434ddc8499 100644
> --- a/virt/kvm/Kconfig
> +++ b/virt/kvm/Kconfig
> @@ -47,7 +47,7 @@ config KVM_GENERIC_DIRTYLOG_READ_PROTECT
>  
>  config KVM_COMPAT
>         def_bool y
> -       depends on KVM && COMPAT && !S390
> +       depends on KVM && COMPAT && !(S390 || ARM64)
>  
>  config HAVE_KVM_IRQ_BYPASS
>         bool
> -- 
> 2.17.1
>