| Message ID | 20180619094251.8586-2-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >>
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.
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.
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>
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 = {
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(-)