linux-kernel - Re: [PATCH v4 06/29] drm/i915/gvt: Explicitly check that vGPU is attached before shadowing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZMhjYo8bOsmsWcCL@yzhao56-desk.sh.intel.com>
Date:   Tue, 1 Aug 2023 09:44:02 +0800
From:   Yan Zhao <yan.y.zhao@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     Paolo Bonzini <pbonzini@...hat.com>,
        Zhenyu Wang <zhenyuw@...ux.intel.com>,
        Zhi Wang <zhi.a.wang@...el.com>, <kvm@...r.kernel.org>,
        <intel-gvt-dev@...ts.freedesktop.org>,
        <intel-gfx@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        Yongwei Ma <yongwei.ma@...el.com>,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [PATCH v4 06/29] drm/i915/gvt: Explicitly check that vGPU is
 attached before shadowing

On Fri, Jul 28, 2023 at 06:35:12PM -0700, Sean Christopherson wrote:
> Move the check that a vGPU is attacked from is_2MB_gtt_possible() all the
typo: "attacked" --> "attached"

> way up to shadow_ppgtt_mm() to avoid unnecessary work, and to make it more
This commit message does not match to what the patch does.
The check in the patch is in ppgtt_populate_shadow_entry().

What you want is like below?

@@ -1796,6 +1797,9 @@ static int shadow_ppgtt_mm(struct intel_vgpu_mm *mm)
        if (mm->ppgtt_mm.shadowed)
                return 0;

+       if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status))
+                return -EINVAL;
+
        mm->ppgtt_mm.shadowed = true;

        for (index = 0; index < ARRAY_SIZE(mm->ppgtt_mm.guest_pdps); index++) {

> obvious that a future cleanup of is_2MB_gtt_possible() isn't introducing a
> bug.
> 
> is_2MB_gtt_possible() has only one caller, ppgtt_populate_shadow_entry(),
> and all paths in ppgtt_populate_shadow_entry() eventually check for
> attachment by way of intel_gvt_dma_map_guest_page().
> 
> And of the paths that lead to ppgtt_populate_shadow_entry(),
> shadow_ppgtt_mm() is the only one that doesn't already check for
> INTEL_VGPU_STATUS_ACTIVE or INTEL_VGPU_STATUS_ATTACHED.

...

> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 5426a27c1b71..2aed31b497c9 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1163,8 +1163,6 @@ static int is_2MB_gtt_possible(struct intel_vgpu *vgpu,
>  	if (!HAS_PAGE_SIZES(vgpu->gvt->gt->i915, I915_GTT_PAGE_SIZE_2M))
>  		return 0;
>  
> -	if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status))
> -		return -EINVAL;
>  	pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry));
>  	if (is_error_noslot_pfn(pfn))
>  		return -EINVAL;
> @@ -1277,6 +1275,9 @@ static int ppgtt_populate_shadow_entry(struct intel_vgpu *vgpu,
>  	if (!pte_ops->test_present(ge))
>  		return 0;
>  
> +	if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status))
> +		return -EINVAL;
> +
>  	gfn = pte_ops->get_pfn(ge);
>  
>  	switch (ge->type) {
> -- 
> 2.41.0.487.g6d72f3e995-goog
>