[PATCH] drm/i915/gvt/scheduler: fix potential NULL pointer dereference

[PATCH] drm/i915/gvt/scheduler: fix potential NULL pointer dereference

[Gustavo A. R. Silva]

_workload_ is being dereferenced before it is null checked, hence
there is a potential null pointer dereference.

Fix this by moving the pointer dereference after _workload_ has
been null checked.

Addresses-Coverity-ID: 1430136 ("Dereference before null check")
Fixes: fa3dd623e559 ("drm/i915/gvt: keep oa config in shadow ctx")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.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 0681264..be1a297 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];
 
-- 
2.7.4

Re: [PATCH] drm/i915/gvt/scheduler: fix potential NULL pointer dereference

[Chris Wilson]

Quoting Gustavo A. R. Silva (2018-03-19 19:30:53)
> _workload_ is being dereferenced before it is null checked, hence
> there is a potential null pointer dereference.
> 
> Fix this by moving the pointer dereference after _workload_ has
> been null checked.

The checks are misleading and not required.
-Chris

Re: [PATCH] drm/i915/gvt/scheduler: fix potential NULL pointer dereference

[Gustavo A. R. Silva]

Hi Chris,

On 03/19/2018 03:38 PM, Chris Wilson wrote:
> Quoting Gustavo A. R. Silva (2018-03-19 19:30:53)
>> _workload_ is being dereferenced before it is null checked, hence
>> there is a potential null pointer dereference.
>>
>> Fix this by moving the pointer dereference after _workload_ has
>> been null checked.
> 
> The checks are misleading and not required.

All of them?

if (!workload || !reg_state || workload->ring_id != RCS)
	return;

or just the null check on workload ?

Thanks for the feedback.
--
Gustavo

Re: [PATCH] drm/i915/gvt/scheduler: fix potential NULL pointer dereference

[Chris Wilson]

Quoting Gustavo A. R. Silva (2018-03-19 20:50:12)
> Hi Chris,
> 
> On 03/19/2018 03:38 PM, Chris Wilson wrote:
> > Quoting Gustavo A. R. Silva (2018-03-19 19:30:53)
> >> _workload_ is being dereferenced before it is null checked, hence
> >> there is a potential null pointer dereference.
> >>
> >> Fix this by moving the pointer dereference after _workload_ has
> >> been null checked.
> > 
> > The checks are misleading and not required.
> 
> All of them?
> 
> if (!workload || !reg_state || workload->ring_id != RCS)
>         return;

workload can not be NULL (dereference in caller), reg_state can not be
NULL (by construct from kmap()).

It may be not an RCS ring through.
-Chris

Re: [PATCH] drm/i915/gvt/scheduler: fix potential NULL pointer dereference

[Gustavo A. R. Silva]

On 03/19/2018 04:23 PM, Chris Wilson wrote:
> Quoting Gustavo A. R. Silva (2018-03-19 20:50:12)
>> Hi Chris,
>>
>> On 03/19/2018 03:38 PM, Chris Wilson wrote:
>>> Quoting Gustavo A. R. Silva (2018-03-19 19:30:53)
>>>> _workload_ is being dereferenced before it is null checked, hence
>>>> there is a potential null pointer dereference.
>>>>
>>>> Fix this by moving the pointer dereference after _workload_ has
>>>> been null checked.
>>>
>>> The checks are misleading and not required.
>>
>> All of them?
>>
>> if (!workload || !reg_state || workload->ring_id != RCS)
>>          return;
> 
> workload can not be NULL (dereference in caller), reg_state can not be
> NULL (by construct from kmap()).
> 
> It may be not an RCS ring through

I got it.

I'll send the following patch then:

--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -74,7 +74,7 @@ static void sr_oa_regs(struct intel_vgpu_workload 
*workload,
                 i915_mmio_reg_offset(EU_PERF_CNTL6),
         };

-       if (!workload || !reg_state || workload->ring_id != RCS)
+       if(workload->ring_id != RCS)
                 return;

         if (save) {


Thanks
--
Gustavo