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

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

[Michael Ellerman]

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

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 */
-- 
2.41.0

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

[Gustavo A. R. Silva]

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 */

Re: [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

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

[Michael Ellerman]

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

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

[Naveen N Rao]

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

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

[Gustavo A. R. Silva]

>> 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

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

[Naveen N Rao]

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

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

[Michael Ellerman]

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