On Mon, 19 Sep 2022, Zheng Wang <[email protected]> wrote:
> From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
> From: xmzyshypnc <[email protected]>
> 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 <[email protected]>
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(&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 {
--
Jani Nikula, Intel Open Source Graphics Center
Got it. I'll try again later.
Best Regards,
Zheng Wang
Jani Nikula <[email protected]> 于2022年9月19日周一 17:30写道：
>
> On Mon, 19 Sep 2022, Zheng Wang <[email protected]> wrote:
> > From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
> > From: xmzyshypnc <[email protected]>
> > 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 <[email protected]>
>
> 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(&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 {
>
> --
> Jani Nikula, Intel Open Source Graphics Center
I've sent it using git send-email with another email account ([email protected])
Regards,
Zheng Wang
Jani Nikula <[email protected]> 于2022年9月19日周一 17:30写道：
>
> On Mon, 19 Sep 2022, Zheng Wang <[email protected]> wrote:
> > From afe79848cb74cc8e45ab426d13fa2394c87e0422 Mon Sep 17 00:00:00 2001
> > From: xmzyshypnc <[email protected]>
> > 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 <[email protected]>
>
> 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(&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 {
>
> --
> Jani Nikula, Intel Open Source Graphics Center
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 <[email protected]>
Signed-off-by: Zheng Wang <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Zheng Wang <[email protected]>
> ---
> 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);
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
> 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
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 spliting invalidate and free in ppgtt_invalidate_spt.
Only free spt when in good case.
Reported-by: Zheng Wang <[email protected]>
Signed-off-by: Zheng Wang <[email protected]>
---
v2:
- split initial function into two api function suggested by Greg
v1: https://lore.kernel.org/all/[email protected]/
---
drivers/gpu/drm/i915/gvt/gtt.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index ce0eb03709c3..55d8e1419302 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -959,6 +959,7 @@ static inline int ppgtt_put_spt(struct intel_vgpu_ppgtt_spt *spt)
return atomic_dec_return(&spt->refcount);
}
+static int ppgtt_invalidate_and_free_spt(struct intel_vgpu_ppgtt_spt *spt);
static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
@@ -995,7 +996,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_and_free_spt(s);
}
static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
@@ -1016,18 +1017,31 @@ 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_and_free_spt(struct intel_vgpu_ppgtt_spt *spt)
{
struct intel_vgpu *vgpu = spt->vgpu;
- struct intel_gvt_gtt_entry e;
- unsigned long index;
int ret;
trace_spt_change(spt->vgpu->id, "die", spt,
- spt->guest_page.gfn, spt->shadow_page.type);
-
+ spt->guest_page.gfn, spt->shadow_page.type);
if (ppgtt_put_spt(spt) > 0)
return 0;
+ ret = ppgtt_invalidate_spt(spt);
+ if (!ret) {
+ trace_spt_change(spt->vgpu->id, "release", spt,
+ spt->guest_page.gfn, spt->shadow_page.type);
+ ppgtt_free_spt(spt);
+ }
+
+ return ret;
+}
+
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
+{
+ struct intel_vgpu *vgpu = spt->vgpu;
+ struct intel_gvt_gtt_entry e;
+ unsigned long index;
+ int ret;
for_each_present_shadow_entry(spt, &e, index) {
switch (e.type) {
@@ -1059,9 +1073,6 @@ static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
}
}
- trace_spt_change(spt->vgpu->id, "release", spt,
- spt->guest_page.gfn, spt->shadow_page.type);
- ppgtt_free_spt(spt);
return 0;
fail:
gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
@@ -1393,7 +1404,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_and_free_spt(s);
if (ret)
goto fail;
} else {
--
2.25.1
On Fri, Oct 07, 2022 at 12:58:45AM +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 spliting invalidate and free in ppgtt_invalidate_spt.
> Only free spt when in good case.
>
> Reported-by: Zheng Wang <[email protected]>
> Signed-off-by: Zheng Wang <[email protected]>
> ---
> v2:
> - split initial function into two api function suggested by Greg
>
> v1: https://lore.kernel.org/all/[email protected]/
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index ce0eb03709c3..55d8e1419302 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -959,6 +959,7 @@ static inline int ppgtt_put_spt(struct intel_vgpu_ppgtt_spt *spt)
> return atomic_dec_return(&spt->refcount);
> }
>
> +static int ppgtt_invalidate_and_free_spt(struct intel_vgpu_ppgtt_spt *spt);
Odd extra space after the 'int', why?
Greg KH <[email protected]> 于2022年10月7日周五 03:22写道：
>
> On Fri, Oct 07, 2022 at 12:58:45AM +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 spliting invalidate and free in ppgtt_invalidate_spt.
> > Only free spt when in good case.
> >
> > Reported-by: Zheng Wang <[email protected]>
> > Signed-off-by: Zheng Wang <[email protected]>
> > ---
> > v2:
> > - split initial function into two api function suggested by Greg
> >
> > v1: https://lore.kernel.org/all/[email protected]/
> > ---
> > drivers/gpu/drm/i915/gvt/gtt.c | 31 +++++++++++++++++++++----------
> > 1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> > index ce0eb03709c3..55d8e1419302 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -959,6 +959,7 @@ static inline int ppgtt_put_spt(struct intel_vgpu_ppgtt_spt *spt)
> > return atomic_dec_return(&spt->refcount);
> > }
> >
> > +static int ppgtt_invalidate_and_free_spt(struct intel_vgpu_ppgtt_spt *spt);
>
> Odd extra space after the 'int', why?
>
Hi Greg,
Sorry it's a spelling mistake. I'll correct it right away :)
Thanks,
Zheng Wang
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 spliting invalidate and free in ppgtt_invalidate_spt.
Only free spt when in good case.
Reported-by: Zheng Wang <[email protected]>
Signed-off-by: Zheng Wang <[email protected]>
---
v3:
- correct spelling mistake and remove unused variable suggested by Greg
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
---
drivers/gpu/drm/i915/gvt/gtt.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index ce0eb03709c3..865d33762e45 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -959,6 +959,7 @@ static inline int ppgtt_put_spt(struct intel_vgpu_ppgtt_spt *spt)
return atomic_dec_return(&spt->refcount);
}
+static int ppgtt_invalidate_and_free_spt(struct intel_vgpu_ppgtt_spt *spt);
static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
@@ -995,7 +996,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_and_free_spt(s);
}
static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
@@ -1016,18 +1017,30 @@ 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_and_free_spt(struct intel_vgpu_ppgtt_spt *spt)
{
- struct intel_vgpu *vgpu = spt->vgpu;
- struct intel_gvt_gtt_entry e;
- unsigned long index;
int ret;
trace_spt_change(spt->vgpu->id, "die", spt,
- spt->guest_page.gfn, spt->shadow_page.type);
-
+ spt->guest_page.gfn, spt->shadow_page.type);
if (ppgtt_put_spt(spt) > 0)
return 0;
+ ret = ppgtt_invalidate_spt(spt);
+ if (!ret) {
+ trace_spt_change(spt->vgpu->id, "release", spt,
+ spt->guest_page.gfn, spt->shadow_page.type);
+ ppgtt_free_spt(spt);
+ }
+
+ return ret;
+}
+
+static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
+{
+ struct intel_vgpu *vgpu = spt->vgpu;
+ struct intel_gvt_gtt_entry e;
+ unsigned long index;
+ int ret;
for_each_present_shadow_entry(spt, &e, index) {
switch (e.type) {
@@ -1059,9 +1072,6 @@ static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
}
}
- trace_spt_change(spt->vgpu->id, "release", spt,
- spt->guest_page.gfn, spt->shadow_page.type);
- ppgtt_free_spt(spt);
return 0;
fail:
gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
@@ -1393,7 +1403,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_and_free_spt(s);
if (ret)
goto fail;
} else {
--
2.25.1
On Fri, 7 Oct 2022 at 11:38, Zheng Wang <[email protected]> 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 spliting invalidate and free in ppgtt_invalidate_spt.
> Only free spt when in good case.
>
> Reported-by: Zheng Wang <[email protected]>
> Signed-off-by: Zheng Wang <[email protected]>
Has this landed in a tree yet, since it's a possible CVE, might be
good to merge it somewhere.
Dave.
> ---
> v3:
> - correct spelling mistake and remove unused variable suggested by Greg
>
> v2: https://lore.kernel.org/all/[email protected]/
>
> v1: https://lore.kernel.org/all/[email protected]/
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 32 +++++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index ce0eb03709c3..865d33762e45 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -959,6 +959,7 @@ static inline int ppgtt_put_spt(struct intel_vgpu_ppgtt_spt *spt)
> return atomic_dec_return(&spt->refcount);
> }
>
> +static int ppgtt_invalidate_and_free_spt(struct intel_vgpu_ppgtt_spt *spt);
> static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt);
>
> static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> @@ -995,7 +996,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_and_free_spt(s);
> }
>
> static inline void ppgtt_invalidate_pte(struct intel_vgpu_ppgtt_spt *spt,
> @@ -1016,18 +1017,30 @@ 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_and_free_spt(struct intel_vgpu_ppgtt_spt *spt)
> {
> - struct intel_vgpu *vgpu = spt->vgpu;
> - struct intel_gvt_gtt_entry e;
> - unsigned long index;
> int ret;
>
> trace_spt_change(spt->vgpu->id, "die", spt,
> - spt->guest_page.gfn, spt->shadow_page.type);
> -
> + spt->guest_page.gfn, spt->shadow_page.type);
> if (ppgtt_put_spt(spt) > 0)
> return 0;
> + ret = ppgtt_invalidate_spt(spt);
> + if (!ret) {
> + trace_spt_change(spt->vgpu->id, "release", spt,
> + spt->guest_page.gfn, spt->shadow_page.type);
> + ppgtt_free_spt(spt);
> + }
> +
> + return ret;
> +}
> +
> +static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
> +{
> + struct intel_vgpu *vgpu = spt->vgpu;
> + struct intel_gvt_gtt_entry e;
> + unsigned long index;
> + int ret;
>
> for_each_present_shadow_entry(spt, &e, index) {
> switch (e.type) {
> @@ -1059,9 +1072,6 @@ static int ppgtt_invalidate_spt(struct intel_vgpu_ppgtt_spt *spt)
> }
> }
>
> - trace_spt_change(spt->vgpu->id, "release", spt,
> - spt->guest_page.gfn, spt->shadow_page.type);
> - ppgtt_free_spt(spt);
> return 0;
> fail:
> gvt_vgpu_err("fail: shadow page %p shadow entry 0x%llx type %d\n",
> @@ -1393,7 +1403,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_and_free_spt(s);
> if (ret)
> goto fail;
> } else {
> --
> 2.25.1
>
Dave Airlie <[email protected]> 于2022年10月27日周四 08:01写道：
>
> On Fri, 7 Oct 2022 at 11:38, Zheng Wang <[email protected]> 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 spliting invalidate and free in ppgtt_invalidate_spt.
> > Only free spt when in good case.
> >
> > Reported-by: Zheng Wang <[email protected]>
> > Signed-off-by: Zheng Wang <[email protected]>
>
> Has this landed in a tree yet, since it's a possible CVE, might be
> good to merge it somewhere.
>
> Dave.
>
Hi Dave,
This patched hasn't been merged yet. Could you please help with this?
Best Regards,
Zheng Wang
On Thu, 27 Oct 2022 at 13:26, Zheng Hacker <[email protected]> wrote:
>
> Dave Airlie <[email protected]> 于2022年10月27日周四 08:01写道：
> >
> > On Fri, 7 Oct 2022 at 11:38, Zheng Wang <[email protected]> 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 spliting invalidate and free in ppgtt_invalidate_spt.
> > > Only free spt when in good case.
> > >
> > > Reported-by: Zheng Wang <[email protected]>
> > > Signed-off-by: Zheng Wang <[email protected]>
> >
> > Has this landed in a tree yet, since it's a possible CVE, might be
> > good to merge it somewhere.
> >
> > Dave.
> >
>
> Hi Dave,
>
> This patched hasn't been merged yet. Could you please help with this?
I'll add some more people who can probably look at it.
Dave.
Dave Airlie <[email protected]> 于2022年10月27日周四 13:12写道：
> I'll add some more people who can probably look at it.
>
> Dave.
Got it, Thanks Dave.
Regards,
Zheng Wang
(+ Tvrtko as FYI)
Zhenyu, can you take a look at the patch ASAP.
Regards, Joonas
Quoting Dave Airlie (2022-10-27 08:12:31)
> On Thu, 27 Oct 2022 at 13:26, Zheng Hacker <[email protected]> wrote:
> >
> > Dave Airlie <[email protected]> 于2022年10月27日周四 08:01写道：
> > >
> > > On Fri, 7 Oct 2022 at 11:38, Zheng Wang <[email protected]> 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 spliting invalidate and free in ppgtt_invalidate_spt.
> > > > Only free spt when in good case.
> > > >
> > > > Reported-by: Zheng Wang <[email protected]>
> > > > Signed-off-by: Zheng Wang <[email protected]>
> > >
> > > Has this landed in a tree yet, since it's a possible CVE, might be
> > > good to merge it somewhere.
> > >
> > > Dave.
> > >
> >
> > Hi Dave,
> >
> > This patched hasn't been merged yet. Could you please help with this?
>
> I'll add some more people who can probably look at it.
>
> Dave.
On 12/15/2022 12:47 PM, Joonas Lahtinen wrote:
> (+ Tvrtko as FYI)
>
> Zhenyu, can you take a look at the patch ASAP.
>
> Regards, Joonas
Thanks so much for the reminding and patch.
Actually I don't think it is proper fix as:
split_2MB_gtt_entry() is going to allocate a new spt structure, which is
a PTE page to hold
the mapping of the 2MB. It will map the sub 4k pages for DMA addrs, form
them as PTE
entries, write the entries into the new PTE page, and then link the
page to the parent
table entry so that the GPU can reach it.
Now something wrong happens when mapping the sub 4K pages. What we need
are 1) The
existing mappings of DMA addr need to be un-done and 2) the newly
allocated spt structure
needs to be freed. These can be handle by ppgtt_invalidate_spt() which
will handle the 1)
and 2) based on the type of shadow page table, either recursively or
not. i.e. in this case,
it's a PTE page.
I guess the code wrongly does 1) 2) on the parent page table when
something error happens in
DMA mapping . You can fix it by releasing the newly allocated spt in the
error case and put a
Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support") in the
patch comment.
BTW: For sending the patches, you can take a look on "git send-email".
It will promise the correct
format and prevent quite some bumps. For email clients, if you feel mutt
is hard to ramp up,
you can try the Claws Mail. More information can be found in
Documentation/process/email-clients.rst
Thanks,
Zhi.
>
> Quoting Dave Airlie (2022-10-27 08:12:31)
>> On Thu, 27 Oct 2022 at 13:26, Zheng Hacker <[email protected]> wrote:
>>> Dave Airlie <[email protected]> 于2022年10月27日周四 08:01写道：
>>>> On Fri, 7 Oct 2022 at 11:38, Zheng Wang <[email protected]> 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 spliting invalidate and free in ppgtt_invalidate_spt.
>>>>> Only free spt when in good case.
>>>>>
>>>>> Reported-by: Zheng Wang <[email protected]>
>>>>> Signed-off-by: Zheng Wang <[email protected]>
>>>> Has this landed in a tree yet, since it's a possible CVE, might be
>>>> good to merge it somewhere.
>>>>
>>>> Dave.
>>>>
>>> Hi Dave,
>>>
>>> This patched hasn't been merged yet. Could you please help with this?
>> I'll add some more people who can probably look at it.
>>
>> Dave.
Hi Zhi,
Thanks for your reply and suggestion about fix. I am a little bit busy now.
I will review the code as soon as possible. Also thanks
Joonas for the reminding. We'll try to think out the new fix.
Best regards,
Zheng Wang
Hi Zhi,
Thanks again for your reply and clear explaination about the function.
I still have some doubt about the fix. Here is a invoke chain :
ppgtt_populate_spt
->ppgtt_populate_shadow_entry
->split_2MB_gtt_entry
As far as I'm concerned, when something error happens in DMA mapping,
which will make intel_gvt_dma_map_guest_page return none-zero code,
It will invoke ppgtt_invalidate_spt and call ppgtt_free_spt,which will
finally free spt by kfree. But the caller doesn't notice that and frees
spt by calling ppgtt_free_spt again. This is a typical UAF/Double Free
vulnerability. So I think the key point is about how to handle spt properly.
The handle newly allocated spt (aka sub_spt) is not the root cause of this
issue. Could you please give me more advice about how to fix this security
bug? Besides, I'm not sure if there are more similar problems in othe location.
Best regards,
Zheng Wang
--
2.25.1