Re: [PULL 12/12] target/hppa: add 64 bit support to gdbstub

[Sven Schnelle <svens@stackframe.org>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Sven!
>
> On 10/3/26 08:06, Sven Schnelle wrote:
>> Sven Schnelle <svens@stackframe.org> writes:
>> 
>>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>>
>>>> Hi,
>>>>
>>>> (now merged as commit b2c2d00f48cc5f4486cfba33b505ff86d79cb137)
>>>>
>>>> On 23/12/25 16:50, deller@kernel.org wrote:
>>>>> From: Sven Schnelle <svens@stackframe.org>
>>>>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>>>>> Reviewed-by: Helge Deller <deller@gmx.de>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>> ---
>>>>> target/hppa/gdbstub.c | 62
>>>> ++++++++++++++++++++++++++++---------------
>>>>>    1 file changed, 41 insertions(+), 21 deletions(-)
>>>>> diff --git a/target/hppa/gdbstub.c b/target/hppa/gdbstub.c
>>>>> index 0daa52f7af..777f4a48b9 100644
>>>>> --- a/target/hppa/gdbstub.c
>>>>> +++ b/target/hppa/gdbstub.c
>>>>> @@ -21,16 +21,25 @@
>>>>> @@ -133,24 +142,35 @@ int hppa_cpu_gdb_read_register(CPUState *cs,
>>>> GByteArray *mem_buf, int n)
>>>>>            val = env->cr[30];
>>>>>            break;
>>>>>        case 64 ... 127:
>>>>> - val = extract64(env->fr[(n - 64) / 2], (n & 1 ? 0 : 32), 32);
>>>>> -        break;
>>>>> -    default:
>>>>> -        if (n < 128) {
>>>>> -            val = 0;
>>>>> +        if (hppa_is_pa20(env)) {
>>>>> +            val = env->fr[n - 64];
>>>>
>>>> Coverity reports:
>>>>
>>>>>>>      CID 1645613:         Memory - illegal accesses  (OVERRUN)
>>>>>>>      Overrunning array "env->fr" of 32 8-byte elements at element
>>>>          index 32 (byte offset 263) using index "n - 64" (which
>>>>         evaluates to 32).
>>>
>>> Yes, there's indeed a check missing wether n is < 96. I'll submit a patch later.
>> Looking again there is
>> if (n >= hppa_num_regs(env)) {
>>          return 0;
>> }
>> right at the beginning of both functions, which returns 96 for 64
>> bit
>> mode as the limit so n should have proper bounds already if I'm not mistaken.
>
> I thought the same but wasn't sure. Maybe adding a pair of assert()
> are sufficient to fell safe and make Coverity happy?

Feel free to do so, but I don't want to write code just to make some
code checker happy.