KVM: fix missing check for memslot flags

This parameter will be used in the later patch

virt/kvm/kvm_main.c +3 -2

-	return hva_to_pfn(kvm, addr, atomic, async, write_fault, writable);
+	return hva_to_pfn(kvm, slot, addr, atomic, async, write_fault,
+			  writable);
 }
 
 pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn)

@@ -1205,7 +1210,7 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
 			 struct kvm_memory_slot *slot, gfn_t gfn)
 {
 	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
-	return hva_to_pfn(kvm, addr, false, NULL, true, NULL);
+	return hva_to_pfn(kvm, slot, addr, false, NULL, true, NULL);
 }
 
 pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,

Quoted 22 lines · Xiao Guangrong

This parameter will be used in the later patch

virt/kvm/kvm_main.c +3 -2

-	return hva_to_pfn(kvm, addr, atomic, async, write_fault, writable);
+	return hva_to_pfn(kvm, slot, addr, atomic, async, write_fault,
+			  writable);
 }
 
 pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn)

@@ -1205,7 +1210,7 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
 			 struct kvm_memory_slot *slot, gfn_t gfn)
 {
 	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
-	return hva_to_pfn(kvm, addr, false, NULL, true, NULL);
+	return hva_to_pfn(kvm, slot, addr, false, NULL, true, NULL);
 }
 
 pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,

@@ -1213,7 +1218,7 @@ pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
 {
 	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
 

virt/kvm/kvm_main.c pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm, · line 1222

Quoted 3 lines

virt/kvm/kvm_main.c +1 -1

-	return hva_to_pfn(kvm, addr, true, NULL, true, NULL);
+	return hva_to_pfn(kvm, slot, addr, true, NULL, true, NULL);
 }

It was unreadable before, but now it's even more unreadable.

This set of functions is only used to read data from host space, read is a special case in the later patch

virt/kvm/kvm_main.c +10 -0

+/*
+ * The hva returned by this function is only allowed to be read.
+ * It should pair with kvm_read_hva() or kvm_read_hva_atomic().
+ */
+static unsigned long gfn_to_hva_read(struct kvm *kvm, gfn_t gfn)
+{
+	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
+}
+
+static int kvm_read_hva(void *data, void *hva, int len)

Quoted 18 lines · Xiao Guangrong

This set of functions is only used to read data from host space, read is a special case in the later patch

virt/kvm/kvm_main.c +14 -0

+/*
+ * The hva returned by this function is only allowed to be read.
+ * It should pair with kvm_read_hva() or kvm_read_hva_atomic().
+ */
+static unsigned long gfn_to_hva_read(struct kvm *kvm, gfn_t gfn)
+{
+	return gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
+}
+
+static int kvm_read_hva(void *data, void *hva, int len)
+{
+	return __copy_from_user(data, (void __user *)hva, len);
+
+}

virt/kvm/kvm_main.c unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn) · line 1073

Quoted 6 lines

virt/kvm/kvm_main.c +6 -0

+
+static int kvm_read_hva_atomic(void *data, void *hva, int len)
+{
+	return __copy_from_user_inatomic(data, (void __user *)hva, len);
+
+}

Why cast to __user? Make it __user in the first place.

Also these are just simple wrappers, why to them at all?

In current code, if we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash

Address Avi's idea, we introduce readonly memory region to map ROM/ROMD to the guest

Signed-off-by Xiao Guangrong <[email protected]>

---
 Documentation/virtual/kvm/api.txt |    9 ++++--
 arch/x86/kvm/x86.c                |    1 +
 include/linux/kvm.h               |    4 ++-
 virt/kvm/kvm_main.c               |   61 ++++++++++++++++++++++++++++--------
 4 files changed, 57 insertions(+), 18 deletions(-)

Documentation/virtual/kvm/api.txt +2 -1

index 310fe50..a97ee90 100644
@@ -857,7 +857,8 @@ struct kvm_userspace_memory_region {
 };
 
 /* for kvm_memory_region::flags */
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
+#define KVM_MEM_LOG_DIRTY_PAGES	(1UL << 0)
+#define KVM_MEM_READONLY	(1UL << 1)
 
 This ioctl allows the user to create or modify a guest physical memory
 slot.  When changing an existing slot, it may be moved in the guest

Documentation/virtual/kvm/api.txt +28 -0

+static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
+{
+	if (unlikely(!(vma->vm_flags & VM_READ)))
+		return false;
+
+	if (write_fault && (unlikely(!(vma->vm_flags & VM_WRITE))))
+		return false;
+
+	return true;
+}
+
+static int hva_to_pfn_fast(unsigned long addr, bool atomic, bool *async,
+			bool slot_writable, bool *writable, struct page **page)
+{
+	int npages = 0;
+
+	if (!slot_writable)
+		return 0;
+
+	if (writable)
+		*writable = true;
+
+	if (atomic || async)
+		npages = __get_user_pages_fast(addr, 1, 1, page);
+
+	return npages;
+}
+
 static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 			unsigned long addr, bool atomic, bool *async,
 			bool write_fault, bool *writable)

