[Mesa-dev] [PATCH] r600g: Fix out of bounds access

[Dieter Nützel]

Hello Bartosz,

can you please rebase?
I think this could land.

Thanks,
Dieter

Am 08.02.2017 17:16, schrieb Bartosz Tomczyk:
> fc_sp variable should indicate number of elements in
> fc_stack array, but fc_sp was increased at beginning of fc_pushlevel
> function. It leads to situation where idx=0 was never used, and last
> 32 element was stored outside fs_stack array.
> ---
>  src/gallium/drivers/r600/r600_asm.h    |  3 ++-
>  src/gallium/drivers/r600/r600_shader.c | 39 
> +++++++++++++++++-----------------
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/src/gallium/drivers/r600/r600_asm.h
> b/src/gallium/drivers/r600/r600_asm.h
> index 1629399d8f..b12913d437 100644
> --- a/src/gallium/drivers/r600/r600_asm.h
> +++ b/src/gallium/drivers/r600/r600_asm.h
> @@ -25,6 +25,7 @@
> 
>  #include "r600_pipe.h"
>  #include "r600_isa.h"
> +#include "tgsi/tgsi_exec.h"
> 
>  struct r600_bytecode_alu_src {
>  	unsigned			sel;
> @@ -238,7 +239,7 @@ struct r600_bytecode {
>  	unsigned			force_add_cf;
>  	uint32_t			*bytecode;
>  	uint32_t			fc_sp;
> -	struct r600_cf_stack_entry	fc_stack[32];
> +	struct r600_cf_stack_entry	fc_stack[TGSI_EXEC_MAX_NESTING];
>  	struct r600_stack_info		stack;
>  	unsigned	ar_loaded;
>  	unsigned	ar_reg;
> diff --git a/src/gallium/drivers/r600/r600_shader.c
> b/src/gallium/drivers/r600/r600_shader.c
> index b80a3f8b62..34cc002c38 100644
> --- a/src/gallium/drivers/r600/r600_shader.c
> +++ b/src/gallium/drivers/r600/r600_shader.c
> @@ -8646,14 +8646,15 @@ static void fc_set_mid(struct r600_shader_ctx
> *ctx, int fc_sp)
> 
>  static void fc_pushlevel(struct r600_shader_ctx *ctx, int type)
>  {
> -	ctx->bc->fc_sp++;
> +	assert(ctx->bc->fc_sp < ARRAY_SIZE(ctx->bc->fc_stack));
>  	ctx->bc->fc_stack[ctx->bc->fc_sp].type = type;
>  	ctx->bc->fc_stack[ctx->bc->fc_sp].start = ctx->bc->cf_last;
> +	ctx->bc->fc_sp++;
>  }
> 
>  static void fc_poplevel(struct r600_shader_ctx *ctx)
>  {
> -	struct r600_cf_stack_entry *sp = &ctx->bc->fc_stack[ctx->bc->fc_sp];
> +	struct r600_cf_stack_entry *sp = &ctx->bc->fc_stack[ctx->bc->fc_sp - 
> 1];
>  	free(sp->mid);
>  	sp->mid = NULL;
>  	sp->num_mid = 0;
> @@ -8749,24 +8750,24 @@ static int tgsi_else(struct r600_shader_ctx 
> *ctx)
>  	r600_bytecode_add_cfinst(ctx->bc, CF_OP_ELSE);
>  	ctx->bc->cf_last->pop_count = 1;
> 
> -	fc_set_mid(ctx, ctx->bc->fc_sp);
> -	ctx->bc->fc_stack[ctx->bc->fc_sp].start->cf_addr = 
> ctx->bc->cf_last->id;
> +	fc_set_mid(ctx, ctx->bc->fc_sp - 1);
> +	ctx->bc->fc_stack[ctx->bc->fc_sp - 1].start->cf_addr = 
> ctx->bc->cf_last->id;
>  	return 0;
>  }
> 
>  static int tgsi_endif(struct r600_shader_ctx *ctx)
>  {
>  	pops(ctx, 1);
> -	if (ctx->bc->fc_stack[ctx->bc->fc_sp].type != FC_IF) {
> +	if (ctx->bc->fc_stack[ctx->bc->fc_sp - 1].type != FC_IF) {
>  		R600_ERR("if/endif unbalanced in shader\n");
>  		return -1;
>  	}
> 
> -	if (ctx->bc->fc_stack[ctx->bc->fc_sp].mid == NULL) {
> -		ctx->bc->fc_stack[ctx->bc->fc_sp].start->cf_addr = 
> ctx->bc->cf_last->id + 2;
> -		ctx->bc->fc_stack[ctx->bc->fc_sp].start->pop_count = 1;
> +	if (ctx->bc->fc_stack[ctx->bc->fc_sp - 1].mid == NULL) {
> +		ctx->bc->fc_stack[ctx->bc->fc_sp - 1].start->cf_addr =
> ctx->bc->cf_last->id + 2;
> +		ctx->bc->fc_stack[ctx->bc->fc_sp - 1].start->pop_count = 1;
>  	} else {
> -		ctx->bc->fc_stack[ctx->bc->fc_sp].mid[0]->cf_addr = 
> ctx->bc->cf_last->id + 2;
> +		ctx->bc->fc_stack[ctx->bc->fc_sp - 1].mid[0]->cf_addr =
> ctx->bc->cf_last->id + 2;
>  	}
>  	fc_poplevel(ctx);
> 
> @@ -8793,7 +8794,7 @@ static int tgsi_endloop(struct r600_shader_ctx 
> *ctx)
> 
>  	r600_bytecode_add_cfinst(ctx->bc, CF_OP_LOOP_END);
> 
> -	if (ctx->bc->fc_stack[ctx->bc->fc_sp].type != FC_LOOP) {
> +	if (ctx->bc->fc_stack[ctx->bc->fc_sp - 1].type != FC_LOOP) {
>  		R600_ERR("loop/endloop in shader code are not paired.\n");
>  		return -EINVAL;
>  	}
> @@ -8803,12 +8804,12 @@ static int tgsi_endloop(struct r600_shader_ctx 
> *ctx)
>  	   LOOP START point to CF after LOOP END
>  	   BRK/CONT point to LOOP END CF
>  	*/
> -	ctx->bc->cf_last->cf_addr = 
> ctx->bc->fc_stack[ctx->bc->fc_sp].start->id + 2;
> +	ctx->bc->cf_last->cf_addr = ctx->bc->fc_stack[ctx->bc->fc_sp -
> 1].start->id + 2;
> 
> -	ctx->bc->fc_stack[ctx->bc->fc_sp].start->cf_addr = 
> ctx->bc->cf_last->id + 2;
> +	ctx->bc->fc_stack[ctx->bc->fc_sp - 1].start->cf_addr =
> ctx->bc->cf_last->id + 2;
> 
> -	for (i = 0; i < ctx->bc->fc_stack[ctx->bc->fc_sp].num_mid; i++) {
> -		ctx->bc->fc_stack[ctx->bc->fc_sp].mid[i]->cf_addr = 
> ctx->bc->cf_last->id;
> +	for (i = 0; i < ctx->bc->fc_stack[ctx->bc->fc_sp - 1].num_mid; i++) {
> +		ctx->bc->fc_stack[ctx->bc->fc_sp - 1].mid[i]->cf_addr = 
> ctx->bc->cf_last->id;
>  	}
>  	/* XXX add LOOPRET support */
>  	fc_poplevel(ctx);
> @@ -8823,7 +8824,7 @@ static int tgsi_loop_breakc(struct 
> r600_shader_ctx *ctx)
> 
>  	for (fscp = ctx->bc->fc_sp; fscp > 0; fscp--)
>  	{
> -		if (FC_LOOP == ctx->bc->fc_stack[fscp].type)
> +		if (FC_LOOP == ctx->bc->fc_stack[fscp - 1].type)
>  			break;
>  	}
>  	if (fscp == 0) {
> @@ -8842,14 +8843,14 @@ static int tgsi_loop_breakc(struct 
> r600_shader_ctx *ctx)
>  		r = r600_bytecode_add_cfinst(ctx->bc, CF_OP_LOOP_BREAK);
>  		if (r)
>  			return r;
> -		fc_set_mid(ctx, fscp);
> +		fc_set_mid(ctx, fscp - 1);
> 
>  		return tgsi_endif(ctx);
>  	} else {
>  		r = emit_logic_pred(ctx, ALU_OP2_PRED_SETE_INT, CF_OP_ALU_BREAK);
>  		if (r)
>  			return r;
> -		fc_set_mid(ctx, fscp);
> +		fc_set_mid(ctx, fscp - 1);
>  	}
> 
>  	return 0;
> @@ -8861,7 +8862,7 @@ static int tgsi_loop_brk_cont(struct 
> r600_shader_ctx *ctx)
> 
>  	for (fscp = ctx->bc->fc_sp; fscp > 0; fscp--)
>  	{
> -		if (FC_LOOP == ctx->bc->fc_stack[fscp].type)
> +		if (FC_LOOP == ctx->bc->fc_stack[fscp - 1].type)
>  			break;
>  	}
> 
> @@ -8872,7 +8873,7 @@ static int tgsi_loop_brk_cont(struct 
> r600_shader_ctx *ctx)
> 
>  	r600_bytecode_add_cfinst(ctx->bc, ctx->inst_info->op);
> 
> -	fc_set_mid(ctx, fscp);
> +	fc_set_mid(ctx, fscp - 1);
> 
>  	return 0;
>  }