[PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt

[Zheng Wang]

A use-after-free bug may occur if init_imstt invokes framebuffer_release
and free the info ptr. The caller, imsttfb_probe didn't notice that and
still keep the ptr as private data in pdev.

If we remove the driver which will call imsttfb_remove to make cleanup,
UAF happens.

Fix it by return error code if bad case happens in init_imstt.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zheng Wang 
---
v2:
- add return error code in another location.
---
 drivers/video/fbdev/imsttfb.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index bea45647184e..975dd682fae4 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1347,7 +1347,7 @@ static const struct fb_ops imsttfb_ops = {
.fb_ioctl   = imsttfb_ioctl,
 };
 
-static void init_imstt(struct fb_info *info)
+static int init_imstt(struct fb_info *info)
 {
struct imstt_par *par = info->par;
__u32 i, tmp, *ip, *end;
@@ -1420,7 +1420,7 @@ static void init_imstt(struct fb_info *info)
|| !(compute_imstt_regvals(par, info->var.xres, info->var.yres))) {
printk("imsttfb: %ux%ux%u not supported\n", info->var.xres, 
info->var.yres, info->var.bits_per_pixel);
framebuffer_release(info);
-   return;
+   return -ENODEV;
}
 
sprintf(info->fix.id, "IMS TT (%s)", par->ramdac == IBM ? "IBM" : 
"TVP");
@@ -1456,12 +1456,13 @@ static void init_imstt(struct fb_info *info)
 
if (register_framebuffer(info) < 0) {
framebuffer_release(info);
-   return;
+   return -ENODEV;
}
 
tmp = (read_reg_le32(par->dc_regs, SSTATUS) & 0x0f00) >> 8;
fb_info(info, "%s frame buffer; %uMB vram; chip version %u\n",
info->fix.id, info->fix.smem_len >> 20, tmp);
+   return 0;
 }
 
 static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
@@ -1529,10 +1530,10 @@ static int imsttfb_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (!par->cmap_regs)
goto error;
info->pseudo_palette = par->palette;
-   init_imstt(info);
-
-   pci_set_drvdata(pdev, info);
-   return 0;
+   ret = init_imstt(info);
+   if (!ret)
+   pci_set_drvdata(pdev, info);
+   return ret;
 
 error:
if (par->dc_regs)
-- 
2.25.1

[PATCH] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt

[Zheng Wang]

A use-after-free bug may occur if init_imstt invokes framebuffer_release
and free the info ptr. The caller, imsttfb_probe didn't notice that and
still keeps the ptr as private data in pdev.

If we remove the driver which will call imsttfb_remove to make cleanup,
UAF happens.

Fix it by return error code if bad case happens in init_imstt.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zheng Wang 
---
 drivers/video/fbdev/imsttfb.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index bea45647184e..92b0c5833bda 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1347,7 +1347,7 @@ static const struct fb_ops imsttfb_ops = {
.fb_ioctl   = imsttfb_ioctl,
 };
 
-static void init_imstt(struct fb_info *info)
+static int init_imstt(struct fb_info *info)
 {
struct imstt_par *par = info->par;
__u32 i, tmp, *ip, *end;
@@ -1420,7 +1420,7 @@ static void init_imstt(struct fb_info *info)
|| !(compute_imstt_regvals(par, info->var.xres, info->var.yres))) {
printk("imsttfb: %ux%ux%u not supported\n", info->var.xres, 
info->var.yres, info->var.bits_per_pixel);
framebuffer_release(info);
-   return;
+   return -ENODEV;
}
 
sprintf(info->fix.id, "IMS TT (%s)", par->ramdac == IBM ? "IBM" : 
"TVP");
@@ -1529,10 +1529,10 @@ static int imsttfb_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (!par->cmap_regs)
goto error;
info->pseudo_palette = par->palette;
-   init_imstt(info);
-
-   pci_set_drvdata(pdev, info);
-   return 0;
+   ret = init_imstt(info);
+   if (!ret)
+   pci_set_drvdata(pdev, info);
+   return ret;
 
 error:
if (par->dc_regs)
-- 
2.25.1

[PATCH v2] drm/bridge: adv7511: fix use after free bug in adv7511_remove due to race condition

[Zheng Wang]

In adv7511_probe, adv7511->hpd_work is bound with adv7511_hpd_work.
adv7511_irq_process might be called to start the work.