Quoted 6 lines · Xiao Guangrong

In current code, if we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash

Address Avi's idea, we introduce readonly memory region to map ROM/ROMD to the guest

Please detail what is the idea in the changelog (a commit message should contain information or precise references to discussions).

Quoted 26 lines

Signed-off-by Xiao Guangrong <[email protected]>

---
 Documentation/virtual/kvm/api.txt |    9 ++++--
 arch/x86/kvm/x86.c                |    1 +
 include/linux/kvm.h               |    4 ++-
 virt/kvm/kvm_main.c               |   61 ++++++++++++++++++++++++++++--------
 4 files changed, 57 insertions(+), 18 deletions(-)

Documentation/virtual/kvm/api.txt +2 -1

index 310fe50..a97ee90 100644
@@ -857,7 +857,8 @@ struct kvm_userspace_memory_region {
 };
 
 /* for kvm_memory_region::flags */
-#define KVM_MEM_LOG_DIRTY_PAGES  1UL
+#define KVM_MEM_LOG_DIRTY_PAGES	(1UL << 0)
+#define KVM_MEM_READONLY	(1UL << 1)
 
 This ioctl allows the user to create or modify a guest physical memory
 slot.  When changing an existing slot, it may be moved in the guest

@@ -873,9 +874,11 @@ It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr
 be identical.  This allows large pages in the guest to be backed by large
 pages in the host.
 

Documentation/virtual/kvm/api.txt It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr · line 881

Quoted 7 lines

Documentation/virtual/kvm/api.txt +4 -2

-The flags field supports just one flag, KVM_MEM_LOG_DIRTY_PAGES, which
+The flags field supports two flag, KVM_MEM_LOG_DIRTY_PAGES, which
 instructs kvm to keep track of writes to memory within the slot.  See
-the KVM_GET_DIRTY_LOG ioctl.
+the KVM_GET_DIRTY_LOG ioctl. Another flag is KVM_MEM_READONLY, which
++indicates the guest memory is read-only, that means, guest is only allowed
++to read it. Writes will be posted to userspace as KVM_EXIT_MMIO exits.

Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with information about the fault?

Then perform this exit only if userspace allows it by explicit enable, and by default have the exit_read_fault handler jump to the mmio handler.

Quoted 37 lines

Documentation/virtual/kvm/api.txt +29 -0

+static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
+{
+	if (unlikely(!(vma->vm_flags & VM_READ)))
+		return false;
+
+	if (write_fault && (unlikely(!(vma->vm_flags & VM_WRITE))))
+		return false;
+
+	return true;
+}
+
+static int hva_to_pfn_fast(unsigned long addr, bool atomic, bool *async,
+			bool slot_writable, bool *writable, struct page **page)
+{
+	int npages = 0;
+
+	if (!slot_writable)
+		return 0;
+
+	if (writable)
+		*writable = true;
+
+	if (atomic || async)
+		npages = __get_user_pages_fast(addr, 1, 1, page);
+
+	return npages;
+}
+
 static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 			unsigned long addr, bool atomic, bool *async,
 			bool write_fault, bool *writable)

@@ -1105,18 +1140,16 @@ static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 	struct page *page[1];
 	int npages = 0;
 	pfn_t pfn;
+	bool slot_writable = !(slot->flags & KVM_MEM_READONLY);
 

Documentation/virtual/kvm/api.txt static pfn_t hva_to_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, · line 1149

Quoted 5 lines

Documentation/virtual/kvm/api.txt +1 -0

 	/* we can do it either atomically or asynchronously, not both */
 	BUG_ON(atomic && async);
 
 	BUG_ON(!write_fault && !writable);
+	BUG_ON(write_fault && !slot_writable);

Why BUG_ON on this condition?

Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with information about the fault?

Then perform this exit only if userspace allows it by explicit enable, and by default have the exit_read_fault handler jump to the mmio handler.

Quoted 3 lines · Marcelo Tosatti

Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with information about the fault?

I think you mean WRITE_FAULT.

But what's wrong with the normal mmio exit?

Quoted 3 lines

Then perform this exit only if userspace allows it by explicit enable, and by default have the exit_read_fault handler jump to the mmio handler.

I don't get this.

Quoted 6 lines · Avi Kivity

Quoted 3 lines · Marcelo Tosatti

Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with information about the fault?

I think you mean WRITE_FAULT.

Yes.

Quoted 1 line

But what's wrong with the normal mmio exit?

It is necessary to perform an address->mmio region lookup, to verify whether the mmio exit is due to an actual mmio (no memory slot) or from a write access to a write protected slot. That information is readily available in the kernel but is lost if the mmio exit is used to transmit the information.

