Re: [PATCH] powerpc/lib: Avoid array bounds warnings in vec ops

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 11/20/23 17:54, Michael Ellerman wrote:
> Building with GCC 13 (which has -array-bounds enabled) there are several
> warnings in sstep.c along the lines of:
> 
>    In function ‘do_byte_reverse’,
>        inlined from ‘do_vec_load’ at arch/powerpc/lib/sstep.c:691:3,
>        inlined from ‘emulate_loadstore’ at arch/powerpc/lib/sstep.c:3439:9:
>    arch/powerpc/lib/sstep.c:289:23: error: array subscript 2 is outside array bounds of ‘u8[16]’ {aka ‘unsigned char[16]’} [-Werror=array-bounds=]
>      289 |                 up[2] = byterev_8(up[1]);
>          |                 ~~~~~~^~~~~~~~~~~~~~~~~~
>    arch/powerpc/lib/sstep.c: In function ‘emulate_loadstore’:
>    arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object ‘u’ of size 16
>      681 |         } u = {};
>          |           ^
> 
> do_byte_reverse() supports a size up to 32 bytes, but in these cases the
> caller is only passing a 16 byte buffer. In practice there is no bug,
> do_vec_load() is only called from the LOAD_VMX case in emulate_loadstore().
> That in turn is only reached when analyse_instr() recognises VMX ops,
> and in all cases the size is no greater than 16:
> 
>    $ git grep -w LOAD_VMX arch/powerpc/lib/sstep.c
>    arch/powerpc/lib/sstep.c:                        op->type = MKOP(LOAD_VMX, 0, 1);
>    arch/powerpc/lib/sstep.c:                        op->type = MKOP(LOAD_VMX, 0, 2);
>    arch/powerpc/lib/sstep.c:                        op->type = MKOP(LOAD_VMX, 0, 4);
>    arch/powerpc/lib/sstep.c:                        op->type = MKOP(LOAD_VMX, 0, 16);
> 
> Similarly for do_vec_store().
> 
> Although the warning is incorrect, the code would be safer if it clamped
> the size from the caller to the known size of the buffer. Do that using
> min_t().
> 
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Reported-by: Jan-Benedict Glaw <jbglaw@lug-owl.de>
> Reported-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Build-tested-by: Gustavo A. R. Silva <gustavoars@kernel.org>

This indeed makes all those warnings go away. :)

Thanks, Michael!
--
Gustavo

> ---
>   arch/powerpc/lib/sstep.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index a4ab8625061a..a13f05cfc7db 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -688,7 +688,7 @@ static nokprobe_inline int do_vec_load(int rn, unsigned long ea,
>   	if (err)
>   		return err;
>   	if (unlikely(cross_endian))
> -		do_byte_reverse(&u.b[ea & 0xf], size);
> +		do_byte_reverse(&u.b[ea & 0xf], min_t(size_t, size, sizeof(u)));
>   	preempt_disable();
>   	if (regs->msr & MSR_VEC)
>   		put_vr(rn, &u.v);
> @@ -719,7 +719,7 @@ static nokprobe_inline int do_vec_store(int rn, unsigned long ea,
>   		u.v = current->thread.vr_state.vr[rn];
>   	preempt_enable();
>   	if (unlikely(cross_endian))
> -		do_byte_reverse(&u.b[ea & 0xf], size);
> +		do_byte_reverse(&u.b[ea & 0xf], min_t(size_t, size, sizeof(u)));
>   	return copy_mem_out(&u.b[ea & 0xf], ea, size, regs);
>   }
>   #endif /* CONFIG_ALTIVEC */