| Message ID | 20241227112920.1547592-1-apoorva.singh@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/gt: Prevent uninitialized pointer reads | expand |
Hi Apoorva, On Fri, Dec 27, 2024 at 04:59:20PM +0530, apoorva.singh@intel.com wrote: > From: Apoorva Singh <apoorva.singh@intel.com> > > Initialize rq to NULL to prevent uninitialized pointer reads. where is it happening? Andi
Hi Apoorva and Andi, > > Initialize rq to NULL to prevent uninitialized pointer reads. > > where is it happening? > > Andi There are two instances that I see: 1) err = fn(..., &rq); <- this probably will set the rq (at least looking at the functions that are used to set "fn", when calling "clear()"), but from the clear()'s execution perspective there are no guarantees that this value will be initialized. 2) If an "err" is set and detected before "rq" is initialized, then there is a path that the execution might take that leads to reading uninitialized "rq": if all calls to i915_gem_object_pin_map() would result in -ENXIO, then the loop would exit with a "continue" and err = -ENXIO. This then triggers "if (err)" after the loop and takes the second "if", which starts by checking the value of "rq", which at this point would still be uninitialized. I think this initialization removes these corner cases, so it is worth introducing. Krzysztof
Hi Apoorva, > From: Apoorva Singh <apoorva.singh@intel.com> > > Initialize rq to NULL to prevent uninitialized pointer reads. > > Signed-off-by: Apoorva Singh <apoorva.singh@intel.com> > --- > drivers/gpu/drm/i915/gt/selftest_migrate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/selftest_migrate.c b/drivers/gpu/drm/i915/gt/selftest_migrate.c > index ca460cee4f8b..1bf7b88d9a9d 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_migrate.c > +++ b/drivers/gpu/drm/i915/gt/selftest_migrate.c > @@ -262,7 +262,7 @@ static int clear(struct intel_migrate *migrate, > { > struct drm_i915_private *i915 = migrate->context->engine->i915; > struct drm_i915_gem_object *obj; > - struct i915_request *rq; > + struct i915_request *rq = NULL; > struct i915_gem_ww_ctx ww; > u32 *vaddr, val = 0; > bool ccs_cap = false; > -- > 2.34.1 Looks good to me Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com> Krzysztof
diff --git a/drivers/gpu/drm/i915/gt/selftest_migrate.c b/drivers/gpu/drm/i915/gt/selftest_migrate.c index ca460cee4f8b..1bf7b88d9a9d 100644 --- a/drivers/gpu/drm/i915/gt/selftest_migrate.c +++ b/drivers/gpu/drm/i915/gt/selftest_migrate.c @@ -262,7 +262,7 @@ static int clear(struct intel_migrate *migrate, { struct drm_i915_private *i915 = migrate->context->engine->i915; struct drm_i915_gem_object *obj; - struct i915_request *rq; + struct i915_request *rq = NULL; struct i915_gem_ww_ctx ww; u32 *vaddr, val = 0; bool ccs_cap = false;