| Message ID | 20241227112920.1547592-1-apoorva.singh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
On Mon, Dec 30, 2024 at 02:24:45PM +0000, Krzysztof Karas wrote: > 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. To be really honest, I'm not entirely convinced of the corner cases, but just because this function is huge and this pointer is used in the very end, far from any of the path setting it, I believe it is already a good reason for introducing this protection. Also, the function can change in the future and this could be forgotten. So, I just pushed the patch to drm-intel-gt-next. Thanks for the patch and review, Rodrigo. > > 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;