target/arm: clamp value to account for RES0 fields

[RFC PATCH] target/arm: clamp value to account for RES0 fields

Posted by Alex Bennée 5 months ago

If the user writes a large value to the register but with the bottom
bits unset we could end up with something illegal. By clamping ahead
of the check we at least assure we won't assert(bpr > 0) later in the
GIC interface code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 4b4cf09157..165f7e9c2f 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -1797,6 +1797,9 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     trace_gicv3_icc_bpr_write(ri->crm == 8 ? 0 : 1,
                               gicv3_redist_affid(cs), value);
 
+    /* clamp the value to 2:0, the rest os RES0 */
+    value = deposit64(0, 0, 3, value);
+
     if (grp == GICV3_G1 && gicv3_use_ns_bank(env)) {
         grp = GICV3_G1NS;
     }
@@ -1820,7 +1823,7 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         value = minval;
     }
 
-    cs->icc_bpr[grp] = value & 7;
+    cs->icc_bpr[grp] = value;
     gicv3_cpuif_update(cs);
 }
 
-- 
2.47.2

Re: [RFC PATCH] target/arm: clamp value to account for RES0 fields

Posted by Peter Maydell 4 months, 4 weeks ago

On Mon, 16 Jun 2025 at 21:10, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> If the user writes a large value to the register but with the bottom
> bits unset we could end up with something illegal. By clamping ahead
> of the check we at least assure we won't assert(bpr > 0) later in the
> GIC interface code.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/intc/arm_gicv3_cpuif.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 4b4cf09157..165f7e9c2f 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -1797,6 +1797,9 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      trace_gicv3_icc_bpr_write(ri->crm == 8 ? 0 : 1,
>                                gicv3_redist_affid(cs), value);
>
> +    /* clamp the value to 2:0, the rest os RES0 */
> +    value = deposit64(0, 0, 3, value);

Should be extract64(), as RTH notes (or just &= 7 if you like).

> +
>      if (grp == GICV3_G1 && gicv3_use_ns_bank(env)) {
>          grp = GICV3_G1NS;
>      }
> @@ -1820,7 +1823,7 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          value = minval;
>      }
>
> -    cs->icc_bpr[grp] = value & 7;
> +    cs->icc_bpr[grp] = value;
>      gicv3_cpuif_update(cs);

Yes, I agree we should do the "work only on the 3 bit field"
part before we do the "enforce the minimum value" logic.

The handling in icv_bpr_write() has a similar issue.

(Why was your guest writing garbage to this register?)

-- PMM

Re: [RFC PATCH] target/arm: clamp value to account for RES0 fields

Posted by Alex Bennée 4 months, 4 weeks ago

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 16 Jun 2025 at 21:10, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> If the user writes a large value to the register but with the bottom
>> bits unset we could end up with something illegal. By clamping ahead
>> of the check we at least assure we won't assert(bpr > 0) later in the
>> GIC interface code.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  hw/intc/arm_gicv3_cpuif.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
>> index 4b4cf09157..165f7e9c2f 100644
>> --- a/hw/intc/arm_gicv3_cpuif.c
>> +++ b/hw/intc/arm_gicv3_cpuif.c
>> @@ -1797,6 +1797,9 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>      trace_gicv3_icc_bpr_write(ri->crm == 8 ? 0 : 1,
>>                                gicv3_redist_affid(cs), value);
>>
>> +    /* clamp the value to 2:0, the rest os RES0 */
>> +    value = deposit64(0, 0, 3, value);
>
> Should be extract64(), as RTH notes (or just &= 7 if you like).
>
>> +
>>      if (grp == GICV3_G1 && gicv3_use_ns_bank(env)) {
>>          grp = GICV3_G1NS;
>>      }
>> @@ -1820,7 +1823,7 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>          value = minval;
>>      }
>>
>> -    cs->icc_bpr[grp] = value & 7;
>> +    cs->icc_bpr[grp] = value;
>>      gicv3_cpuif_update(cs);
>
> Yes, I agree we should do the "work only on the 3 bit field"
> part before we do the "enforce the minimum value" logic.
>
> The handling in icv_bpr_write() has a similar issue.
>
> (Why was your guest writing garbage to this register?)

Heh, it was writing the SP instead of the XR, this is how I found out I
was getting it wrong ;-)

>
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH] target/arm: clamp value to account for RES0 fields

Posted by Richard Henderson 5 months ago

On 6/16/25 13:10, Alex Bennée wrote:
> If the user writes a large value to the register but with the bottom
> bits unset we could end up with something illegal. By clamping ahead
> of the check we at least assure we won't assert(bpr > 0) later in the
> GIC interface code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/intc/arm_gicv3_cpuif.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 4b4cf09157..165f7e9c2f 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -1797,6 +1797,9 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>       trace_gicv3_icc_bpr_write(ri->crm == 8 ? 0 : 1,
>                                 gicv3_redist_affid(cs), value);
>   
> +    /* clamp the value to 2:0, the rest os RES0 */
> +    value = deposit64(0, 0, 3, value);

Surely extract, not deposit.

r~

> +
>       if (grp == GICV3_G1 && gicv3_use_ns_bank(env)) {
>           grp = GICV3_G1NS;
>       }
> @@ -1820,7 +1823,7 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>           value = minval;
>       }
>   
> -    cs->icc_bpr[grp] = value & 7;
> +    cs->icc_bpr[grp] = value;
>       gicv3_cpuif_update(cs);
>   }
>