[PATCH 2/6] drm/i915/gvt: Prevent invalid ring_id access to array regs[]

[Zhenyu Wang]

On 2019.03.20 11:21:26 +0800, Colin Xu wrote:
> gvt only inits ring regs for existing ring in supported GEN graphics,
> the ring_id could be incontinuous so regs[] could have uninitialized
> value. So limit the ring_id to initilized rings.
> Access to them could cause unexpected behaviours. Although these access
> are protected by current gvt logic and won't hit during running, but still
> has potential security risk in future.
> 
> intel_context_lookup() could return NULL but is_inhibit_context() will
> access the pointer. Validate intel_context pointer before using.
> 
> Signed-off-by: Colin Xu <colin.xu at intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/mmio_context.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/mmio_context.c b/drivers/gpu/drm/i915/gvt/mmio_context.c
> index 8a2f6e9d2257..01602c2ed85a 100644
> --- a/drivers/gpu/drm/i915/gvt/mmio_context.c
> +++ b/drivers/gpu/drm/i915/gvt/mmio_context.c
> @@ -162,6 +162,9 @@ static void load_render_mocs(struct drm_i915_private *dev_priv)
>  	for (ring_id = 0; ring_id < ARRAY_SIZE(regs); ring_id++) {
>  		if (!HAS_ENGINE(dev_priv, ring_id))
>  			continue;
> +		if (ring_id != RCS0 && ring_id != VCS0 && ring_id != VCS1 &&
> +		    ring_id != BCS0 && ring_id != VECS0)
> +			continue;
>  		offset.reg = regs[ring_id];

If we won't want to depend on any order of engine id definition, maybe just change
current array index way into a simple switch, and bail out for not valid engine.

>  		for (i = 0; i < GEN9_MOCS_SIZE; i++) {
>  			gen9_render_mocs.control_table[ring_id][i] =
> @@ -340,8 +343,12 @@ static void handle_tlb_pending_event(struct intel_vgpu *vgpu, int ring_id)
>  		[VECS0] = 0x4270,
>  	};
>  
> -	if (WARN_ON(ring_id >= ARRAY_SIZE(regs)))
> +	if (ring_id != RCS0 && ring_id != VCS0 && ring_id != VCS1 &&
> +	    ring_id != BCS0 && ring_id != VECS0) {
> +		WARN_ON(1);
> +		gvt_vgpu_err("Invalid ring id (%d)\n", ring_id);
>  		return;
> +	}
>  
>  	if (!test_and_clear_bit(ring_id, (void *)s->tlb_handle_pending))
>  		return;
> @@ -389,8 +396,13 @@ static void switch_mocs(struct intel_vgpu *pre, struct intel_vgpu *next,
>  	int i;
>  
>  	dev_priv = pre ? pre->gvt->dev_priv : next->gvt->dev_priv;
> -	if (WARN_ON(ring_id >= ARRAY_SIZE(regs)))
> +
> +	if (ring_id != RCS0 && ring_id != VCS0 && ring_id != VCS1 &&
> +	    ring_id != BCS0 && ring_id != VECS0) {
> +		WARN_ON(1);
> +		gvt_err("Invalid ring id (%d)\n", ring_id);
>  		return;
> +	}
>  
>  	if (ring_id == RCS0 &&
>  	    (IS_KABYLAKE(dev_priv) ||
> @@ -458,6 +470,7 @@ static void switch_mmio(struct intel_vgpu *pre,
>  	struct drm_i915_private *dev_priv;
>  	struct intel_vgpu_submission *s;
>  	struct engine_mmio *mmio;
> +	struct intel_context *ce = NULL;
>  	u32 old_v, new_v;
>  
>  	dev_priv = pre ? pre->gvt->dev_priv : next->gvt->dev_priv;
> @@ -495,9 +508,8 @@ static void switch_mmio(struct intel_vgpu *pre,
>  			 * image if it's not inhibit context, it will restore
>  			 * itself.
>  			 */
> -			if (mmio->in_context &&
> -			    !is_inhibit_context(intel_context_lookup(s->shadow_ctx,
> -								     dev_priv->engine[ring_id])))
> +			ce = intel_context_lookup(s->shadow_ctx, dev_priv->engine[ring_id]);
> +			if (mmio->in_context && ce && !is_inhibit_context(ce))
>  				continue;
>  
>  			if (mmio->mask)
> -- 
> 2.21.0
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20190322/b863a45b/attachment.sig>