From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH] drm/vc4: Fix NULL pointer dereference in vc4_save_hang_state() Date: Wed, 17 Jan 2018 12:03:15 -0800 Message-ID: <877esgfe2k.fsf@anholt.net> References: <20180110151825.23477-1-boris.brezillon@free-electrons.com> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Return-path: In-Reply-To: <20180110151825.23477-1-boris.brezillon@free-electrons.com> Sender: stable-owner@vger.kernel.org To: David Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org Cc: Boris Brezillon , stable@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org --=-=-= Content-Type: text/plain Boris Brezillon writes: > When saving BOs in the hang state we skip one entry of the > kernel_state->bo[] array, thus leaving it to NULL. This leads to a NULL > pointer dereference when, later in this function, we iterate over all > BOs to check their ->madv state. > > Fixes: ca26d28bbaa3 ("drm/vc4: improve throughput by pipelining binning and rendering jobs") > Cc: > Signed-off-by: Boris Brezillon > --- > drivers/gpu/drm/vc4/vc4_gem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c > index 6c32c89a83a9..19ac7fe0e5db 100644 > --- a/drivers/gpu/drm/vc4/vc4_gem.c > +++ b/drivers/gpu/drm/vc4/vc4_gem.c > @@ -208,7 +208,7 @@ vc4_save_hang_state(struct drm_device *dev) > kernel_state->bo[j + prev_idx] = &bo->base.base; > j++; > } > - prev_idx = j + 1; > + prev_idx = j; Could we replace the whole "[j + prev_idx]" with a "[k++]" and maybe a WARN_ON_ONCE(k != state->bo_count) at the end? I really need to figure out if I can come up with a way to make IGT cases for GPU hangs on vc4, despite the validation. I found a bug in GPU reset due to BCL hangs when doing vc5, but I don't have a testcase. Maybe some submit flags that overwrite the BCL or RCL to do an infinite loop? --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlpfrAMACgkQtdYpNtH8 nuiaLQ//fe8CHZjEwet/Q0d9JnCdTPrHkiNYNwqpRssgzkQz5zR/KC/+I+kLGBdX jHlvWSf5JVJg8ogQdDQ0tvb/n5qbYTGqn9oTx4tIyZ/62de6Ip7Hckv0HVRxFL1P rPWk9dKeFLtGntQq45LHzlChEYStmqqpg7lk5lNCzvdWa4JUnnnb186BuZnhdz9A XqS3Ey4Jo4Z1Mp3FNFOTK/k2pxpEcLIpboBgutv5hhMzJSIJ+5vTvyyj1pdYT/3U fqIiQf39Ks9c2U0QhAjIx2paQtwenUQ2VHAyuhAKPYxNVJ90xvQTnPdscTfz/Qb1 +jXG06OKmrGfFiZKdb2G9CVW3NSiB4M4LpMC/FLpClaN/x8n+E7IOEmQIBAC33a7 /3ouE3iYra1KVFZBtnm9gnzygb9Tn+syog8LG2AsMGB9yYtt9Ki1pdwQGU9mTtda Ab4yTKOtQs6wNJmLznko4BkjZqPuj1718SUfJyNiAQuE8n5CFeF9CFGBbGcBuk1H 6KWoZ2qVtI/YGY3BJBMArbMXdXJSAhtZY8/OvhPJN7eC8pRjsRfOh6SYpsZBZJq9 +6zfBEg1O/GBWx0trvg2in4ITkJCtjt6BH06YOi/aEUikOi75dRRNUA5/vR2Obqm WHCh1pnolsZX5b1E5husV3fAElzkC/YyMyEJ3v8e068VvHlxOw4= =p6O2 -----END PGP SIGNATURE----- --=-=-=-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from anholt.net ([50.246.234.109]:40990 "EHLO anholt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752320AbeAQUDU (ORCPT ); Wed, 17 Jan 2018 15:03:20 -0500 From: Eric Anholt To: Boris Brezillon , David Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org Cc: Boris Brezillon , stable@vger.kernel.org Subject: Re: [PATCH] drm/vc4: Fix NULL pointer dereference in vc4_save_hang_state() In-Reply-To: <20180110151825.23477-1-boris.brezillon@free-electrons.com> References: <20180110151825.23477-1-boris.brezillon@free-electrons.com> Date: Wed, 17 Jan 2018 12:03:15 -0800 Message-ID: <877esgfe2k.fsf@anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: stable-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain Boris Brezillon writes: > When saving BOs in the hang state we skip one entry of the > kernel_state->bo[] array, thus leaving it to NULL. This leads to a NULL > pointer dereference when, later in this function, we iterate over all > BOs to check their ->madv state. > > Fixes: ca26d28bbaa3 ("drm/vc4: improve throughput by pipelining binning and rendering jobs") > Cc: > Signed-off-by: Boris Brezillon > --- > drivers/gpu/drm/vc4/vc4_gem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c > index 6c32c89a83a9..19ac7fe0e5db 100644 > --- a/drivers/gpu/drm/vc4/vc4_gem.c > +++ b/drivers/gpu/drm/vc4/vc4_gem.c > @@ -208,7 +208,7 @@ vc4_save_hang_state(struct drm_device *dev) > kernel_state->bo[j + prev_idx] = &bo->base.base; > j++; > } > - prev_idx = j + 1; > + prev_idx = j; Could we replace the whole "[j + prev_idx]" with a "[k++]" and maybe a WARN_ON_ONCE(k != state->bo_count) at the end? I really need to figure out if I can come up with a way to make IGT cases for GPU hangs on vc4, despite the validation. I found a bug in GPU reset due to BCL hangs when doing vc5, but I don't have a testcase. Maybe some submit flags that overwrite the BCL or RCL to do an infinite loop? --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlpfrAMACgkQtdYpNtH8 nuiaLQ//fe8CHZjEwet/Q0d9JnCdTPrHkiNYNwqpRssgzkQz5zR/KC/+I+kLGBdX jHlvWSf5JVJg8ogQdDQ0tvb/n5qbYTGqn9oTx4tIyZ/62de6Ip7Hckv0HVRxFL1P rPWk9dKeFLtGntQq45LHzlChEYStmqqpg7lk5lNCzvdWa4JUnnnb186BuZnhdz9A XqS3Ey4Jo4Z1Mp3FNFOTK/k2pxpEcLIpboBgutv5hhMzJSIJ+5vTvyyj1pdYT/3U fqIiQf39Ks9c2U0QhAjIx2paQtwenUQ2VHAyuhAKPYxNVJ90xvQTnPdscTfz/Qb1 +jXG06OKmrGfFiZKdb2G9CVW3NSiB4M4LpMC/FLpClaN/x8n+E7IOEmQIBAC33a7 /3ouE3iYra1KVFZBtnm9gnzygb9Tn+syog8LG2AsMGB9yYtt9Ki1pdwQGU9mTtda Ab4yTKOtQs6wNJmLznko4BkjZqPuj1718SUfJyNiAQuE8n5CFeF9CFGBbGcBuk1H 6KWoZ2qVtI/YGY3BJBMArbMXdXJSAhtZY8/OvhPJN7eC8pRjsRfOh6SYpsZBZJq9 +6zfBEg1O/GBWx0trvg2in4ITkJCtjt6BH06YOi/aEUikOi75dRRNUA5/vR2Obqm WHCh1pnolsZX5b1E5husV3fAElzkC/YyMyEJ3v8e068VvHlxOw4= =p6O2 -----END PGP SIGNATURE----- --=-=-=--