drm/i915/gvt: Add carefully checking in GTT walker paths

When debugging the gtt code, found the intel_vgpu_gma_to_gpa() can
translate any given GMA though the GMA is not valid. This because
the GTT ops suppress the possible errors, which may result in an
invalid PT entry is retrieved by upper caller.

This patch changed the prototype of pte ops to propagate status to
callers. Then we make sure the GTT walker stop as early as when
a error is detected to prevent undefined behavior.
Signed-off-by: Changbin Du <changbin.du@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>

drivers/gpu/drm/i915/gvt/gtt.c

@@ -259,7 +259,7 @@ static void write_pte64(struct drm_i915_private *dev_priv,
 	writeq(pte, addr);
 }
 
-static inline struct intel_gvt_gtt_entry *gtt_get_entry64(void *pt,
+static inline int gtt_get_entry64(void *pt,
 		struct intel_gvt_gtt_entry *e,
 		unsigned long index, bool hypervisor_access, unsigned long gpa,
 		struct intel_vgpu *vgpu)
@@ -268,22 +268,23 @@ static inline struct intel_gvt_gtt_entry *gtt_get_entry64(void *pt,
 	int ret;
 
 	if (WARN_ON(info->gtt_entry_size != 8))
-		return e;
+		return -EINVAL;
 
 	if (hypervisor_access) {
 		ret = intel_gvt_hypervisor_read_gpa(vgpu, gpa +
 				(index << info->gtt_entry_size_shift),
 				&e->val64, 8);
-		WARN_ON(ret);
+		if (WARN_ON(ret))
+			return ret;
 	} else if (!pt) {
 		e->val64 = read_pte64(vgpu->gvt->dev_priv, index);
 	} else {
 		e->val64 = *((u64 *)pt + index);
 	}
-	return e;
+	return 0;
 }
 
-static inline struct intel_gvt_gtt_entry *gtt_set_entry64(void *pt,
+static inline int gtt_set_entry64(void *pt,
 		struct intel_gvt_gtt_entry *e,
 		unsigned long index, bool hypervisor_access, unsigned long gpa,
 		struct intel_vgpu *vgpu)
@@ -292,19 +293,20 @@ static inline struct intel_gvt_gtt_entry *gtt_set_entry64(void *pt,
 	int ret;
 
 	if (WARN_ON(info->gtt_entry_size != 8))
-		return e;
+		return -EINVAL;
 
 	if (hypervisor_access) {
 		ret = intel_gvt_hypervisor_write_gpa(vgpu, gpa +
 				(index << info->gtt_entry_size_shift),
 				&e->val64, 8);
-		WARN_ON(ret);
+		if (WARN_ON(ret))
+			return ret;
 	} else if (!pt) {
 		write_pte64(vgpu->gvt->dev_priv, index, e->val64);
 	} else {
 		*((u64 *)pt + index) = e->val64;
 	}
-	return e;
+	return 0;
 }
 
 #define GTT_HAW 46
@@ -445,21 +447,25 @@ static int gtt_entry_p2m(struct intel_vgpu *vgpu, struct intel_gvt_gtt_entry *p,
 /*
  * MM helpers.
  */
-struct intel_gvt_gtt_entry *intel_vgpu_mm_get_entry(struct intel_vgpu_mm *mm,
+int intel_vgpu_mm_get_entry(struct intel_vgpu_mm *mm,
 		void *page_table, struct intel_gvt_gtt_entry *e,
 		unsigned long index)
 {
 	struct intel_gvt *gvt = mm->vgpu->gvt;
 	struct intel_gvt_gtt_pte_ops *ops = gvt->gtt.pte_ops;
+	int ret;
 
 	e->type = mm->page_table_entry_type;
 
-	ops->get_entry(page_table, e, index, false, 0, mm->vgpu);
+	ret = ops->get_entry(page_table, e, index, false, 0, mm->vgpu);
+	if (ret)
+		return ret;
+
 	ops->test_pse(e);
-	return e;
+	return 0;
 }
 
-struct intel_gvt_gtt_entry *intel_vgpu_mm_set_entry(struct intel_vgpu_mm *mm,
+int intel_vgpu_mm_set_entry(struct intel_vgpu_mm *mm,
 		void *page_table, struct intel_gvt_gtt_entry *e,
 		unsigned long index)
 {
@@ -472,7 +478,7 @@ struct intel_gvt_gtt_entry *intel_vgpu_mm_set_entry(struct intel_vgpu_mm *mm,
 /*
  * PPGTT shadow page table helpers.
  */
-static inline struct intel_gvt_gtt_entry *ppgtt_spt_get_entry(
+static inline int ppgtt_spt_get_entry(
 		struct intel_vgpu_ppgtt_spt *spt,
 		void *page_table, int type,
 		struct intel_gvt_gtt_entry *e, unsigned long index,
@@ -480,20 +486,24 @@ static inline struct intel_gvt_gtt_entry *ppgtt_spt_get_entry(
 {
 	struct intel_gvt *gvt = spt->vgpu->gvt;
 	struct intel_gvt_gtt_pte_ops *ops = gvt->gtt.pte_ops;
+	int ret;
 
 	e->type = get_entry_type(type);
 
 	if (WARN(!gtt_type_is_entry(e->type), "invalid entry type\n"))
-		return e;
+		return -EINVAL;
 
-	ops->get_entry(page_table, e, index, guest,
+	ret = ops->get_entry(page_table, e, index, guest,
 			spt->guest_page.gfn << GTT_PAGE_SHIFT,
 			spt->vgpu);
+	if (ret)
+		return ret;
+
 	ops->test_pse(e);
-	return e;
+	return 0;
 }
 
-static inline struct intel_gvt_gtt_entry *ppgtt_spt_set_entry(
+static inline int ppgtt_spt_set_entry(
 		struct intel_vgpu_ppgtt_spt *spt,
 		void *page_table, int type,
 		struct intel_gvt_gtt_entry *e, unsigned long index,
@@ -503,7 +513,7 @@ static inline struct intel_gvt_gtt_entry *ppgtt_spt_set_entry(
 	struct intel_gvt_gtt_pte_ops *ops = gvt->gtt.pte_ops;
 
 	if (WARN(!gtt_type_is_entry(e->type), "invalid entry type\n"))
-		return e;
+		return -EINVAL;
 
 	return ops->set_entry(page_table, e, index, guest,
 			spt->guest_page.gfn << GTT_PAGE_SHIFT,
@@ -792,13 +802,13 @@ static struct intel_vgpu_ppgtt_spt *ppgtt_find_shadow_page(
 
 #define for_each_present_guest_entry(spt, e, i) \
 	for (i = 0; i < pt_entries(spt); i++) \
-	if (spt->vgpu->gvt->gtt.pte_ops->test_present( \
-		ppgtt_get_guest_entry(spt, e, i)))
+		if (!ppgtt_get_guest_entry(spt, e, i) && \
+		    spt->vgpu->gvt->gtt.pte_ops->test_present(e))
 
 #define for_each_present_shadow_entry(spt, e, i) \
 	for (i = 0; i < pt_entries(spt); i++) \
-	if (spt->vgpu->gvt->gtt.pte_ops->test_present( \
-		ppgtt_get_shadow_entry(spt, e, i)))
+		if (!ppgtt_get_shadow_entry(spt, e, i) && \
+		    spt->vgpu->gvt->gtt.pte_ops->test_present(e))
 
 static void ppgtt_get_shadow_page(struct intel_vgpu_ppgtt_spt *spt)
 {
@@ -1713,8 +1723,10 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma)
 		if (!vgpu_gmadr_is_valid(vgpu, gma))
 			goto err;
 
-		ggtt_get_guest_entry(mm, &e,
-			gma_ops->gma_to_ggtt_pte_index(gma));
+		ret = ggtt_get_guest_entry(mm, &e,
+				gma_ops->gma_to_ggtt_pte_index(gma));
+		if (ret)
+			goto err;
 		gpa = (pte_ops->get_pfn(&e) << GTT_PAGE_SHIFT)
 			+ (gma & ~GTT_PAGE_MASK);
 
@@ -1724,7 +1736,9 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma)
 
 	switch (mm->page_table_level) {
 	case 4:
-		ppgtt_get_shadow_root_entry(mm, &e, 0);
+		ret = ppgtt_get_shadow_root_entry(mm, &e, 0);
+		if (ret)
+			goto err;
 		gma_index[0] = gma_ops->gma_to_pml4_index(gma);
 		gma_index[1] = gma_ops->gma_to_l4_pdp_index(gma);
 		gma_index[2] = gma_ops->gma_to_pde_index(gma);
@@ -1732,15 +1746,19 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma)
 		index = 4;
 		break;
 	case 3:
-		ppgtt_get_shadow_root_entry(mm, &e,
+		ret = ppgtt_get_shadow_root_entry(mm, &e,
 				gma_ops->gma_to_l3_pdp_index(gma));
+		if (ret)
+			goto err;
 		gma_index[0] = gma_ops->gma_to_pde_index(gma);
 		gma_index[1] = gma_ops->gma_to_pte_index(gma);
 		index = 2;
 		break;
 	case 2:
-		ppgtt_get_shadow_root_entry(mm, &e,
+		ret = ppgtt_get_shadow_root_entry(mm, &e,
 				gma_ops->gma_to_pde_index(gma));
+		if (ret)
+			goto err;
 		gma_index[0] = gma_ops->gma_to_pte_index(gma);
 		index = 1;
 		break;
@@ -1755,6 +1773,11 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma)
 			(i == index - 1));
 		if (ret)
 			goto err;
+
+		if (!pte_ops->test_present(&e)) {
+			gvt_dbg_core("GMA 0x%lx is not present\n", gma);
+			goto err;
+		}
 	}
 
 	gpa = (pte_ops->get_pfn(&e) << GTT_PAGE_SHIFT)

drivers/gpu/drm/i915/gvt/gtt.h

@@ -49,14 +49,18 @@ struct intel_gvt_gtt_entry {
 };
 
 struct intel_gvt_gtt_pte_ops {
-	struct intel_gvt_gtt_entry *(*get_entry)(void *pt,
-		struct intel_gvt_gtt_entry *e,
-		unsigned long index, bool hypervisor_access, unsigned long gpa,
-		struct intel_vgpu *vgpu);
-	struct intel_gvt_gtt_entry *(*set_entry)(void *pt,
-		struct intel_gvt_gtt_entry *e,
-		unsigned long index, bool hypervisor_access, unsigned long gpa,
-		struct intel_vgpu *vgpu);
+	int (*get_entry)(void *pt,
+			 struct intel_gvt_gtt_entry *e,
+			 unsigned long index,
+			 bool hypervisor_access,
+			 unsigned long gpa,
+			 struct intel_vgpu *vgpu);
+	int (*set_entry)(void *pt,
+			 struct intel_gvt_gtt_entry *e,
+			 unsigned long index,
+			 bool hypervisor_access,
+			 unsigned long gpa,
+			 struct intel_vgpu *vgpu);
 	bool (*test_present)(struct intel_gvt_gtt_entry *e);
 	void (*clear_present)(struct intel_gvt_gtt_entry *e);
 	bool (*test_pse)(struct intel_gvt_gtt_entry *e);
@@ -143,12 +147,12 @@ struct intel_vgpu_mm {
 	struct intel_vgpu *vgpu;
 };
 
-extern struct intel_gvt_gtt_entry *intel_vgpu_mm_get_entry(
+extern int intel_vgpu_mm_get_entry(
 		struct intel_vgpu_mm *mm,
 		void *page_table, struct intel_gvt_gtt_entry *e,
 		unsigned long index);
 
-extern struct intel_gvt_gtt_entry *intel_vgpu_mm_set_entry(
+extern int intel_vgpu_mm_set_entry(
 		struct intel_vgpu_mm *mm,
 		void *page_table, struct intel_gvt_gtt_entry *e,
 		unsigned long index);