On Wed, Nov 22, 2023 at 03:44:07PM +1100, Michael Ellerman wrote:
> 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? :)
You're right, a simpler check should suffice. I will send an updated
patch.
Thanks,
Naveen
