| Message ID | 20180321190653.3829-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-03-21 at 19:06 +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The pointer workload is dereferenced before it is null checked, hence > there is a potential for a null pointer dereference on workload. Fix > this by only dereferencing workload after it is null checked. > > Detected by CoverityScan, CID#1466017 ("Dereference before null check") Maybe true, but is it possible for workload to be null? Maybe the null test should be removed instead. > Fixes: fa3dd623e559 ("drm/i915/gvt: keep oa config in shadow ctx") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/gpu/drm/i915/gvt/scheduler.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c > index 068126404151..f3010e365a48 100644 > --- a/drivers/gpu/drm/i915/gvt/scheduler.c > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c > @@ -60,9 +60,9 @@ static void set_context_pdp_root_pointer( > static void sr_oa_regs(struct intel_vgpu_workload *workload, > u32 *reg_state, bool save) > { > - struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv; > - u32 ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; > - u32 ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; > + struct drm_i915_private *dev_priv; > + u32 ctx_oactxctrl; > + u32 ctx_flexeu0; > int i = 0; > u32 flex_mmio[] = { > i915_mmio_reg_offset(EU_PERF_CNTL0), > @@ -77,6 +77,10 @@ static void sr_oa_regs(struct intel_vgpu_workload *workload, > if (!workload || !reg_state || workload->ring_id != RCS) > return; > > + dev_priv = workload->vgpu->gvt->dev_priv; > + ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; > + ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; > + > if (save) { > workload->oactxctrl = reg_state[ctx_oactxctrl + 1]; >
On 21/03/18 19:09, Joe Perches wrote: > On Wed, 2018-03-21 at 19:06 +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> The pointer workload is dereferenced before it is null checked, hence >> there is a potential for a null pointer dereference on workload. Fix >> this by only dereferencing workload after it is null checked. >> >> Detected by CoverityScan, CID#1466017 ("Dereference before null check") > > Maybe true, but is it possible for workload to be null? > Maybe the null test should be removed instead. From what I understand from the static analysis, there may be a potential for workload to be null, I couldn't rule it out so I went with the more paranoid stance of keeping the null check in. > >> Fixes: fa3dd623e559 ("drm/i915/gvt: keep oa config in shadow ctx") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/gpu/drm/i915/gvt/scheduler.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c >> index 068126404151..f3010e365a48 100644 >> --- a/drivers/gpu/drm/i915/gvt/scheduler.c >> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c >> @@ -60,9 +60,9 @@ static void set_context_pdp_root_pointer( >> static void sr_oa_regs(struct intel_vgpu_workload *workload, >> u32 *reg_state, bool save) >> { >> - struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv; >> - u32 ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; >> - u32 ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; >> + struct drm_i915_private *dev_priv; >> + u32 ctx_oactxctrl; >> + u32 ctx_flexeu0; >> int i = 0; >> u32 flex_mmio[] = { >> i915_mmio_reg_offset(EU_PERF_CNTL0), >> @@ -77,6 +77,10 @@ static void sr_oa_regs(struct intel_vgpu_workload *workload, >> if (!workload || !reg_state || workload->ring_id != RCS) >> return; >> >> + dev_priv = workload->vgpu->gvt->dev_priv; >> + ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; >> + ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; >> + >> if (save) { >> workload->oactxctrl = reg_state[ctx_oactxctrl + 1]; >>
Quoting Colin Ian King (2018-03-21 19:18:28) > On 21/03/18 19:09, Joe Perches wrote: > > On Wed, 2018-03-21 at 19:06 +0000, Colin King wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> The pointer workload is dereferenced before it is null checked, hence > >> there is a potential for a null pointer dereference on workload. Fix > >> this by only dereferencing workload after it is null checked. > >> > >> Detected by CoverityScan, CID#1466017 ("Dereference before null check") > > > > Maybe true, but is it possible for workload to be null? > > Maybe the null test should be removed instead. > > From what I understand from the static analysis, there may be a > potential for workload to be null, I couldn't rule it out so I went with > the more paranoid stance of keeping the null check in. Not sr_oa_regs() problem if workload is NULL, that's the callers. I reviewed the identical patch yesterday, and we ended up with removing the NULL checks, just keeping the workload->id != RCS. -Chris
On Wed, 2018-03-21 at 19:18 +0000, Colin Ian King wrote: > On 21/03/18 19:09, Joe Perches wrote: > > On Wed, 2018-03-21 at 19:06 +0000, Colin King wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > The pointer workload is dereferenced before it is null checked, hence > > > there is a potential for a null pointer dereference on workload. Fix > > > this by only dereferencing workload after it is null checked. > > > > > > Detected by CoverityScan, CID#1466017 ("Dereference before null check") > > > > Maybe true, but is it possible for workload to be null? > > Maybe the null test should be removed instead. > > From what I understand from the static analysis, there may be a > potential for workload to be null, I couldn't rule it out so I went with > the more paranoid stance of keeping the null check in. workload cannot be null here. Look at the uses of sr_oa_regs and see that workload has already been dereferenced. > > > > > Fixes: fa3dd623e559 ("drm/i915/gvt: keep oa config in shadow ctx") > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > --- > > > drivers/gpu/drm/i915/gvt/scheduler.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c > > > index 068126404151..f3010e365a48 100644 > > > --- a/drivers/gpu/drm/i915/gvt/scheduler.c > > > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c > > > @@ -60,9 +60,9 @@ static void set_context_pdp_root_pointer( > > > static void sr_oa_regs(struct intel_vgpu_workload *workload, > > > u32 *reg_state, bool save) > > > { > > > - struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv; > > > - u32 ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; > > > - u32 ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; > > > + struct drm_i915_private *dev_priv; > > > + u32 ctx_oactxctrl; > > > + u32 ctx_flexeu0; > > > int i = 0; > > > u32 flex_mmio[] = { > > > i915_mmio_reg_offset(EU_PERF_CNTL0), > > > @@ -77,6 +77,10 @@ static void sr_oa_regs(struct intel_vgpu_workload *workload, > > > if (!workload || !reg_state || workload->ring_id != RCS) > > > return; > > > > > > + dev_priv = workload->vgpu->gvt->dev_priv; > > > + ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; > > > + ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; > > > + > > > if (save) { > > > workload->oactxctrl = reg_state[ctx_oactxctrl + 1]; > > > > >
On 21/03/18 19:23, Chris Wilson wrote: > Quoting Colin Ian King (2018-03-21 19:18:28) >> On 21/03/18 19:09, Joe Perches wrote: >>> On Wed, 2018-03-21 at 19:06 +0000, Colin King wrote: >>>> From: Colin Ian King <colin.king@canonical.com> >>>> >>>> The pointer workload is dereferenced before it is null checked, hence >>>> there is a potential for a null pointer dereference on workload. Fix >>>> this by only dereferencing workload after it is null checked. >>>> >>>> Detected by CoverityScan, CID#1466017 ("Dereference before null check") >>> >>> Maybe true, but is it possible for workload to be null? >>> Maybe the null test should be removed instead. >> >> From what I understand from the static analysis, there may be a >> potential for workload to be null, I couldn't rule it out so I went with >> the more paranoid stance of keeping the null check in. > > Not sr_oa_regs() problem if workload is NULL, that's the callers. I > reviewed the identical patch yesterday, and we ended up with removing > the NULL checks, just keeping the workload->id != RCS. > -Chris > Ah, OK, thanks for the clarification Chris.
diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c index 068126404151..f3010e365a48 100644 --- a/drivers/gpu/drm/i915/gvt/scheduler.c +++ b/drivers/gpu/drm/i915/gvt/scheduler.c @@ -60,9 +60,9 @@ static void set_context_pdp_root_pointer( static void sr_oa_regs(struct intel_vgpu_workload *workload, u32 *reg_state, bool save) { - struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv; - u32 ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; - u32 ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; + struct drm_i915_private *dev_priv; + u32 ctx_oactxctrl; + u32 ctx_flexeu0; int i = 0; u32 flex_mmio[] = { i915_mmio_reg_offset(EU_PERF_CNTL0), @@ -77,6 +77,10 @@ static void sr_oa_regs(struct intel_vgpu_workload *workload, if (!workload || !reg_state || workload->ring_id != RCS) return; + dev_priv = workload->vgpu->gvt->dev_priv; + ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset; + ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; + if (save) { workload->oactxctrl = reg_state[ctx_oactxctrl + 1];