2018-03-19 19:32:59

by Gustavo A. R. Silva

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

_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 <[email protected]>
---
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

2018-03-19 20:41:49

by Chris Wilson

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

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

2018-03-19 21:47:21

by Gustavo A. R. Silva

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

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

2018-03-20 01:54:52

by Gustavo A. R. Silva

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

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

2018-03-20 01:56:14

by Chris Wilson

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

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