Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 11/20/23 09:21, Naveen N Rao wrote:
> On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote:
>>
>>
>> On 11/20/23 08:25, Naveen N Rao wrote:
>>> On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
>>>> Hi all,
>>>>
>>>> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
>>>> to get your feedback on this, please:
>>>>
>>>> In function 'do_byte_reverse',
>>>>       inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>>>>       inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>>     287 |                 up[3] = tmp;
>>>>         |                 ~~~~~~^~~~~
>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>> arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>     708 |         } u;
>>>>         |           ^
>>>> In function 'do_byte_reverse',
>>>>       inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>>>>       inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>>>> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>>     289 |                 up[2] = byterev_8(up[1]);
>>>>         |                 ~~~~~~^~~~~~~~~~~~~~~~~~
>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>> arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u' of size 16
>>>>     708 |         } u;
>>>>         |           ^
>>>> In function 'do_byte_reverse',
>>>>       inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
>>>>       inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>>     287 |                 up[3] = tmp;
>>>>         |                 ~~~~~~^~~~~
>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>     681 |         } u = {};
>>>>         |           ^
>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>
>>>> The origing of the issue seems to be the following calls to function `do_vec_store()`, when
>>>> `size > 16`, or more precisely when `size == 32`
>>>>
>>>> arch/powerpc/lib/sstep.c:
>>>> 3436         case LOAD_VMX:
>>>> 3437                 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>>>> 3438                         return 0;
>>>> 3439                 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
>>>> 3440                 break;
>>>> ...
>>>> 3507         case STORE_VMX:
>>>> 3508                 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>>>> 3509                         return 0;
>>>> 3510                 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
>>>> 3511                 break;
>>>
>>> LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
>>> exceed 16. So, the warning looks incorrect to me.
>>
>> Does that mean that this piece of code is never actually executed:
>>
>>   281         case 32: {
>>   282                 unsigned long *up = (unsigned long *)ptr;
>>   283                 unsigned long tmp;
>>   284
>>   285                 tmp = byterev_8(up[0]);
>>   286                 up[0] = byterev_8(up[3]);
>>   287                 up[3] = tmp;
>>   288                 tmp = byterev_8(up[2]);
>>   289                 up[2] = byterev_8(up[1]);
>>   290                 up[1] = tmp;
>>   291                 break;
>>   292         }
>>
>> or rather, under which conditions the above code is executed, if any?
> 
> Please see git history to understand the commit that introduced that
> code. You can do that by starting with a 'git blame' on the file. You'll
> find that the commit that introduced the above hunk was af99da74333b
> ("powerpc/sstep: Support VSX vector paired storage access
> instructions").
> 
> The above code path is hit via
> do_vsx_load()->emulate_vsx_load()->do_byte_reverse()

It seems there is another path (at least the one that triggers the
-Wstringop-overflow warnings):

emulate_step()->emulate_loadstore()->do_vec_load()->do_byte_reverse()

emulate_step() {
...
if (OP_IS_LOAD_STORE(type)) {
                 err = emulate_loadstore(regs, &op);
                 if (err)
                         return 0;
                 goto instr_done;
         }
...
}

----> emulate_loadstore() {
...
#ifdef CONFIG_ALTIVEC
         case LOAD_VMX:
                 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
                         return 0;
                 err = do_vec_load(op->reg, ea, size, regs, cross_endian); // with size == 32
                 break;
#endif
...
#ifdef CONFIG_ALTIVEC
         case STORE_VMX:
                 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
                         return 0;
                 err = do_vec_store(op->reg, ea, size, regs, cross_endian); // with size == 32
                 break;
#endif
...
}

----> do_vec_load() // Here is where we have the union of size 16 bytes:
{
...
         union {
                 __vector128 v;
                 u8 b[sizeof(__vector128)];
         } u = {};
...
	if (unlikely(cross_endian))
                 do_byte_reverse(&u.b[ea & 0xf], size);
...
}

----> do_byte_reverse()
{
...
         case 32: {
                 unsigned long *up = (unsigned long *)ptr;  // this is a pointer to u.b
                 unsigned long tmp;

                 tmp = byterev_8(up[0]);
                 up[0] = byterev_8(up[3]);
                 up[3] = tmp;		    // and here...
                 tmp = byterev_8(up[2]);
                 up[2] = byterev_8(up[1]);   // and here we are accessing ptr out-of-bounds
                 up[1] = tmp;
                 break;
         }
...
}

This is where I'm trying to determine if there is a bug. If this path is actually
executed, then it seems we have an issue.

Is it possible that commit af99da74333b overlooked the path described above?

--
Gustavo