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@xxxxxxx>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>
> 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...
