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

[Marek Olšák]

Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Marek

On Mon, Mar 21, 2016 at 9:41 PM, Nicolai Hähnle <nhaehnle at gmail.com> 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.
> ---
>  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);
> --
> 2.5.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev