| Message ID | 20211130141545.153899-1-zhou1615@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Fix a NULL pointer dereference in igt_request_rewind() | expand |
On 30/11/2021 14:15, Zhou Qingyang wrote: > In igt_request_rewind(), mock_context(i915, "A") is assigned to ctx[0] > and used in i915_gem_context_get_engine(). There is a dereference > of ctx[0] in i915_gem_context_get_engine(), which could lead to a NULL > pointer dereference on failure of mock_context(i915, "A") . > > So as mock_context(i915, "B"). > > Although this bug is not serious for it belongs to testing code, it is > better to be fixed to avoid unexpected failure in testing. > > Fix this bugs by adding checks about ctx[0] and ctx[1]. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > > Builds with CONFIG_DRM_I915_SELFTEST=y show no new warnings, > and our static analyzer no longer warns about this code. > > Fixes: ca883c304f54 ("drm/i915/selftests: Pass intel_context to mock_request") I think it is this one instead: 591c0fb85d1c ("drm/i915: Exercise request cancellation using a mock selftest") Fix looks correct so: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Thanks for the patch! Regards, Tvrtko P.S. Although Fixes: is probably a bit over the top since it is selftests only so I'll probably drop it while applying. > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu> > --- > drivers/gpu/drm/i915/selftests/i915_request.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c > index d67710d10615..d6fc7b892793 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_request.c > +++ b/drivers/gpu/drm/i915/selftests/i915_request.c > @@ -209,6 +209,10 @@ static int igt_request_rewind(void *arg) > int err = -EINVAL; > > ctx[0] = mock_context(i915, "A"); > + if (!ctx[0]) { > + err = -ENOMEM; > + goto err_ctx_0; > + } > > ce = i915_gem_context_get_engine(ctx[0], RCS0); > GEM_BUG_ON(IS_ERR(ce)); > @@ -223,6 +227,10 @@ static int igt_request_rewind(void *arg) > i915_request_add(request); > > ctx[1] = mock_context(i915, "B"); > + if (!ctx[1]) { > + err = -ENOMEM; > + goto err_ctx_1; > + } > > ce = i915_gem_context_get_engine(ctx[1], RCS0); > GEM_BUG_ON(IS_ERR(ce)); > @@ -261,9 +269,11 @@ static int igt_request_rewind(void *arg) > i915_request_put(vip); > err_context_1: > mock_context_close(ctx[1]); > +err_ctx_1: > i915_request_put(request); > err_context_0: > mock_context_close(ctx[0]); > +err_ctx_0: > mock_device_flush(i915); > return err; > } >
diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c index d67710d10615..d6fc7b892793 100644 --- a/drivers/gpu/drm/i915/selftests/i915_request.c +++ b/drivers/gpu/drm/i915/selftests/i915_request.c @@ -209,6 +209,10 @@ static int igt_request_rewind(void *arg) int err = -EINVAL; ctx[0] = mock_context(i915, "A"); + if (!ctx[0]) { + err = -ENOMEM; + goto err_ctx_0; + } ce = i915_gem_context_get_engine(ctx[0], RCS0); GEM_BUG_ON(IS_ERR(ce)); @@ -223,6 +227,10 @@ static int igt_request_rewind(void *arg) i915_request_add(request); ctx[1] = mock_context(i915, "B"); + if (!ctx[1]) { + err = -ENOMEM; + goto err_ctx_1; + } ce = i915_gem_context_get_engine(ctx[1], RCS0); GEM_BUG_ON(IS_ERR(ce)); @@ -261,9 +269,11 @@ static int igt_request_rewind(void *arg) i915_request_put(vip); err_context_1: mock_context_close(ctx[1]); +err_ctx_1: i915_request_put(request); err_context_0: mock_context_close(ctx[0]); +err_ctx_0: mock_device_flush(i915); return err; }
In igt_request_rewind(), mock_context(i915, "A") is assigned to ctx[0] and used in i915_gem_context_get_engine(). There is a dereference of ctx[0] in i915_gem_context_get_engine(), which could lead to a NULL pointer dereference on failure of mock_context(i915, "A") . So as mock_context(i915, "B"). Although this bug is not serious for it belongs to testing code, it is better to be fixed to avoid unexpected failure in testing. Fix this bugs by adding checks about ctx[0] and ctx[1]. This bug was found by a static analyzer. The analysis employs differential checking to identify inconsistent security operations (e.g., checks or kfrees) between two code paths and confirms that the inconsistent operations are not recovered in the current function or the callers, so they constitute bugs. Note that, as a bug found by static analysis, it can be a false positive or hard to trigger. Multiple researchers have cross-reviewed the bug. Builds with CONFIG_DRM_I915_SELFTEST=y show no new warnings, and our static analyzer no longer warns about this code. Fixes: ca883c304f54 ("drm/i915/selftests: Pass intel_context to mock_request") Signed-off-by: Zhou Qingyang <zhou1615@umn.edu> --- drivers/gpu/drm/i915/selftests/i915_request.c | 10 ++++++++++ 1 file changed, 10 insertions(+)