Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[Andrey Shumilin <shum.sdl@nppct.ru>]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

 1. s - This argument passes a pointer to the GICState structure that
    contains the state of the interrupt controller. This argument is
    passed first and used correctly.
 2. regno - This is the register number, which in the context of
    gic_cpu_read is calculated as (offset - 0xd0) / 4. In gic_cpu_read
    code, this number is used to identify the specific APR register to
    be read or modified. In gic_apr_ns_view, this number is also used to
    determine which NSAPR register to read or how to compute bits,
    implying that it is used as a register index.
 3. cpu - This is the CPU number used to address a particular CPU in the
    nsapr, apr, and other arrays.

Based on the context, it is important that regno and cpu are passed to 
gic_apr_ns_view in the correct order for correct handling of arrays and 
indexes within this function. An error in the order of the arguments can 
result in incorrect data handling, as arrays are indexed first by CPU 
number and then by register number. In the considered call 
gic_apr_ns_view the arguments are passed in the wrong order

06.05.2024 13:02, Alex Bennée пишет:
> Andrey Shumilin<shum.sdl@nppct.ru>  writes:
>
>> 1 Possibly mismatched call arguments in function 'gic_apr_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and 'int
>>   cpu'.
>> 2 Possibly mismatched call arguments in function 'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and
>>   'int cpu'.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> So this purely a heuristic test based on the parameter names?
>
>>  From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001
>> From: Andrey Shumilin<shum.sdl@nppct.ru>
>> Date: Sun, 5 May 2024 20:13:40 +0300
>> Subject: [PATCH] Patch hw/intc/arm_gic.c
>>
>> Signed-off-by: Andrey Shumilin<shum.sdl@nppct.ru>
>> ---
>>   hw/intc/arm_gic.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 7a34bc0998..47f01e45e3 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -1658,7 +1658,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>>               *data = s->h_apr[gic_get_vcpu_real_id(cpu)];
>>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
>> -            *data = gic_apr_ns_view(s, regno, cpu);
>> +            *data = gic_apr_ns_view(s, cpu, regno);
>>           } else {
>>               *data = s->apr[regno][cpu];
>>           }
>> @@ -1746,7 +1746,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>>               s->h_apr[gic_get_vcpu_real_id(cpu)] = value;
>>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
>> -            gic_apr_write_ns_view(s, regno, cpu, value);
>> +            gic_apr_write_ns_view(s, cpu, regno, value);
>>           } else {
>>               s->apr[regno][cpu] = value;
>>           }
> Ahh C's lack of strong typing wins again :-/
>
> Reviewed-by: Alex Bennée<alex.bennee@linaro.org>
>

[-- Attachment #2: Type: text/html, Size: 4153 bytes --]