Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
> That is a horrible way to make an api (and it should be a bool too.) > Now every time you see this call in the code, you have to go look up > what the last parameter means. Just make 2 functions, one that does the > "is error" thing, and one that does not, and that will be much easier to > maintain and understand for the next 10+ years. Got it. I'll figure out anothr way. :) Thanks, Zheng Wang
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
On Wed, Sep 28, 2022 at 11:33:40AM +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.
>
> Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
>
> Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
> Reported-by: Zheng Wang
> Signed-off-by: Zheng Wang
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index ce0eb03709c3..550519f0acca 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct
> intel_vgpu_ppgtt_spt *spt)
> return atomic_dec_return(>refcount);
> }
>
> -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
> +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> is_error);
That is a horrible way to make an api (and it should be a bool too.)
Now every time you see this call in the code, you have to go look up
what the last parameter means. Just make 2 functions, one that does the
"is error" thing, and one that does not, and that will be much easier to
maintain and understand for the next 10+ years.
thanks,
greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
I've sent it using git send-email with another email account (zyytlz...@163.com)
Regards,
Zheng Wang
Jani Nikula 于2022年9月19日周一 17:30写道：
>
> On Mon, 19 Sep 2022, Zheng Wang <1002992...@qq.com> wrote:
> > From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
> > From: xmzyshypnc <1002992...@qq.com>
> > Date: Fri, 16 Sep 2022 23:48:23 +0800
> > Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
> >
> > There is a double-free security bug in split_2MB_gtt_entry.
> >
> > Here is a calling chain :
> > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> >
> > If intel_gvt_dma_map_guest_page failed, it will call
> > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> > kfree(spt). But the caller does not notice that, and it will call
> > ppgtt_free_spt again in error path.
> >
> > Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
> >
> > Signed-off-by: xmzyshypnc <1002992...@qq.com>
>
> Please use git send-email. The patch is whitespace broken and line
> wrapped, making it unusable.
>
> BR,
> Jani.
>
>
> > ---
> > drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> > index ce0eb03709c3..550519f0acca 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct
> > intel_vgpu_ppgtt_spt *spt)
> > return atomic_dec_return(>refcount);
> > }
> >
> > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
> > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> > is_error);
> >
> > static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> > struct intel_gvt_gtt_entry *e)
> > @@ -995,7 +995,7 @@ static int
> > ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> > ops->get_pfn(e));
> > return -ENXIO;
> > }
> > -return ppgtt_invalidate_spt(s);
> > +return ppgtt_invalidate_spt(s, 0);
> > }
> >
> > static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
> > @@ -1016,7 +1016,7 @@ static inline void ppgtt_invalidate_pte(struct
> > intel_vgpu_ppgtt_spt *spt,
> > intel_gvt_dma_unmap_guest_page(vgpu, pfn << PAGE_SHIFT);
> > }
> >
> > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
> > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> > is_error)
> > {
> > struct intel_vgpu *vgpu = spt->vgpu;
> > struct intel_gvt_gtt_entry e;
> > @@ -1059,9 +1059,11 @@ static int ppgtt_invalidate_spt(struct
> > intel_vgpu_ppgtt_spt *spt)
> > }
> > }
> >
> > -trace_spt_change(spt->vgpu->id, "release", spt,
> > +if (!is_error) {
> > +trace_spt_change(spt->vgpu->id, "release", spt,
> >spt->guest_page.gfn, spt->shadow_page.type);
> > -ppgtt_free_spt(spt);
> > +ppgtt_free_spt(spt);
> > +}
> > return 0;
> > fail:
> > gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
> > @@ -1215,7 +1217,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> > *vgpu,
> > ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
> > PAGE_SIZE, _addr);
> > if (ret) {
> > -ppgtt_invalidate_spt(spt);
> > +ppgtt_invalidate_spt(spt, 1);
> > return ret;
> > }
> > sub_se.val64 = se->val64;
> > @@ -1393,7 +1395,7 @@ static int ppgtt_handle_guest_entry_removal(struct
> > intel_vgpu_ppgtt_spt *spt,
> > ret = -ENXIO;
> > goto fail;
> > }
> > -ret = ppgtt_invalidate_spt(s);
> > +ret = ppgtt_invalidate_spt(s, 0);
> > if (ret)
> > goto fail;
> > } else {
>
> --
> Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
Got it. I'll try again later.
Best Regards,
Zheng Wang
Jani Nikula 于2022年9月19日周一 17:30写道：
>
> On Mon, 19 Sep 2022, Zheng Wang <1002992...@qq.com> wrote:
> > From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
> > From: xmzyshypnc <1002992...@qq.com>
> > Date: Fri, 16 Sep 2022 23:48:23 +0800
> > Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
> >
> > There is a double-free security bug in split_2MB_gtt_entry.
> >
> > Here is a calling chain :
> > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> >
> > If intel_gvt_dma_map_guest_page failed, it will call
> > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> > kfree(spt). But the caller does not notice that, and it will call
> > ppgtt_free_spt again in error path.
> >
> > Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
> >
> > Signed-off-by: xmzyshypnc <1002992...@qq.com>
>
> Please use git send-email. The patch is whitespace broken and line
> wrapped, making it unusable.
>
> BR,
> Jani.
>
>
> > ---
> > drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> > index ce0eb03709c3..550519f0acca 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct
> > intel_vgpu_ppgtt_spt *spt)
> > return atomic_dec_return(>refcount);
> > }
> >
> > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
> > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> > is_error);
> >
> > static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> > struct intel_gvt_gtt_entry *e)
> > @@ -995,7 +995,7 @@ static int
> > ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> > ops->get_pfn(e));
> > return -ENXIO;
> > }
> > -return ppgtt_invalidate_spt(s);
> > +return ppgtt_invalidate_spt(s, 0);
> > }
> >
> > static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
> > @@ -1016,7 +1016,7 @@ static inline void ppgtt_invalidate_pte(struct
> > intel_vgpu_ppgtt_spt *spt,
> > intel_gvt_dma_unmap_guest_page(vgpu, pfn << PAGE_SHIFT);
> > }
> >
> > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
> > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> > is_error)
> > {
> > struct intel_vgpu *vgpu = spt->vgpu;
> > struct intel_gvt_gtt_entry e;
> > @@ -1059,9 +1059,11 @@ static int ppgtt_invalidate_spt(struct
> > intel_vgpu_ppgtt_spt *spt)
> > }
> > }
> >
> > -trace_spt_change(spt->vgpu->id, "release", spt,
> > +if (!is_error) {
> > +trace_spt_change(spt->vgpu->id, "release", spt,
> >spt->guest_page.gfn, spt->shadow_page.type);
> > -ppgtt_free_spt(spt);
> > +ppgtt_free_spt(spt);
> > +}
> > return 0;
> > fail:
> > gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
> > @@ -1215,7 +1217,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> > *vgpu,
> > ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
> > PAGE_SIZE, _addr);
> > if (ret) {
> > -ppgtt_invalidate_spt(spt);
> > +ppgtt_invalidate_spt(spt, 1);
> > return ret;
> > }
> > sub_se.val64 = se->val64;
> > @@ -1393,7 +1395,7 @@ static int ppgtt_handle_guest_entry_removal(struct
> > intel_vgpu_ppgtt_spt *spt,
> > ret = -ENXIO;
> > goto fail;
> > }
> > -ret = ppgtt_invalidate_spt(s);
> > +ret = ppgtt_invalidate_spt(s, 0);
> > if (ret)
> > goto fail;
> > } else {
>
> --
> Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Here I introduced is_error to judge if the ppgtt_invalidate_spt is
called from good case or not. Only free spt in good case, leave it to
handle for the error path of caller.
Zheng Hacker 于2022年9月16日周五 14:39写道：
>
> From 8d95c1399e3ff345500a575e21254a73b0c89144 Mon Sep 17 00:00:00 2001
> From: xmzyshypnc <1002992...@qq.com>
> Date: Fri, 16 Sep 2022 14:37:48 +0800
> Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
>
> There is a double-free security bug in split_2MB_gtt_entry.
>
> Here is a calling chain :
> ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> If intel_gvt_dma_map_guest_page failed, it will call
> ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> kfree(spt). But the caller does not notice that, and it will call
> ppgtt_free_spt again in error path.
>
> Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
>
> Signed-off-by: xmzyshypnc <1002992...@qq.com>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 9f14fded8c0c..31d2a8d56384 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct
> intel_vgpu_ppgtt_spt *spt)
> return atomic_dec_return(>refcount);
> }
>
> -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
> +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *sptm,
> int is_error);
>
> static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> struct intel_gvt_gtt_entry *e)
> @@ -995,7 +995,7 @@ static int
> ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> ops->get_pfn(e));
> return -ENXIO;
> }
> - return ppgtt_invalidate_spt(s);
> + return ppgtt_invalidate_spt(s, 0);
> }
>
> static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
> @@ -1016,7 +1016,7 @@ static inline void ppgtt_invalidate_pte(struct
> intel_vgpu_ppgtt_spt *spt,
> intel_gvt_dma_unmap_guest_page(vgpu, pfn << PAGE_SHIFT);
> }
>
> -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
> +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> is_error)
> {
> struct intel_vgpu *vgpu = spt->vgpu;
> struct intel_gvt_gtt_entry e;
> @@ -1059,9 +1059,11 @@ static int ppgtt_invalidate_spt(struct
> intel_vgpu_ppgtt_spt *spt)
> }
> }
>
> - trace_spt_change(spt->vgpu->id, "release", spt,
> + if (!is_error) {
> + trace_spt_change(spt->vgpu->id, "release", spt,
> spt->guest_page.gfn, spt->shadow_page.type);
> - ppgtt_free_spt(spt);
> + ppgtt_free_spt(spt);
> + }
> return 0;
> fail:
> gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
> @@ -1215,7 +1217,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
> PAGE_SIZE, _addr);
> if (ret) {
> - ret = ppgtt_invalidate_spt(spt);
> + ret = ppgtt_invalidate_spt(spt, 1);
> return ret;
> }
> sub_se.val64 = se->val64;
> @@ -1393,7 +1395,7 @@ static int
> ppgtt_handle_guest_entry_removal(struct intel_vgpu_ppgtt_spt *spt,
> ret = -ENXIO;
> goto fail;
> }
> - ret = ppgtt_invalidate_spt(s);
> + ret = ppgtt_invalidate_spt(s, 0);
> if (ret)
> goto fail;
> } else {
> --
> 2.25.1
>
> Zheng Hacker 于2022年9月8日周四 19:59写道：
> >
> > Hi Greg,
> >
> > I got it, Greg.
> >
> > Mid-Autumn Festival is coming and I will have a couple of days off.
> > I'll see what I can do after holiday :)
> >
> > Regards,
> >
> > Zheng Wang
> >
> > 在 2022年9月8日星期四，Greg KH 写道：
> >>
> >> On Thu, Sep 08, 2022 at 05:09:40PM +0800, Zheng Hacker wrote:
> >> > Hi Zhenyu,
> >> >
> >> > This issue has been open for a few days. Could you plz write a patch
> >> > for that :) I'm not familiar with the logical code here.
> >>
> >> As this is only able to be hit in a theoretical system, it isn't that
> >> high of a priority, if any priority at all. Why not try to write a
> >> patch for it yourself to help resolve the issue faster?
> >>
> >> thanks,
> >>
> >> greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
I'll try using another mail client like Mutt later. :) Regards, Zheng Wang Greg KH 于2022年9月17日周六 17:07写道： > > On Fri, Sep 16, 2022 at 11:54:42PM +0800, Zheng Hacker wrote: > > >From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001 > > From: xmzyshypnc <1002992...@qq.com> > > Date: Fri, 16 Sep 2022 23:48:23 +0800 > > Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry > > > > There is a double-free security bug in split_2MB_gtt_entry. > > > > Here is a calling chain : > > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. > > > > If intel_gvt_dma_map_guest_page failed, it will call > > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and > > kfree(spt). But the caller does not notice that, and it will call > > ppgtt_free_spt again in error path. > > > > Fix this by only freeing spt in ppgtt_invalidate_spt in good case. > > > > Signed-off-by: Zheng Wang > > --- > > drivers/gpu/drm/i915/gvt/gtt.c | 16 +--- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > > index ce0eb03709c3..550519f0acca 100644 > > --- a/drivers/gpu/drm/i915/gvt/gtt.c > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > > @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct > > intel_vgpu_ppgtt_spt *spt) > > return atomic_dec_return(>refcount); > > } > > > > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt); > > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int > > is_error); > > > > static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu, > > struct intel_gvt_gtt_entry *e) > > @@ -995,7 +995,7 @@ static int > > ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu, > > Still line-wrapped and whitespace broken :( >
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
From: xmzyshypnc 1002992...@qq.com
Date: Fri, 16 Sep 2022 23:48:23 +0800
Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
There is a double-free security bug in split_2MB_gtt_entry.
Here is a calling chain :
ppgtt_populate_spt-ppgtt_populate_shadow_entry-split_2MB_gtt_entry.
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
kfree(spt). But the caller does not notice that, and it will call
ppgtt_free_spt again in error path.
Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
Signed-off-by: xmzyshypnc 1002992...@qq.com
---
drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index ce0eb03709c3..550519f0acca 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct intel_vgpu_ppgtt_spt
*spt)
return atomic_dec_return(spt-refcount);
}
-static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
is_error);
static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
struct intel_gvt_gtt_entry *e)
@@ -995,7 +995,7 @@ static int ppgtt_invalidate_spt_by_shadow_entry(struct
intel_vgpu *vgpu,
ops-get_pfn(e));
return -ENXIO;
}
- return ppgtt_invalidate_spt(s);
+ return ppgtt_invalidate_spt(s, 0);
}
static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
@@ -1016,7 +1016,7 @@ static inline void ppgtt_invalidate_pte(struct
intel_vgpu_ppgtt_spt *spt,
intel_gvt_dma_unmap_guest_page(vgpu, pfn PAGE_SHIFT);
}
-static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int is_error)
{
struct intel_vgpu *vgpu = spt-vgpu;
struct intel_gvt_gtt_entry e;
@@ -1059,9 +1059,11 @@ static int ppgtt_invalidate_spt(struct
intel_vgpu_ppgtt_spt *spt)
}
}
- trace_spt_change(spt-vgpu-id, "release", spt,
+ if (!is_error) {
+ trace_spt_change(spt-vgpu-id, "release", spt,
spt-guest_page.gfn, spt-shadow_page.type);
- ppgtt_free_spt(spt);
+ ppgtt_free_spt(spt);
+ }
return 0;
fail:
gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
@@ -1215,7 +1217,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, dma_addr);
if (ret) {
- ppgtt_invalidate_spt(spt);
+ ppgtt_invalidate_spt(spt, 1);
return ret;
}
sub_se.val64 = se-val64;
@@ -1393,7 +1395,7 @@ static int ppgtt_handle_guest_entry_removal(struct
intel_vgpu_ppgtt_spt *spt,
ret = -ENXIO;
goto fail;
}
- ret = ppgtt_invalidate_spt(s);
+ ret = ppgtt_invalidate_spt(s, 0);
if (ret)
goto fail;
} else {
--
2.25.1
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
>From 8d95c1399e3ff345500a575e21254a73b0c89144 Mon Sep 17 00:00:00 2001
From: xmzyshypnc <1002992...@qq.com>
Date: Fri, 16 Sep 2022 14:37:48 +0800
Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
There is a double-free security bug in split_2MB_gtt_entry.
Here is a calling chain :
ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
kfree(spt). But the caller does not notice that, and it will call
ppgtt_free_spt again in error path.
Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
Signed-off-by: xmzyshypnc <1002992...@qq.com>
---
drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 9f14fded8c0c..31d2a8d56384 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct
intel_vgpu_ppgtt_spt *spt)
return atomic_dec_return(>refcount);
}
-static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *sptm,
int is_error);
static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
struct intel_gvt_gtt_entry *e)
@@ -995,7 +995,7 @@ static int
ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
ops->get_pfn(e));
return -ENXIO;
}
- return ppgtt_invalidate_spt(s);
+ return ppgtt_invalidate_spt(s, 0);
}
static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
@@ -1016,7 +1016,7 @@ static inline void ppgtt_invalidate_pte(struct
intel_vgpu_ppgtt_spt *spt,
intel_gvt_dma_unmap_guest_page(vgpu, pfn << PAGE_SHIFT);
}
-static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int is_error)
{
struct intel_vgpu *vgpu = spt->vgpu;
struct intel_gvt_gtt_entry e;
@@ -1059,9 +1059,11 @@ static int ppgtt_invalidate_spt(struct
intel_vgpu_ppgtt_spt *spt)
}
}
- trace_spt_change(spt->vgpu->id, "release", spt,
+ if (!is_error) {
+ trace_spt_change(spt->vgpu->id, "release", spt,
spt->guest_page.gfn, spt->shadow_page.type);
- ppgtt_free_spt(spt);
+ ppgtt_free_spt(spt);
+ }
return 0;
fail:
gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
@@ -1215,7 +1217,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, _addr);
if (ret) {
- ret = ppgtt_invalidate_spt(spt);
+ ret = ppgtt_invalidate_spt(spt, 1);
return ret;
}
sub_se.val64 = se->val64;
@@ -1393,7 +1395,7 @@ static int
ppgtt_handle_guest_entry_removal(struct intel_vgpu_ppgtt_spt *spt,
ret = -ENXIO;
goto fail;
}
- ret = ppgtt_invalidate_spt(s);
+ ret = ppgtt_invalidate_spt(s, 0);
if (ret)
goto fail;
} else {
--
2.25.1
Zheng Hacker 于2022年9月8日周四 19:59写道：
>
> Hi Greg,
>
> I got it, Greg.
>
> Mid-Autumn Festival is coming and I will have a couple of days off.
> I'll see what I can do after holiday :)
>
> Regards,
>
> Zheng Wang
>
> 在 2022年9月8日星期四，Greg KH 写道：
>>
>> On Thu, Sep 08, 2022 at 05:09:40PM +0800, Zheng Hacker wrote:
>> > Hi Zhenyu,
>> >
>> > This issue has been open for a few days. Could you plz write a patch
>> > for that :) I'm not familiar with the logical code here.
>>
>> As this is only able to be hit in a theoretical system, it isn't that
>> high of a priority, if any priority at all. Why not try to write a
>> patch for it yourself to help resolve the issue faster?
>>
>> thanks,
>>
>> greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
>From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
From: xmzyshypnc <1002992...@qq.com>
Date: Fri, 16 Sep 2022 23:48:23 +0800
Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
There is a double-free security bug in split_2MB_gtt_entry.
Here is a calling chain :
ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
kfree(spt). But the caller does not notice that, and it will call
ppgtt_free_spt again in error path.
Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
Signed-off-by: Zheng Wang
---
drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index ce0eb03709c3..550519f0acca 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct
intel_vgpu_ppgtt_spt *spt)
return atomic_dec_return(>refcount);
}
-static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
is_error);
static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
struct intel_gvt_gtt_entry *e)
@@ -995,7 +995,7 @@ static int
ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
ops->get_pfn(e));
return -ENXIO;
}
- return ppgtt_invalidate_spt(s);
+ return ppgtt_invalidate_spt(s, 0);
}
static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
@@ -1016,7 +1016,7 @@ static inline void ppgtt_invalidate_pte(struct
intel_vgpu_ppgtt_spt *spt,
intel_gvt_dma_unmap_guest_page(vgpu, pfn << PAGE_SHIFT);
}
-static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int is_error)
{
struct intel_vgpu *vgpu = spt->vgpu;
struct intel_gvt_gtt_entry e;
@@ -1059,9 +1059,11 @@ static int ppgtt_invalidate_spt(struct
intel_vgpu_ppgtt_spt *spt)
}
}
- trace_spt_change(spt->vgpu->id, "release", spt,
+ if (!is_error) {
+ trace_spt_change(spt->vgpu->id, "release", spt,
spt->guest_page.gfn, spt->shadow_page.type);
- ppgtt_free_spt(spt);
+ ppgtt_free_spt(spt);
+ }
return 0;
fail:
gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
@@ -1215,7 +1217,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, _addr);
if (ret) {
- ppgtt_invalidate_spt(spt);
+ ppgtt_invalidate_spt(spt, 1);
return ret;
}
sub_se.val64 = se->val64;
@@ -1393,7 +1395,7 @@ static int
ppgtt_handle_guest_entry_removal(struct intel_vgpu_ppgtt_spt *spt,
ret = -ENXIO;
goto fail;
}
- ret = ppgtt_invalidate_spt(s);
+ ret = ppgtt_invalidate_spt(s, 0);
if (ret)
goto fail;
} else {
--
2.25.1
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hi Greg, Sorry for that. I’ll write another one. Regards, Zheng Wang 在 2022年9月17日星期六，Greg KH 写道： > On Fri, Sep 16, 2022 at 11:54:42PM +0800, Zheng Hacker wrote: > > >From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001 > > From: xmzyshypnc <1002992...@qq.com> > > Date: Fri, 16 Sep 2022 23:48:23 +0800 > > Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry > > > > There is a double-free security bug in split_2MB_gtt_entry. > > > > Here is a calling chain : > > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. > > > > If intel_gvt_dma_map_guest_page failed, it will call > > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and > > kfree(spt). But the caller does not notice that, and it will call > > ppgtt_free_spt again in error path. > > > > Fix this by only freeing spt in ppgtt_invalidate_spt in good case. > > > > Signed-off-by: Zheng Wang > > --- > > drivers/gpu/drm/i915/gvt/gtt.c | 16 +--- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/ > gtt.c > > index ce0eb03709c3..550519f0acca 100644 > > --- a/drivers/gpu/drm/i915/gvt/gtt.c > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > > @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct > > intel_vgpu_ppgtt_spt *spt) > > return atomic_dec_return(>refcount); > > } > > > > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt); > > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int > > is_error); > > > > static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu > *vgpu, > > struct intel_gvt_gtt_entry *e) > > @@ -995,7 +995,7 @@ static int > > ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu, > > Still line-wrapped and whitespace broken :( > >
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hi greg, Thanks for pointing that out. Working on it now :) Best wishes, Zheng Wang Greg KH 于2022年9月16日周五 16:25写道： > > On Fri, Sep 16, 2022 at 02:39:21PM +0800, Zheng Hacker wrote: > > >From 8d95c1399e3ff345500a575e21254a73b0c89144 Mon Sep 17 00:00:00 2001 > > From: xmzyshypnc <1002992...@qq.com> > > Date: Fri, 16 Sep 2022 14:37:48 +0800 > > Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry > > > > There is a double-free security bug in split_2MB_gtt_entry. > > > > Here is a calling chain : > > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. > > If intel_gvt_dma_map_guest_page failed, it will call > > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and > > kfree(spt). But the caller does not notice that, and it will call > > ppgtt_free_spt again in error path. > > > > Fix this by only freeing spt in ppgtt_invalidate_spt in good case. > > > > Signed-off-by: xmzyshypnc <1002992...@qq.com> > > --- > > drivers/gpu/drm/i915/gvt/gtt.c | 16 +--- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > > index 9f14fded8c0c..31d2a8d56384 100644 > > --- a/drivers/gpu/drm/i915/gvt/gtt.c > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > > @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct > > intel_vgpu_ppgtt_spt *spt) > > return atomic_dec_return(>refcount); > > } > > > > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt); > > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *sptm, > > int is_error); > > Your patch is whitespace damaged and linewrapped and can not be applied, > and can only barely read :( > > Please fix up your email client to not do this so that the change can be > properly reviewed and accepted if correct. > > thanks, > > greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
On Mon, 19 Sep 2022, Zheng Wang <1002992...@qq.com> wrote:
> From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
> From: xmzyshypnc <1002992...@qq.com>
> Date: Fri, 16 Sep 2022 23:48:23 +0800
> Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry
>
> There is a double-free security bug in split_2MB_gtt_entry.
>
> Here is a calling chain :
> ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
>
> If intel_gvt_dma_map_guest_page failed, it will call
> ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> kfree(spt). But the caller does not notice that, and it will call
> ppgtt_free_spt again in error path.
>
> Fix this by only freeing spt in ppgtt_invalidate_spt in good case.
>
> Signed-off-by: xmzyshypnc <1002992...@qq.com>
Please use git send-email. The patch is whitespace broken and line
wrapped, making it unusable.
BR,
Jani.
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index ce0eb03709c3..550519f0acca 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct
> intel_vgpu_ppgtt_spt *spt)
> return atomic_dec_return(>refcount);
> }
>
> -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
> +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> is_error);
>
> static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> struct intel_gvt_gtt_entry *e)
> @@ -995,7 +995,7 @@ static int
> ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> ops->get_pfn(e));
> return -ENXIO;
> }
> - return ppgtt_invalidate_spt(s);
> + return ppgtt_invalidate_spt(s, 0);
> }
>
> static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
> @@ -1016,7 +1016,7 @@ static inline void ppgtt_invalidate_pte(struct
> intel_vgpu_ppgtt_spt *spt,
> intel_gvt_dma_unmap_guest_page(vgpu, pfn << PAGE_SHIFT);
> }
>
> -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
> +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int
> is_error)
> {
> struct intel_vgpu *vgpu = spt->vgpu;
> struct intel_gvt_gtt_entry e;
> @@ -1059,9 +1059,11 @@ static int ppgtt_invalidate_spt(struct
> intel_vgpu_ppgtt_spt *spt)
> }
> }
>
> - trace_spt_change(spt->vgpu->id, "release", spt,
> + if (!is_error) {
> + trace_spt_change(spt->vgpu->id, "release", spt,
> spt->guest_page.gfn, spt->shadow_page.type);
> - ppgtt_free_spt(spt);
> + ppgtt_free_spt(spt);
> + }
> return 0;
> fail:
> gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
> @@ -1215,7 +1217,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> *vgpu,
> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
> PAGE_SIZE, _addr);
> if (ret) {
> - ppgtt_invalidate_spt(spt);
> + ppgtt_invalidate_spt(spt, 1);
> return ret;
> }
> sub_se.val64 = se->val64;
> @@ -1393,7 +1395,7 @@ static int ppgtt_handle_guest_entry_removal(struct
> intel_vgpu_ppgtt_spt *spt,
> ret = -ENXIO;
> goto fail;
> }
> - ret = ppgtt_invalidate_spt(s);
> + ret = ppgtt_invalidate_spt(s, 0);
> if (ret)
> goto fail;
> } else {
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
On Fri, Sep 16, 2022 at 11:54:42PM +0800, Zheng Hacker wrote: > >From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001 > From: xmzyshypnc <1002992...@qq.com> > Date: Fri, 16 Sep 2022 23:48:23 +0800 > Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry > > There is a double-free security bug in split_2MB_gtt_entry. > > Here is a calling chain : > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. > > If intel_gvt_dma_map_guest_page failed, it will call > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and > kfree(spt). But the caller does not notice that, and it will call > ppgtt_free_spt again in error path. > > Fix this by only freeing spt in ppgtt_invalidate_spt in good case. > > Signed-off-by: Zheng Wang > --- > drivers/gpu/drm/i915/gvt/gtt.c | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > index ce0eb03709c3..550519f0acca 100644 > --- a/drivers/gpu/drm/i915/gvt/gtt.c > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct > intel_vgpu_ppgtt_spt *spt) > return atomic_dec_return(>refcount); > } > > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt); > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt, int > is_error); > > static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu, > struct intel_gvt_gtt_entry *e) > @@ -995,7 +995,7 @@ static int > ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu, Still line-wrapped and whitespace broken :(
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
On Fri, Sep 16, 2022 at 02:39:21PM +0800, Zheng Hacker wrote: > >From 8d95c1399e3ff345500a575e21254a73b0c89144 Mon Sep 17 00:00:00 2001 > From: xmzyshypnc <1002992...@qq.com> > Date: Fri, 16 Sep 2022 14:37:48 +0800 > Subject: [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry > > There is a double-free security bug in split_2MB_gtt_entry. > > Here is a calling chain : > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. > If intel_gvt_dma_map_guest_page failed, it will call > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and > kfree(spt). But the caller does not notice that, and it will call > ppgtt_free_spt again in error path. > > Fix this by only freeing spt in ppgtt_invalidate_spt in good case. > > Signed-off-by: xmzyshypnc <1002992...@qq.com> > --- > drivers/gpu/drm/i915/gvt/gtt.c | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > index 9f14fded8c0c..31d2a8d56384 100644 > --- a/drivers/gpu/drm/i915/gvt/gtt.c > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > @@ -959,7 +959,7 @@ static inline int ppgtt_put_spt(struct > intel_vgpu_ppgtt_spt *spt) > return atomic_dec_return(>refcount); > } > > -static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt); > +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *sptm, > int is_error); Your patch is whitespace damaged and linewrapped and can not be applied, and can only barely read :( Please fix up your email client to not do this so that the change can be properly reviewed and accepted if correct. thanks, greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hi Greg, I got it, Greg. Mid-Autumn Festival is coming and I will have a couple of days off. I'll see what I can do after holiday :) Regards, Zheng Wang 在 2022年9月8日星期四，Greg KH 写道： > On Thu, Sep 08, 2022 at 05:09:40PM +0800, Zheng Hacker wrote: > > Hi Zhenyu, > > > > This issue has been open for a few days. Could you plz write a patch > > for that :) I'm not familiar with the logical code here. > > As this is only able to be hit in a theoretical system, it isn't that > high of a priority, if any priority at all. Why not try to write a > patch for it yourself to help resolve the issue faster? > > thanks, > > greg k-h >
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hi Zhenyu,
This issue has been open for a few days. Could you plz write a patch
for that :) I'm not familiar with the logical code here.
Regards,
Zheng Wang
Zhenyu Wang 于2022年9月7日周三 11:33写道：
>
> On 2022.09.06 19:36:56 +0800, Zheng Hacker wrote:
> > Hi Greg,
> >
> > Alex has explained how we figured out the patch. We did analyze the
> > code and found it possible to reach the vulnerability code. But we
> > have no physical device in hand to test the driver. So we'd like to
> > discuss with developers to see if the issue exists or not.
> >
> > Best regards,
> > Zheng Wang.
> >
> > Greg KH ???2022???9???5? 16:04?
> > >
> > > On Mon, Sep 05, 2022 at 03:46:09PM +0800, Zheng Hacker wrote:
> > > > I rewrote the letter. Hope it works.
> > > >
> > > > There is a double-free security bug in split_2MB_gtt_entry.
> > > >
> > > > Here is a calling chain :
> > > > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> > > > If intel_gvt_dma_map_guest_page failed, it will call
> > > > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> > > > kfree(spt). But the caller does not notice that, and it will call
> > > > ppgtt_free_spt again in error path.
> > > >
>
> It's a little mess in code so in theory it might be possible but
> intel_gvt_dma_map_guest_page won't fail in practise...
>
> > > > Fix this by returning the result of ppgtt_invalidate_spt to
> > > > split_2MB_gtt_entry.
> > > >
>
> I don't see why changing ret value can fix this issue, as it doesn't change
> any behavior e.g caller of ppgtt_populate_spt to handle possible different
> error return.
>
> As current code looks assuming that ppgtt_invalidate_spt would free spt in
> good case,
> I think the real cleanup should split that assumption and handle free in
> error case properly.
>
> > > > Signed-off-by: Zheng Wang
>
> This misses proper email address.
>
> thanks
>
> > > >
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > b/drivers/gpu/drm/i915/gvt/gtt.c
> > > > index ce0eb03709c3..9f14fded8c0c 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > > > @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> > > > *vgpu,
> > > > ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
> > > > sub_index,
> > > >PAGE_SIZE,
> > > > _addr);
> > > > if (ret) {
> > > > - ppgtt_invalidate_spt(spt);
> > > > + ret = ppgtt_invalidate_spt(spt);
> > > > return ret;
> > >
> > > But now you just lost the original error, shouldn't this succeed even if
> > > intel_gvt_dma_map_guest_page() failed?
> > >
> > > And how are you causing intel_gvt_dma_map_guest_page() to fail in a real
> > > system?
> > >
> > > thanks,
> > >
> > > greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hi Zhenyu,
Very glad for your reply. I agree that the bug is hard to trigger in
userspace. But it is possible to happen in some specific scene. For
example, if calling pfn_valid failed, the bug will be triggered. And
it did happened as the [1] commit description illustrates.
As for the patch, I think your plan is the best. We need to free the
spt only in bad case.
[1]
https://github.com/torvalds/linux/commit/39b4cbadb9a95bf3f13ea102d6ec841940916ee2
Regards,
Zheng Wang
Zhenyu Wang 于2022年9月7日周三 11:33写道：
>
> On 2022.09.06 19:36:56 +0800, Zheng Hacker wrote:
> > Hi Greg,
> >
> > Alex has explained how we figured out the patch. We did analyze the
> > code and found it possible to reach the vulnerability code. But we
> > have no physical device in hand to test the driver. So we'd like to
> > discuss with developers to see if the issue exists or not.
> >
> > Best regards,
> > Zheng Wang.
> >
> > Greg KH ???2022???9???5? 16:04?
> > >
> > > On Mon, Sep 05, 2022 at 03:46:09PM +0800, Zheng Hacker wrote:
> > > > I rewrote the letter. Hope it works.
> > > >
> > > > There is a double-free security bug in split_2MB_gtt_entry.
> > > >
> > > > Here is a calling chain :
> > > > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> > > > If intel_gvt_dma_map_guest_page failed, it will call
> > > > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> > > > kfree(spt). But the caller does not notice that, and it will call
> > > > ppgtt_free_spt again in error path.
> > > >
>
> It's a little mess in code so in theory it might be possible but
> intel_gvt_dma_map_guest_page won't fail in practise...
>
> > > > Fix this by returning the result of ppgtt_invalidate_spt to
> > > > split_2MB_gtt_entry.
> > > >
>
> I don't see why changing ret value can fix this issue, as it doesn't change
> any behavior e.g caller of ppgtt_populate_spt to handle possible different
> error return.
>
> As current code looks assuming that ppgtt_invalidate_spt would free spt in
> good case,
> I think the real cleanup should split that assumption and handle free in
> error case properly.
>
> > > > Signed-off-by: Zheng Wang
>
> This misses proper email address.
>
> thanks
>
> > > >
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > b/drivers/gpu/drm/i915/gvt/gtt.c
> > > > index ce0eb03709c3..9f14fded8c0c 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > > > @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> > > > *vgpu,
> > > > ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
> > > > sub_index,
> > > >PAGE_SIZE,
> > > > _addr);
> > > > if (ret) {
> > > > - ppgtt_invalidate_spt(spt);
> > > > + ret = ppgtt_invalidate_spt(spt);
> > > > return ret;
> > >
> > > But now you just lost the original error, shouldn't this succeed even if
> > > intel_gvt_dma_map_guest_page() failed?
> > >
> > > And how are you causing intel_gvt_dma_map_guest_page() to fail in a real
> > > system?
> > >
> > > thanks,
> > >
> > > greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
On Thu, Sep 08, 2022 at 05:09:40PM +0800, Zheng Hacker wrote: > Hi Zhenyu, > > This issue has been open for a few days. Could you plz write a patch > for that :) I'm not familiar with the logical code here. As this is only able to be hit in a theoretical system, it isn't that high of a priority, if any priority at all. Why not try to write a patch for it yourself to help resolve the issue faster? thanks, greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
On 2022.09.06 19:36:56 +0800, Zheng Hacker wrote:
> Hi Greg,
>
> Alex has explained how we figured out the patch. We did analyze the
> code and found it possible to reach the vulnerability code. But we
> have no physical device in hand to test the driver. So we'd like to
> discuss with developers to see if the issue exists or not.
>
> Best regards,
> Zheng Wang.
>
> Greg KH ???2022???9???5? 16:04?
> >
> > On Mon, Sep 05, 2022 at 03:46:09PM +0800, Zheng Hacker wrote:
> > > I rewrote the letter. Hope it works.
> > >
> > > There is a double-free security bug in split_2MB_gtt_entry.
> > >
> > > Here is a calling chain :
> > > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> > > If intel_gvt_dma_map_guest_page failed, it will call
> > > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> > > kfree(spt). But the caller does not notice that, and it will call
> > > ppgtt_free_spt again in error path.
> > >
It's a little mess in code so in theory it might be possible but
intel_gvt_dma_map_guest_page won't fail in practise...
> > > Fix this by returning the result of ppgtt_invalidate_spt to
> > > split_2MB_gtt_entry.
> > >
I don't see why changing ret value can fix this issue, as it doesn't change
any behavior e.g caller of ppgtt_populate_spt to handle possible different
error return.
As current code looks assuming that ppgtt_invalidate_spt would free spt in good
case,
I think the real cleanup should split that assumption and handle free in error
case properly.
> > > Signed-off-by: Zheng Wang
This misses proper email address.
thanks
> > >
> > > ---
> > > drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> > > b/drivers/gpu/drm/i915/gvt/gtt.c
> > > index ce0eb03709c3..9f14fded8c0c 100644
> > > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > > @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> > > *vgpu,
> > > ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
> > > sub_index,
> > >PAGE_SIZE, _addr);
> > > if (ret) {
> > > - ppgtt_invalidate_spt(spt);
> > > + ret = ppgtt_invalidate_spt(spt);
> > > return ret;
> >
> > But now you just lost the original error, shouldn't this succeed even if
> > intel_gvt_dma_map_guest_page() failed?
> >
> > And how are you causing intel_gvt_dma_map_guest_page() to fail in a real
> > system?
> >
> > thanks,
> >
> > greg k-h
signature.asc
Description: PGP signature
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hi everyone,
Now the letter is really plain-text now :)
Thanks Greg
Regards,
Zheng Wang
Zheng Hacker 于2022年9月5日周一 12:47写道：
>
> Hello everyone,
>
> I'm Zheng Wang. I found a potential double-free bug in
> drivers/gpu/drm/i915/gvt/gtt.c. I haven't been replied for a long time. So I
> decided to send it to more relavent supporters and developers to help to
> solve the problem.
>
> Best regards,
> Zheng Wang.
>
> xmzyshypnc <1002992...@qq.com> 于2022年9月4日周日 20:32写道：
>>
>> There is a double-free security bug in split_2MB_gtt_entry.
>>
>> Here is a calling chain :
>> ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. If
>> intel_gvt_dma_map_guest_page failed, it will call ppgtt_invalidate_spt,
>> which will finally call ppgtt_free_spt and kfree(spt). But the caller does
>> not notice that, and it will call ppgtt_free_spt again in error path.
>>
>> Fix this by returning the result of ppgtt_invalidate_spt to
>> split_2MB_gtt_entry.
>>
>> Signed-off-by: Zheng Wang <1002992...@qq.com>
>> ---
>> drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index ce0eb03709c3..9f14fded8c0c 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
>> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
>> sub_index,
>>PAGE_SIZE, _addr);
>> if (ret) {
>> - ppgtt_invalidate_spt(spt);
>> + ret = ppgtt_invalidate_spt(spt);
>> return ret;
>> }
>> sub_se.val64 = se->val64;
>> --
>> 2.25.1
>>
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Resent the mail for the former letter contains html text.
Regards,
Zheng Wang
Zheng Hacker 于2022年9月5日周一 12:47写道：
> Hello everyone,
>
> I'm Zheng Wang. I found a potential double-free bug
> in drivers/gpu/drm/i915/gvt/gtt.c. I haven't been replied for a long time.
> So I decided to send it to more relavent supporters and developers to help
> to solve the problem.
>
> Best regards,
> Zheng Wang.
>
> xmzyshypnc <1002992...@qq.com> 于2022年9月4日周日 20:32写道：
>
>> There is a double-free security bug in split_2MB_gtt_entry.
>>
>> Here is a calling chain :
>> ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. If
>> intel_gvt_dma_map_guest_page failed, it will call ppgtt_invalidate_spt,
>> which will finally call ppgtt_free_spt and kfree(spt). But the caller does
>> not notice that, and it will call ppgtt_free_spt again in error path.
>>
>> Fix this by returning the result of ppgtt_invalidate_spt to
>> split_2MB_gtt_entry.
>>
>> Signed-off-by: Zheng Wang <1002992...@qq.com>
>> ---
>> drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
>> b/drivers/gpu/drm/i915/gvt/gtt.c
>> index ce0eb03709c3..9f14fded8c0c 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
>> *vgpu,
>> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
>> sub_index,
>>PAGE_SIZE, _addr);
>> if (ret) {
>> - ppgtt_invalidate_spt(spt);
>> + ret = ppgtt_invalidate_spt(spt);
>> return ret;
>> }
>> sub_se.val64 = se->val64;
>> --
>> 2.25.1
>>
>>
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
I rewrote the letter. Hope it works.
There is a double-free security bug in split_2MB_gtt_entry.
Here is a calling chain :
ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
If intel_gvt_dma_map_guest_page failed, it will call
ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
kfree(spt). But the caller does not notice that, and it will call
ppgtt_free_spt again in error path.
Fix this by returning the result of ppgtt_invalidate_spt to split_2MB_gtt_entry.
Signed-off-by: Zheng Wang
---
drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index ce0eb03709c3..9f14fded8c0c 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
PAGE_SIZE, _addr);
if (ret) {
- ppgtt_invalidate_spt(spt);
+ ret = ppgtt_invalidate_spt(spt);
return ret;
}
sub_se.val64 = se->val64;
--
2.25.1
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hello everyone,
I'm Zheng Wang. I found a potential double-free bug
in drivers/gpu/drm/i915/gvt/gtt.c. I haven't been replied for a long time.
So I decided to send it to more relavent supporters and developers to help
to solve the problem.
Best regards,
Zheng Wang.
xmzyshypnc <1002992...@qq.com> 于2022年9月4日周日 20:32写道：
> There is a double-free security bug in split_2MB_gtt_entry.
>
> Here is a calling chain :
> ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry. If
> intel_gvt_dma_map_guest_page failed, it will call ppgtt_invalidate_spt,
> which will finally call ppgtt_free_spt and kfree(spt). But the caller does
> not notice that, and it will call ppgtt_free_spt again in error path.
>
> Fix this by returning the result of ppgtt_invalidate_spt to
> split_2MB_gtt_entry.
>
> Signed-off-by: Zheng Wang <1002992...@qq.com>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> b/drivers/gpu/drm/i915/gvt/gtt.c
> index ce0eb03709c3..9f14fded8c0c 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> *vgpu,
> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
> sub_index,
>PAGE_SIZE, _addr);
> if (ret) {
> - ppgtt_invalidate_spt(spt);
> + ret = ppgtt_invalidate_spt(spt);
> return ret;
> }
> sub_se.val64 = se->val64;
> --
> 2.25.1
>
>
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
Hi Greg,
Alex has explained how we figured out the patch. We did analyze the
code and found it possible to reach the vulnerability code. But we
have no physical device in hand to test the driver. So we'd like to
discuss with developers to see if the issue exists or not.
Best regards,
Zheng Wang.
Greg KH 于2022年9月5日周一 16:04写道：
>
> On Mon, Sep 05, 2022 at 03:46:09PM +0800, Zheng Hacker wrote:
> > I rewrote the letter. Hope it works.
> >
> > There is a double-free security bug in split_2MB_gtt_entry.
> >
> > Here is a calling chain :
> > ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> > If intel_gvt_dma_map_guest_page failed, it will call
> > ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> > kfree(spt). But the caller does not notice that, and it will call
> > ppgtt_free_spt again in error path.
> >
> > Fix this by returning the result of ppgtt_invalidate_spt to
> > split_2MB_gtt_entry.
> >
> > Signed-off-by: Zheng Wang
> >
> > ---
> > drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> > index ce0eb03709c3..9f14fded8c0c 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu
> > *vgpu,
> > ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
> > sub_index,
> >PAGE_SIZE, _addr);
> > if (ret) {
> > - ppgtt_invalidate_spt(spt);
> > + ret = ppgtt_invalidate_spt(spt);
> > return ret;
>
> But now you just lost the original error, shouldn't this succeed even if
> intel_gvt_dma_map_guest_page() failed?
>
> And how are you causing intel_gvt_dma_map_guest_page() to fail in a real
> system?
>
> thanks,
>
> greg k-h
Re: [Intel-gfx] [PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry.
On Mon, Sep 05, 2022 at 03:46:09PM +0800, Zheng Hacker wrote:
> I rewrote the letter. Hope it works.
>
> There is a double-free security bug in split_2MB_gtt_entry.
>
> Here is a calling chain :
> ppgtt_populate_spt->ppgtt_populate_shadow_entry->split_2MB_gtt_entry.
> If intel_gvt_dma_map_guest_page failed, it will call
> ppgtt_invalidate_spt, which will finally call ppgtt_free_spt and
> kfree(spt). But the caller does not notice that, and it will call
> ppgtt_free_spt again in error path.
>
> Fix this by returning the result of ppgtt_invalidate_spt to
> split_2MB_gtt_entry.
>
> Signed-off-by: Zheng Wang
>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index ce0eb03709c3..9f14fded8c0c 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1215,7 +1215,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn +
> sub_index,
>PAGE_SIZE, _addr);
> if (ret) {
> - ppgtt_invalidate_spt(spt);
> + ret = ppgtt_invalidate_spt(spt);
> return ret;
But now you just lost the original error, shouldn't this succeed even if
intel_gvt_dma_map_guest_page() failed?
And how are you causing intel_gvt_dma_map_guest_page() to fail in a real
system?
thanks,
greg k-h
