[PATCH] drm/i915/gvt: Add carefully checking in GTT walker paths
Zhenyu Wang
zhenyuw at linux.intel.com
Thu Aug 3 02:27:00 UTC 2017
On 2017.08.02 15:06:37 +0800, changbin.du at intel.com wrote:
> From: Changbin Du <changbin.du at intel.com>
>
> 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 at intel.com>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 77 +++++++++++++++++++++++++++---------------
> drivers/gpu/drm/i915/gvt/gtt.h | 24 +++++++------
> 2 files changed, 64 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 6166e34..e397f5e 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/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)
Could we fix this function call by removing "e" as pass-in/out parameter but
only as return pointer for "struct intel_gvt_gtt_entry *" then use formal
kernel ERR_PTR() to return error and check?
> @@ -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)
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.h b/drivers/gpu/drm/i915/gvt/gtt.h
> index f88eb5e..abb41ee 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.h
> +++ b/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);
> --
> 2.7.4
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20170803/76295ff4/attachment.sig>
More information about the intel-gvt-dev
mailing list