If we call adv7511_remove with an unfinished work. There may be a
race condition. Here is the possible sequence:

CPU0  CPU1

 |adv7511_hpd_work
adv7511_remove   |
cec_devnode_release  |
cec_unregister_adapter|
cec_devnode_unregister|
put_device(&devnode->dev);|
cec_devnode_release |
cec_delete_adapter  |
kfree(adap);|
 |cec_phys_addr_invalidate
 |//use adap

Fix it by canceling the work before cleanup in adv7511_remove.

This is the patch with new title in order to clarify the bug. Old patch
is here. The root cause is the same as old one.

https://lore.kernel.org/all/20230316160548.1566989-1-zyytlz...@163.com/

Fixes: 518cb7057a59 ("drm/bridge: adv7511: Use work_struct to defer hotplug 
handing to out of irq context")
Signed-off-by: Zheng Wang 
---
v2:
- Fix space error from checkpacth script
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..e702a993fe6f 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1349,6 +1349,10 @@ static void adv7511_remove(struct i2c_client *i2c)
 {
struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
+   if (i2c->irq)
+   devm_free_irq(&i2c->dev, i2c->irq, adv7511);
+   cancel_work_sync(&adv7511->hpd_work);
+
adv7511_uninit_regulators(adv7511);
 
drm_bridge_remove(&adv7511->bridge);
-- 
2.25.1

[PATCH] drm/bridge: adv7511: fix use after free bug in adv7511_remove due to race condition

[Zheng Wang]

In adv7511_probe, adv7511->hpd_work is bound with adv7511_hpd_work.
adv7511_irq_process might be called to start the work.

If we call adv7511_remove with an unfinished work. There may be a
race condition. Here is the possible sequence:

CPU0  CPU1

 |adv7511_hpd_work
adv7511_remove   |
cec_devnode_release  |
cec_unregister_adapter|
cec_devnode_unregister|
put_device(&devnode->dev);|
cec_devnode_release |
cec_delete_adapter  |
kfree(adap);|
 |cec_phys_addr_invalidate
 |//use adap

Fix it by canceling the work before cleanup in adv7511_remove.

This is the patch with new title in order to clarify the bug. Old patch is here.
The root cause is the same as old one.

https://lore.kernel.org/all/20230316160548.1566989-1-zyytlz...@163.com/

Fixes: 518cb7057a59 ("drm/bridge: adv7511: Use work_struct to defer hotplug 
handing to out of irq context")
Signed-off-by: Zheng Wang 
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..e702a993fe6f 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1349,6 +1349,10 @@ static void adv7511_remove(struct i2c_client *i2c)
 {
struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
+   if (i2c->irq)
+   devm_free_irq(&i2c->dev, i2c->irq, adv7511);
+   cancel_work_sync(&adv7511->hpd_work);
+   
adv7511_uninit_regulators(adv7511);
 
drm_bridge_remove(&adv7511->bridge);
-- 
2.25.1

[PATCH v3] drm/bridge: adv7511: fix race condition bug in adv7511_remove due to unfinished work

[Zheng Wang]

In adv7511_probe, adv7511->hpd_work is bound with adv7511_hpd_work.
If we call adv7511_remove with a unfinished work. There may be a 
race condition where bridge->hpd_mutex was destroyed by 
drm_bridge_remove and used in adv7511_hpd_work in drm_bridge_hpd_notify.

Fix it by canceling the work before cleanup in adv7511_remove.
Fixes: 518cb7057a59 ("drm/bridge: adv7511: Use work_struct to defer hotplug 
handing to out of irq context")
Signed-off-by: Zheng Wang 
---
v3:
- add patch modification information
v2:
- add Fix label
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..9bf72dd6c1d3 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1349,6 +1349,7 @@ static void adv7511_remove(struct i2c_client *i2c)
 {
struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
+   cancel_work_sync(&adv7511->hpd_work);
adv7511_uninit_regulators(adv7511);
 
drm_bridge_remove(&adv7511->bridge);
-- 
2.25.1

[PATCH v2] drm/bridge: adv7511: fix race condition bug in adv7511_remove due to unfinished work

[Zheng Wang]

In adv7511_probe, adv7511->hpd_work is bound with adv7511_hpd_work.
If we call adv7511_remove with a unfinished work. There may be a 
race condition where bridge->hpd_mutex was destroyed by 
drm_bridge_remove and used in adv7511_hpd_work in drm_bridge_hpd_notify.

Fix it by canceling the work before cleanup in adv7511_remove.
Fixes: 518cb7057a59 ("drm/bridge: adv7511: Use work_struct to defer hotplug 
handing to out of irq context")
Signed-off-by: Zheng Wang 
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..9bf72dd6c1d3 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1349,6 +1349,7 @@ static void adv7511_remove(struct i2c_client *i2c)
 {
struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
+   cancel_work_sync(&adv7511->hpd_work);
adv7511_uninit_regulators(adv7511);
 
drm_bridge_remove(&adv7511->bridge);
-- 
2.25.1

[PATCH] drm/bridge: adv7511: fix race condition bug in adv7511_remove due to unfinished work

[Zheng Wang]

In adv7511_probe, adv7511->hpd_work is bound with adv7511_hpd_work.
If we call adv7511_remove with a unfinished work. There may be a 
race condition where bridge->hpd_mutex was destroyed by 
drm_bridge_remove and used in adv7511_hpd_work in drm_bridge_hpd_notify.

Fix it by canceling the work before cleanup in adv7511_remove.

Signed-off-by: Zheng Wang 
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..9bf72dd6c1d3 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1349,6 +1349,7 @@ static void adv7511_remove(struct i2c_client *i2c)
 {
struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
+   cancel_work_sync(&adv7511->hpd_work);
adv7511_uninit_regulators(adv7511);
 
drm_bridge_remove(&adv7511->bridge);
-- 
2.25.1

[PATCH] drm/nouveau/mmu: fix Use after Free bug in nvkm_vmm_node_split

[Zheng Wang]

Here is a function call chain.
nvkm_vmm_pfn_map->nvkm_vmm_pfn_split_merge->nvkm_vmm_node_split
If nvkm_vma_tail return NULL in nvkm_vmm_node_split, it will
finally invoke nvkm_vmm_node_merge->nvkm_vmm_node_delete, which
will free the vma. However, nvkm_vmm_pfn_map didn't notice that.
It goes into next label and UAF happens.

Fix it by returning the return-value of nvkm_vmm_node_merge
instead of NULL.

Signed-off-by: Zheng Wang 
---
 drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
index ae793f400ba1..84d6fc87b2e8 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
@@ -937,8 +937,8 @@ nvkm_vmm_node_split(struct nvkm_vmm *vmm,
if (vma->size != size) {
struct nvkm_vma *tmp;
if (!(tmp = nvkm_vma_tail(vma, vma->size - size))) {
-   nvkm_vmm_node_merge(vmm, prev, vma, NULL, vma->size);
-   return NULL;
+   tmp = nvkm_vmm_node_merge(vmm, prev, vma, NULL, 
vma->size);
+   return tmp;
}
tmp->part = true;
nvkm_vmm_node_insert(vmm, tmp);
-- 
2.25.1

[PATCH v6] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[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 function ppgtt_populate_spt_by_guest_entry
 does not notice that, it will free spt again in its error
 path.

Fix this by canceling the mapping of DMA address and freeing sub_spt.
Besides, leave the handle of spt destroy to caller function instead
of callee function when error occurs.

Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang 
Reviewed-by: Zhenyu Wang 
---
v6:
- remove the code for setting unused variable to NULL and fix type suggested
by Zhenyu

v5:
- remove unnecessary switch-case code for there is only one particular case,
correct the unmap target from parent_spt to sub_spt.add more details in
commit message. All suggested by Zhenyu

v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi

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 | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 51e5e8fb505b..7379e8d98417 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
   PAGE_SIZE, &dma_addr);
-   if (ret) {
-   ppgtt_invalidate_spt(spt);
-   return ret;
-   }
+   if (ret)
+   goto err;
sub_se.val64 = se->val64;
 
/* Copy the PAT field from PDE. */
@@ -1231,6 +1229,17 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ops->set_pfn(se, sub_spt->shadow_page.mfn);
ppgtt_set_shadow_entry(spt, se, index);
return 0;
+err:
+   /* Cancel the existing addess mappings of DMA addr. */
+   for_each_present_shadow_entry(sub_spt, &sub_se, sub_index) {
+   gvt_vdbg_mm("invalidate 4K entry\n");
+   ppgtt_invalidate_pte(sub_spt, &sub_se);
+   }
+   /* Release the new allocated spt. */
+   trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
+   sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
+   ppgtt_free_spt(sub_spt);
+   return ret;
 }
 
 static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
-- 
2.25.1

[PATCH v5] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[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 function ppgtt_populate_spt_by_guest_entry does not notice
 that, it will free spt again in its error path.

Fix this by undoing the mapping of DMA address and freeing sub_spt.
Besides, leave the handle of spt destroy to caller function instead of
callee function when error occurs.

Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang 
---
v5:
- remove unnecessary switch-case code for there is only one particular case,
correct the unmap target from parent_spt to sub_spt.add more details in
commit message. All suggested by Zhenyu

v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi

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 | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 51e5e8fb505b..4d478a59eb7d 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
   PAGE_SIZE, &dma_addr);
-   if (ret) {
-   ppgtt_invalidate_spt(spt);
-   return ret;
-   }
+   if (ret)
+   goto err;
sub_se.val64 = se->val64;
 
/* Copy the PAT field from PDE. */
@@ -1231,6 +1229,18 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ops->set_pfn(se, sub_spt->shadow_page.mfn);
ppgtt_set_shadow_entry(spt, se, index);
return 0;
+err:
+   /* Undone the existing mappings of DMA addr. */
+   for_each_present_shadow_entry(sub_spt, &sub_se, sub_index) {
+   gvt_vdbg_mm("invalidate 4K entry\n");
+   ppgtt_invalidate_pte(sub_spt, &sub_se);
+   }
+   /* Release the new allocated spt. */
+   trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
+   sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
+   ppgtt_free_spt(sub_spt);
+   sub_spt = NULL;
+   return ret;
 }
 
 static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
-- 
2.25.1

[RESEND PATCH v4] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[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 undoing the mapping of DMA address and freeing sub_spt.

Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang 
---
v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi

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 | 53 +-
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 51e5e8fb505b..b472e021e5a4 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1192,11 +1192,11 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
 {
const struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
struct intel_vgpu_ppgtt_spt *sub_spt;
-   struct intel_gvt_gtt_entry sub_se;
+   struct intel_gvt_gtt_entry sub_se, e;
unsigned long start_gfn;
dma_addr_t dma_addr;
-   unsigned long sub_index;
-   int ret;
+   unsigned long sub_index, parent_index;
+   int ret, ret1;
 
gvt_dbg_mm("Split 2M gtt entry, index %lu\n", index);
 
@@ -1209,10 +1209,8 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
   PAGE_SIZE, &dma_addr);
-   if (ret) {
-   ppgtt_invalidate_spt(spt);
-   return ret;
-   }
+   if (ret)
+   goto err;
sub_se.val64 = se->val64;
 
/* Copy the PAT field from PDE. */
@@ -1231,6 +1229,47 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
ops->set_pfn(se, sub_spt->shadow_page.mfn);
ppgtt_set_shadow_entry(spt, se, index);
return 0;
+err:
+   /* Undone the existing mappings of DMA addr. */
+   for_each_present_shadow_entry(spt, &e, parent_index) {
+   switch (e.type) {
+   case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
+   gvt_vdbg_mm("invalidate 4K entry\n");
+   ppgtt_invalidate_pte(spt, &e);
+   break;
+   case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
+   /* We don't setup 64K shadow entry so far. */
+   WARN(1, "suspicious 64K gtt entry\n");
+   continue;
+   case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
+   gvt_vdbg_mm("invalidate 2M entry\n");
+   continue;
+   case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
+   WARN(1, "GVT doesn't support 1GB page\n");
+   continue;
+   case GTT_TYPE_PPGTT_PML4_ENTRY:
+   case GTT_TYPE_PPGTT_PDP_ENTRY:
+   case GTT_TYPE_PPGTT_PDE_ENTRY:
+   gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
+   ret1 = ppgtt_invalidate_spt_by_shadow_entry(
+   spt->vgpu, &e);
+   if (ret1) {
+   gvt_vgpu_err("fail: shadow page %p shadow entry 
0x%llx type %d\n",
+   spt, e.val64, e.type);
+   goto free_spt;
+   }
+   break;
+   default:
+   GEM_BUG_ON(1);
+   }
+   }
+   /* Release the new alloced apt. */
+free_spt:
+   trace_spt_change(sub_spt->vgpu->id, "release", sub_spt,
+   sub_spt->guest_page.gfn, sub_spt->shadow_page.type);
+   ppgtt_free_spt(sub_spt);
+   sub_spt = NULL;
+   return ret;
 }
 
 static int split_64KB_gtt_entry(struct intel_vgpu *vgpu,
-- 
2.25.1

[PATCH v4] [PATCH v4] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[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 undoing the mapping of DMA address and freeing sub_spt.

Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support")
Signed-off-by: Zheng Wang 
---
v4:
- fix by undo the mapping of DMA address and free sub_spt suggested by Zhi

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 | 58 +-
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 45271acc5038..b472e021e5a4 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1209,7 +1209,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
for_each_shadow_entry(sub_spt, &sub_se, sub_index) {
ret = intel_gvt_dma_map_guest_page(vgpu, start_gfn + sub_index,
   PAGE_SIZE, &dma_addr);
-   if (ret) 
+   if (ret)
goto err;
sub_se.val64 = se->val64;
 
@@ -1233,34 +1233,34 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
/* Undone the existing mappings of DMA addr. */
for_each_present_shadow_entry(spt, &e, parent_index) {
switch (e.type) {
-   case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
-   gvt_vdbg_mm("invalidate 4K entry\n");
-   ppgtt_invalidate_pte(spt, &e);
-   break;
-   case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
-   /* We don't setup 64K shadow entry so far. */
-   WARN(1, "suspicious 64K gtt entry\n");
-   continue;
-   case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
-   gvt_vdbg_mm("invalidate 2M entry\n");
-   continue;
-   case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
-   WARN(1, "GVT doesn't support 1GB page\n");
-   continue;
-   case GTT_TYPE_PPGTT_PML4_ENTRY:
-   case GTT_TYPE_PPGTT_PDP_ENTRY:
-   case GTT_TYPE_PPGTT_PDE_ENTRY:
-   gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
-   ret1 = ppgtt_invalidate_spt_by_shadow_entry(
-   spt->vgpu, &e);
-   if (ret1) {
-   gvt_vgpu_err("fail: shadow page %p 
shadow entry 0x%llx type %d\n",
-   spt, e.val64, e.type);
-   goto free_spt;
-   }
-   break;
-   default:
-   GEM_BUG_ON(1);
+   case GTT_TYPE_PPGTT_PTE_4K_ENTRY:
+   gvt_vdbg_mm("invalidate 4K entry\n");
+   ppgtt_invalidate_pte(spt, &e);
+   break;
+   case GTT_TYPE_PPGTT_PTE_64K_ENTRY:
+   /* We don't setup 64K shadow entry so far. */
+   WARN(1, "suspicious 64K gtt entry\n");
+   continue;
+   case GTT_TYPE_PPGTT_PTE_2M_ENTRY:
+   gvt_vdbg_mm("invalidate 2M entry\n");
+   continue;
+   case GTT_TYPE_PPGTT_PTE_1G_ENTRY:
+   WARN(1, "GVT doesn't support 1GB page\n");
+   continue;
+   case GTT_TYPE_PPGTT_PML4_ENTRY:
+   case GTT_TYPE_PPGTT_PDP_ENTRY:
+   case GTT_TYPE_PPGTT_PDE_ENTRY:
+   gvt_vdbg_mm("invalidate PMUL4/PDP/PDE entry\n");
+   ret1 = ppgtt_invalidate_spt_by_shadow_entry(
+   spt->vgpu, &e);
+   if (ret1) {
+   gvt_vgpu_err("fail: shadow page %p shadow entry 
0x%llx type %d\n",
+   spt, e.val64, e.type);
+   goto free_spt;
+   }
+   break;
+   default:
+   GEM_BUG_ON(1);
}
}
/* Release the new alloced apt. */
-- 
2.25.1

Re: [Intel-gfx] [PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[Zheng Wang]

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

[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

[PATCH v3] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[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 
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(&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

[PATCH v2] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[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 
Signed-off-by: Zheng Wang 
---
v2:
- split initial function into two api function suggested by Greg

v1: https://lore.kernel.org/all/20220928033340.1063949-1-zyytlz...@163.com/
---
 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

[PATCH] drm/i915/gvt: fix double free bug in split_2MB_gtt_entry

[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 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(&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

[PATCH] drm/i915/gvt: fix double-free bug in split_2MB_gtt_entry

[Zheng Wang]

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