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

[Naveen N Rao]

On Tue, Nov 21, 2023 at 10:54:36AM +1100, Michael Ellerman wrote:
> Building with GCC 13 (which has -array-bounds enabled) there are several

Thanks, gcc13 indeed helps reproduce the warnings.

> 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().

But, do_vec_load() and do_vec_store() assume that the maximum size is 16 
(the address_ok() check as an example). So, should we be considering a 
bigger hammer to help detect future incorrect use?

Something like the below?

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index a4ab8625061a..ac22136032b8 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -680,6 +680,9 @@ static nokprobe_inline int do_vec_load(int rn, unsigned long ea,
                u8 b[sizeof(__vector128)];
        } u = {};
 
+       if (WARN_ON_ONCE(size > sizeof(u)))
+               return -EINVAL;
+
        if (!address_ok(regs, ea & ~0xfUL, 16))
                return -EFAULT;
        /* align to multiple of size */
@@ -707,6 +710,9 @@ static nokprobe_inline int do_vec_store(int rn, unsigned long ea,
                u8 b[sizeof(__vector128)];
        } u;
 
+       if (WARN_ON_ONCE(size > sizeof(u)))
+               return -EINVAL;
+
        if (!address_ok(regs, ea & ~0xfUL, 16))
                return -EFAULT;
        /* align to multiple of size */


- Naveen