[Intel-gfx] [PATCH] drm/i915/gvt: Prevent use-after-free in ppgtt_free_all_spt()

[Chris Wilson]

Quoting Zhenyu Wang (2019-04-04 08:48:34)
> On 2019.04.04 08:30:56 +0100, Chris Wilson wrote:
> > ppgtt_free_all_spt() iterates the radixtree as it is deleting it,
> > forgoing all protection against the leaves being freed in the process
> > (leaving the iter pointing into the void).
> > 
> > A minimal fix seems to be to use the available post_shadow_list to
> > decompose the tree into a list prior to destroying the radixtree.
> > 
> > Alerted by the sparse warnings:
> > 
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4> **
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4> **
> > drivers/gpu/drm/i915/gvt/gtt.c:758:45: warning: incorrect type in argument 1 (different address spaces)
> > drivers/gpu/drm/i915/gvt/gtt.c:758:45:    expected void [noderef] <asn:4> **slot
> > drivers/gpu/drm/i915/gvt/gtt.c:758:45:    got void **slot
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in argument 1 (different address spaces)
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void [noderef] <asn:4> **slot
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void **slot
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
> > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4> **
> > 
> > This would also have been loudly warning if run through CI for the
> > invalid RCU dereferences.
> > 
> > Fixes: b6c126a39345 ("drm/i915/gvt: Manage shadow pages with radix tree")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Changbin Du <changbin.du at intel.com>
> > Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> > Cc: Zhi Wang <zhi.a.wang at intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/gtt.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> > index cf133ef03873..9814773882ec 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -750,14 +750,20 @@ static void ppgtt_free_spt(struct intel_vgpu_ppgtt_spt *spt)
> >  
> >  static void ppgtt_free_all_spt(struct intel_vgpu *vgpu)
> >  {
> > -     struct intel_vgpu_ppgtt_spt *spt;
> > +     struct intel_vgpu_ppgtt_spt *spt, *spn;
> >       struct radix_tree_iter iter;
> > -     void **slot;
> > +     LIST_HEAD(all_spt);
> > +     void __rcu **slot;
> >  
> > +     rcu_read_lock();
> >       radix_tree_for_each_slot(slot, &vgpu->gtt.spt_tree, &iter, 0) {
> >               spt = radix_tree_deref_slot(slot);
> > -             ppgtt_free_spt(spt);
> > +             list_move(&spt->post_shadow_list, &all_spt);
> >       }
> > +     rcu_read_unlock();
> > +
> > +     list_for_each_entry_safe(spt, spn, &all_spt, post_shadow_list)
> > +             ppgtt_free_spt(spt);
> >  }
> >
> 
> As we ensure to flush post shadow list, so this is safe to reuse.

Phew! I looked, couldn't see that it would be used at this point, so
hoped for the best.
 
> Reviewed-by: Zhenyu Wang <zhenyuw at linux.intel.com>

Will you take both of these patches through your tree?
-Chris