Moreover, i'd argue the uses are different: one is an mmio emulation exit, the other is more like handling a pagefault in qemu.

Quoted 6 lines · Avi Kivity

Quoted 3 lines · Marcelo Tosatti

Can you introduce a separate exit reason, say KVM_EXIT_READ_FAULT, with information about the fault?

I think you mean WRITE_FAULT.

Yes.

Quoted 1 line

But what's wrong with the normal mmio exit?

It is necessary to perform an address->mmio region lookup, to verify whether the mmio exit is due to an actual mmio (no memory slot) or from a write access to a write protected slot. That information is readily available in the kernel but is lost if the mmio exit is used to transmit the information.

Moreover, i'd argue the uses are different: one is an mmio emulation exit, the other is more like handling a pagefault in qemu.

Quoted 7 lines

Quoted 4 lines

Then perform this exit only if userspace allows it by explicit enable, and by default have the exit_read_fault handler jump to the mmio handler.

I don't get this.

CAN USERSPACE HANDLE WRITE FAULT EXITS?
YES: WRITE FAULT EXIT.
NO: MMIO EXIT.

But then again userspace won't set read-only slots if it does not know about them. So it is not necessary.

In current code, if we map a readonly memory space from host to guest and the page is not currently mapped in the host, we will get a fault-pfn and async is not allowed, then the vm will crash

Address Avi's idea, we introduce readonly memory region to map ROM/ROMD to the guest

index 4b96bc2..b551db1 100644

virt/kvm/kvm_main.c +2 -2

-static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
-				     gfn_t *nr_pages)
+static unsigned long __gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
+				     gfn_t *nr_pages, bool write)

{

virt/kvm/kvm_main.c +2 -1

-	if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
+	if (!slot || slot->flags & KVM_MEMSLOT_INVALID ||
+	      ((slot->flags & KVM_MEM_READONLY) && write))
 		return bad_hva();
 
 	if (nr_pages)

virt/kvm/kvm_main.c void update_memslots(struct kvm_memslots *slots, struct kvm_memory_slot *new) · line 692

Quoted 5 lines

static int check_memory_region_flags(struct kvm_userspace_memory_region *mem) {

virt/kvm/kvm_main.c +1 -1

-	if (mem->flags & ~KVM_MEM_LOG_DIRTY_PAGES)
+	if (mem->flags & ~(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY))
 		return -EINVAL;

Only x86 supports readonly so far.

Quoted 16 lines

virt/kvm/kvm_main.c +4 -3

-static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
-				     gfn_t *nr_pages)
+static unsigned long __gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
+				     gfn_t *nr_pages, bool write)
 {
-	if (!slot || slot->flags & KVM_MEMSLOT_INVALID)
+	if (!slot || slot->flags & KVM_MEMSLOT_INVALID ||
+	      ((slot->flags & KVM_MEM_READONLY) && write))
 		return bad_hva();
 
 	if (nr_pages)

@@ -1045,6 +1046,12 @@ static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
 	return gfn_to_hva_memslot(slot, gfn);
 }
 

virt/kvm/kvm_main.c static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn, · line 1053

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

+static unsigned long gfn_to_hva_many(struct kvm_memory_slot *slot, gfn_t gfn,
+				     gfn_t *nr_pages)
+{
+	return __gfn_to_hva_many(slot, gfn, nr_pages, true);
+}

We have dozens of translation functions: read/write guest virtual, guest physical (nested and non nested), host virtual, host physical, atomic/nonatomic, sync/async, with/without slot lookup, and probably a few more I forgot.

I think we should refactor this into a series of on-step translations:

   /*
    * Translate gva/len write access to a number of tlb entries
    * (due to cross-page splits) or a fault
    */
   gva_to_tlb(gva, len, ACCESS_WRITE, &translation);
   /*
    * Translate tlb entries to callbacks that do I/O (either directly
    * or through KVM_EXIT_MMIO, provided there is no exception pending
    */
   tlb_to_io(&translation, &iolist, IO_ATOMIC);
   /*
    * Initiate I/O (if no exception)
    */
   run_iolist(&iolist, data);

   struct gpa_scatterlist {
       unsigned nr_entries;
       struct {
           gpa_t gpa;
           unsigned len;
       } entry[2];
       struct x86_exception exception;
   };

   struct kvm_iolist {
       unsigned nr_entries;
       struct kvm_ioentry {
           struct kvm_memslot *slot;  /* NULL for mmio */
           struct something *kernel_iodevice;
           gfn_t page_in_slot;
           unsigned offset_in_page;
           unsigned len;
           void (*iofunc)(struct kvm_ioentry *entry, void *data);
       } entry[2];
       struct x86_exception execption;
   };

This is of course outside the scope of this patchset, just something to think about (and write opinions on).