Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
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
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
Wang, Zhi A 于2022年12月19日周一 16:22写道： > > I think it is a case-by-case thing. For example: > > The current scenario in this function looks like below: > > caller pass spt a > function > alloc spt b > something error > free spt a > return error > > The problem is: the function wrongly frees the spt a instead free what > it allocates. Thanks for your clear explaination. It’s really helpfult to me. I think I might know how to fix now. > A proper fix should be: > > caller pass spt a > function > alloc spt b > something error > *free spt b* > return error > As it's a case-by-case thing, I'll extract the un-done-mapping-dma part from ppgtt_invalidate_spt and put it in error path. Then I'll add the code of freeing new allocated spt. If I misunderstand your meaning, feel free to let me know. Working on a new fix now. Best regards, Zheng Wang
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
On 12/19/2022 9:57 AM, Zheng Wang wrote: > 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 > I think it is a case-by-case thing. For example: The current scenario in this function looks like below: caller pass spt a function alloc spt b something error free spt a return error The problem is: the function wrongly frees the spt a instead free what it allocates. A proper fix should be: caller pass spt a function alloc spt b something error *free spt b* return error Thanks, Zhi.
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
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
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
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 wrote:
>>> Dave Airlie 于2022年10月27日周四 08:01写道：
On Fri, 7 Oct 2022 at 11:38, 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
> Signed-off-by: Zheng Wang
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.
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
(+ 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 wrote: > > > > Dave Airlie 于2022年10月27日周四 08:01写道： > > > > > > On Fri, 7 Oct 2022 at 11:38, 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 > > > > Signed-off-by: Zheng Wang > > > > > > 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.
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
Dave Airlie 于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
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
On Thu, 27 Oct 2022 at 13:26, Zheng Hacker wrote: > > Dave Airlie 于2022年10月27日周四 08:01写道： > > > > On Fri, 7 Oct 2022 at 11:38, 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 > > > Signed-off-by: Zheng Wang > > > > 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.
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
Dave Airlie 于2022年10月27日周四 08:01写道： > > On Fri, 7 Oct 2022 at 11:38, 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 > > Signed-off-by: Zheng Wang > > 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
Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
On Fri, 7 Oct 2022 at 11:38, 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
> Signed-off-by: Zheng Wang
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/20221006165845.1735393-1-zyytlz...@163.com/
>
> v1: https://lore.kernel.org/all/20220928033340.1063949-1-zyytlz...@163.com/
> ---
> 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(>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, , 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
>
[Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry
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
Signed-off-by: Zheng Wang
---
v3:
- correct spelling mistake and remove unused variable suggested by Greg
v2: https://lore.kernel.org/all/20221006165845.1735393-1-zyytlz...@163.com/
v1: https://lore.kernel.org/all/20220928033340.1063949-1-zyytlz...@163.com/
---
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(>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, , 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
