From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zhenyu Wang Subject: Re: [PATCH] drm/i915/gvt: Prevent use-after-free in ppgtt_free_all_spt() Date: Mon, 8 Apr 2019 10:27:09 +0800 Message-ID: <20190408022709.GT2322@zhen-hp.sh.intel.com> References: <20190404073056.12407-1-chris@chris-wilson.co.uk> <20190404074834.GS2322@zhen-hp.sh.intel.com> <155436551568.7532.6039435513500149424@skylake-alporthouse-com> Reply-To: Zhenyu Wang Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1538867276==" Return-path: In-Reply-To: <155436551568.7532.6039435513500149424@skylake-alporthouse-com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, intel-gvt-dev@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1538867276== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tgGnixv3tJWXBxdL" Content-Disposition: inline --tgGnixv3tJWXBxdL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 2019.04.04 09:11:55 +0100, Chris Wilson wrote: > 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). > > >=20 > > > 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. > > >=20 > > > Alerted by the sparse warnings: > > >=20 > > > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assi= gnment (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] ** > > > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assi= gnment (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] ** > > > drivers/gpu/drm/i915/gvt/gtt.c:758:45: warning: incorrect type in arg= ument 1 (different address spaces) > > > drivers/gpu/drm/i915/gvt/gtt.c:758:45: expected void [noderef] **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 argu= ment 1 (different address spaces) > > > drivers/gpu/drm/i915/gvt/gtt.c:757:9: expected void [noderef] **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 assi= gnment (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] ** > > >=20 > > > This would also have been loudly warning if run through CI for the > > > invalid RCU dereferences. > > >=20 > > > Fixes: b6c126a39345 ("drm/i915/gvt: Manage shadow pages with radix tr= ee") > > > Signed-off-by: Chris Wilson > > > Cc: Changbin Du > > > Cc: Zhenyu Wang > > > Cc: Zhi Wang > > > --- > > > drivers/gpu/drm/i915/gvt/gtt.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > >=20 > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gv= t/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_pp= gtt_spt *spt) > > > =20 > > > 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; > > > =20 > > > + rcu_read_lock(); > > > radix_tree_for_each_slot(slot, &vgpu->gtt.spt_tree, &iter, 0) { > > > spt =3D 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); > > > } > > > > >=20 > > As we ensure to flush post shadow list, so this is safe to reuse. >=20 > Phew! I looked, couldn't see that it would be used at this point, so > hoped for the best. > =20 > > Reviewed-by: Zhenyu Wang >=20 > Will you take both of these patches through your tree? Yes. Thanks! --=20 Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827 --tgGnixv3tJWXBxdL Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQTXuabgHDW6LPt9CICxBBozTXgYJwUCXKqxfQAKCRCxBBozTXgY J3brAJ0WFNeqAFFRQQ4jSVrV+wClquowZACfTQ7RIiUZ1b1A1UssmuQDORuQ/7A= =h/z2 -----END PGP SIGNATURE----- --tgGnixv3tJWXBxdL-- --===============1538867276== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4 --===============1538867276==--