Naveen N Rao <nav...@kernel.org> writes:
> 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.
Actually that part is no longer true since 0da6e5fd6c37 ("gcc: disable
'-Warray-bounds' for gcc-13 too").
>> 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?
Yeah true.
To be honest I don't know how paranoid we want to get, we could end up
putting WARN's all over the kernel :)
In this case I guess if the size is too large we overflow the buffer on
the kernel stack, so we should at least check the size.
But does it need a WARN? I'm not sure. If we had a case that was passing
a out-of-bound size hopefully we would notice in testing? :)
cheers
> 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
