[Mesa-dev] [PATCH] radeonsi: fix out-of-bounds indexing of shader images

[Nicolai Hähnle]

On 21.03.2016 15:41, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Results are undefined but may not crash. Without this change, out-of-bounds
> indexing can lead to VM faults and GPU hangs.
>
> Constant buffers, samplers, and possibly others will eventually need similar
> treatment to support GL_ARB_robust_buffer_access_behavior.

FWIW, I read the relevant specs again, and we do already need to apply 
the same change to the other buffers as well for ARB_robustness. I'll 
follow up with a patch soon.

Amusingly enough, this ends up producing cleaner code. The reason is 
simple: previously, LLVM produced code that works with a full 32 bit 
index, hence manipulating 64 bit pointer values all the way. By masking 
down to the low bits, LLVM understands that some of the intermediate 
computations (i.e., multiplying by the stride of the descriptor array) 
can be done using only 32 bit registers.

Cheers,
Nicolai

> ---
>   src/gallium/drivers/radeonsi/si_shader.c | 44 +++++++++++++++++++++++++++++++-
>   1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
> index 9ad2290..1e4bf82 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -532,6 +532,37 @@ static LLVMValueRef get_indirect_index(struct si_shader_context *ctx,
>   }
>
>   /**
> + * Like get_indirect_index, but restricts the return value to a (possibly
> + * undefined) value inside [0..num).
> + */
> +static LLVMValueRef get_bounded_indirect_index(struct si_shader_context *ctx,
> +					       const struct tgsi_ind_register *ind,
> +					       int rel_index, unsigned num)
> +{
> +	struct gallivm_state *gallivm = &ctx->radeon_bld.gallivm;
> +	LLVMBuilderRef builder = gallivm->builder;
> +	LLVMValueRef result = get_indirect_index(ctx, ind, rel_index);
> +	LLVMValueRef c_max = LLVMConstInt(ctx->i32, num - 1, 0);
> +	LLVMValueRef cc;
> +
> +	if (util_is_power_of_two(num)) {
> +		result = LLVMBuildAnd(builder, result, c_max, "");
> +	} else {
> +		/* In theory, this MAX pattern should result in code that is
> +		 * as good as the bit-wise AND above.
> +		 *
> +		 * In practice, LLVM generates worse code (at the time of
> +		 * writing), because its value tracking is not strong enough.
> +		 */
> +		cc = LLVMBuildICmp(builder, LLVMIntULE, result, c_max, "");
> +		result = LLVMBuildSelect(builder, cc, result, c_max, "");
> +	}
> +
> +	return result;
> +}
> +
> +
> +/**
>    * Calculate a dword address given an input or output register and a stride.
>    */
>   static LLVMValueRef get_dw_address(struct si_shader_context *ctx,
> @@ -2814,7 +2845,18 @@ image_fetch_rsrc(
>   		LLVMValueRef rsrc_ptr;
>   		LLVMValueRef tmp;
>
> -		ind_index = get_indirect_index(ctx, &image->Indirect, image->Register.Index);
> +		/* From the GL_ARB_shader_image_load_store extension spec:
> +		 *
> +		 *    If a shader performs an image load, store, or atomic
> +		 *    operation using an image variable declared as an array,
> +		 *    and if the index used to select an individual element is
> +		 *    negative or greater than or equal to the size of the
> +		 *    array, the results of the operation are undefined but may
> +		 *    not lead to termination.
> +		 */
> +		ind_index = get_bounded_indirect_index(ctx, &image->Indirect,
> +						       image->Register.Index,
> +						       SI_NUM_IMAGES);
>
>   		rsrc_ptr = LLVMGetParam(ctx->radeon_bld.main_fn, SI_PARAM_IMAGES);
>   		tmp = build_indexed_load_const(ctx, rsrc_ptr, ind_index);
>