Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <11728bc1-7b59-1623-b517-d1a0d57eb275@intel.com>
 <20221219125204.1001149-1-zyytlz.wz@163.com> <20221220082255.GE30028@zhen-hp.sh.intel.com>
In-Reply-To: <20221220082255.GE30028@zhen-hp.sh.intel.com>
From:   Zheng Hacker <hackerzheng666@gmail.com>
Date:   Tue, 20 Dec 2022 17:03:41 +0800
Message-ID: <CAJedcCzD6Zc=ncxH5821OA=zL49bUFqD2hYT=TruU2AVt+_2hg@mail.gmail.com>
Subject: Re: [RESEND PATCH v4] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
To:     Zhenyu Wang <zhenyuw@linux.intel.com>
Cc:     Zheng Wang <zyytlz.wz@163.com>, zhi.a.wang@intel.com,
        alex000young@gmail.com, security@kernel.org,
        intel-gvt-dev@lists.freedesktop.org,
        tvrtko.ursulin@linux.intel.com, airlied@linux.ie,
        gregkh@linuxfoundation.org, intel-gfx@lists.freedesktop.org,
        joonas.lahtinen@linux.intel.com, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, 1002992920@qq.com, airlied@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Zhenyu Wang <zhenyuw@linux.intel.com> =E4=BA=8E2022=E5=B9=B412=E6=9C=8820=
=E6=97=A5=E5=91=A8=E4=BA=8C 16:25=E5=86=99=E9=81=93=EF=BC=9A
>
> On 2022.12.19 20:52:04 +0800, Zheng Wang wrote:
> > If intel_gvt_dma_map_guest_page failed, it will call
> >  ppgtt_invalidate_spt, which will finally free the spt. But the caller =
does
> >  not notice that, it will free spt again in error path.
> >
>
> It's not clear from this description which caller is actually wrong,
> better to clarify the problem in ppgtt_populate_spt_by_guest_entry() func=
tion.
>

Get it, will do in the next fix.


> >                                                  PAGE_SIZE, &dma_addr);
> > -             if (ret) {
> > -                     ppgtt_invalidate_spt(spt);
> > -                     return ret;
> > -             }
> > +             if (ret)
> > +                     goto err;
>
> I think it's fine to remove this and leave to upper caller, but again ple=
ase
> describe the behavior change in commit message as well, e.g to fix the sa=
nity
> of spt destroy that leaving previous invalidate and free of spt to caller=
 function
> instead of within callee function.

Sorry for my bad habit. Will do in the next version.

> >               sub_se.val64 =3D se->val64;
> >
> >               /* Copy the PAT field from PDE. */
> > @@ -1231,6 +1229,47 @@ static int split_2MB_gtt_entry(struct intel_vgpu=
 *vgpu,
> >       ops->set_pfn(se, sub_spt->shadow_page.mfn);
> >       ppgtt_set_shadow_entry(spt, se, index);
> >       return 0;
> > +err:
> > +     /* Undone the existing mappings of DMA addr. */
> > +     for_each_present_shadow_entry(spt, &e, parent_index) {
>
> sub_spt? We're undoing what's mapped for sub_spt right?

Yes, will change it to sub_spt in the next version.

>
> > +             switch (e.type) {
> > +             case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
> > +                     gvt_vdbg_mm("invalidate 4K entry\n");
> > +                     ppgtt_invalidate_pte(spt, &e);
> > +                     break;
> > +             case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
> > +                     /* We don't setup 64K shadow entry so far. */
> > +                     WARN(1, "suspicious 64K gtt entry\n");
> > +                     continue;
> > +             case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
> > +                     gvt_vdbg_mm("invalidate 2M entry\n");
> > +                     continue;
> > +             case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
> > +                     WARN(1, "GVT doesn't support 1GB page\n");
> > +                     continue;
> > +             case GTT_TYPE_PPGTT_PML4_ENTRY:
> > +             case GTT_TYPE_PPGTT_PDP_ENTRY:
> > +             case GTT_TYPE_PPGTT_PDE_ENTRY:
>
> I don't think this all entry type makes sense, as here we just split
> 2M entry for multiple 4K PTE entry.

I got it. I will leave the code for handling 4K PTE entry only.

>
> > +                     gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
> > +                     ret1 =3D ppgtt_invalidate_spt_by_shadow_entry(
> > +                                     spt->vgpu, &e);
> > +                     if (ret1) {
> > +                             gvt_vgpu_err("fail: shadow page %p shadow=
 entry 0x%llx type %d\n",
> > +                             spt, e.val64, e.type);
> > +                             goto free_spt;
> > +                     }
>
> for above reason, I don't think this is valid.

Got it.


Thanks for your carefully reviewing. I'll try to fix that in the coming pat=
ch.

Best regards,
Zheng Wang