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 <zhenyuw@xxxxxxxxxxxxxxx> 于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 <gregkh@xxxxxxxxxxxxxxxxxxx> ???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, &dma_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
