| Message ID | 20231120235436.1569255-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | df99da19c6c24ab65052ae1bc0904f99069478d9 |
| Headers | show |
| Series | powerpc/lib: Avoid array bounds warnings in vec ops | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
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 */
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
Naveen N Rao <naveen@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
On Wed, Nov 22, 2023 at 03:44:07PM +1100, Michael Ellerman wrote: > Naveen N Rao <naveen@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
>> 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. This[1] patch indeed also makes those -Wstringop-overflow warnings go away. :) I'm not subscribed to the list but here are my Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Build-tested-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thank you guys! -- Gustavo [1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-November/265936.html
On Thu, Nov 23, 2023 at 09:17:54AM -0600, Gustavo A. R. Silva wrote: > > > > 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. > > This[1] patch indeed also makes those -Wstringop-overflow warnings go away. :) > > I'm not subscribed to the list but here are my > > Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Build-tested-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks for testing. I intended my patch to go atop Michael's patch since do_fp_load()/do_fp_store() also clamp down the size passed to do_byte_reverse(). While the use of min() isn't strictly necessary with the added check for 'size' at the beginning of the function, it doesn't hurt to have it and Michael's patch does have a better description for the change :) - Naveen
On Tue, 21 Nov 2023 10:54:36 +1100, 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 = {}; > | ^ > > [...] Applied to powerpc/next. [1/1] powerpc/lib: Avoid array bounds warnings in vec ops https://git.kernel.org/powerpc/c/df99da19c6c24ab65052ae1bc0904f99069478d9 cheers
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 */
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> --- arch/powerpc/lib/sstep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)