Re: [PATCH 26/27] Enable 32bit dirty log pointers on 64bit host

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 29 September 2009, Avi Kivity wrote:
> >
> >         r = -EINVAL;
> >         if (log->slot >= KVM_MEMORY_SLOTS)
> > @@ -718,8 +719,15 @@ int kvm_get_dirty_log(struct kvm *kvm,
> >         for (i = 0; !any && i < n/sizeof(long); ++i)
> >                 any = memslot->dirty_bitmap[i];
> >
> > +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
> > +       /* Need to convert user pointers */
> > +       if (test_thread_flag(TIF_32BIT))
> > +               target_bm = (void*)((u64)log->dirty_bitmap >> 32);
> > +       else
> > +#endif
> > +       target_bm = log->dirty_bitmap;
> >         r = -EFAULT;
> > -       if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
> > +       if (copy_to_user(target_bm, memslot->dirty_bitmap, n))
> >                 goto out;
> >
> >         if (any)
> 
> Ah, that's much better.  Plus a mental note not to put pointers in 
> user-visible structures in the future.  This can serve as a reminder :)

It's still broken on s390, which 

1. uses TIF_31BIT instead of TIF_32BIT
2. needs to call compat_ptr() to do a real conversion instead of a cast

The TIF_32BIT method is also not reliable. E.g. on x86_64 you are supposed
to get the 32 bit ABI when calling through INT80 instead of syscall/sysenter,
independent of the value of TIF_32BIT.

A better way to do this is to add a separate compat_ioctl() method that
converts this for you.

The patch below is an example for the canonical way to do this. Not tested!

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 897bff3..20f88ad 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2297,6 +2297,49 @@ out:
 	return r;
 }
 
+#ifdef CONFIG_COMPAT
+struct compat_kvm_dirty_log {
+	__u32 slot;
+	__u32 padding1;
+	union {
+		compat_uptr_t dirty_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
+static long kvm_vm_compat_ioctl(struct file *filp,
+			   unsigned int ioctl, unsigned long arg)
+{
+	struct kvm *kvm = filp->private_data;
+	int r;
+
+	if (kvm->mm != current->mm)
+		return -EIO;
+	switch (ioctl) {
+	case KVM_GET_DIRTY_LOG: {
+		struct compat_kvm_dirty_log compat_log;
+		struct kvm_dirty_log log;
+
+		r = -EFAULT;
+		if (copy_from_user(&compat_log, (void __user *)arg, sizeof log))
+			goto out;
+		log.slot	 = compat_log.slot;
+		log.padding1	 = compat_log.padding1;
+		log.padding2	 = compat_log.padding2;
+		log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap);
+
+		r = kvm_vm_ioctl_get_dirty_log(kvm, &log.log);
+		if (r)
+			goto out;
+		break;
+	default:
+		r = kvm_vm_ioctl(filp, ioctl, arg);
+	}
+
+	return r;
+}
+#endif
+
 static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct page *page[1];
@@ -2331,7 +2374,7 @@ static int kvm_vm_mmap(struct file *file, struct vm_area_struct *vma)
 static struct file_operations kvm_vm_fops = {
 	.release        = kvm_vm_release,
 	.unlocked_ioctl = kvm_vm_ioctl,
-	.compat_ioctl   = kvm_vm_ioctl,
+	.compat_ioctl   = kvm_vm_compat_ioctl,
 	.mmap           = kvm_vm_mmap,
 };