Re: [PATCH] hw/intc/arm_gicv3: Fix GICv3 redistributor security checking

[Tianrui Wei]

Hi Peter,

Many thanks for your detailed explanation. Upon further reflection, it

seems that I have misinterpreted some of the explanations in

the manual. Sorry for the confusion, the original implementation is

correct.

Thanks,

Tianrui

On Fri, Jul 16, 2021 at 4:32 PM Peter Maydell <peter.maydell@linaro.org> wrote:

On Wed, 14 Jul 2021 at 20:46, Tianrui Wei <tianrui-wei@outlook.com> wrote:
>
> For redistributor to send sgi, we must test NSACR bits in secure mode.
> However, current implementation inverts the security check, wrongly
> skipping this it when the CPU is in secure state, and only carrying out
> the check when the CPU is not secure or security is not implemented.
> This patch corrects this problem by correcting the inversion of CPU
> secure state checking. It has been tested to work with Linux version 5.11
> in both aarch64 and arm version of qemu.
>
> According to “Arm Generic Interrupt Controller Architecture
> Specification GIC architecture version 3 and version 4,” p. 930, 2008.
> Chapter 12, page 530, when there is only one security state implemented,
> GICD.CTLR.DS is always 0, thus checking NSACR in non-secure state. When
> cpu is in secure state, ns = 0, thus the NSACR check is never performed.
>
> Signed-off-by: Tianrui Wei <tianrui-wei@outlook.com>
> Signed-off-by: Jonathan Balkind <jbalkind@ucsb.edu>
> Tested-by: Tianrui Wei <tianrui-wei@outlook.com>
> ---
>  hw/intc/arm_gicv3_redist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index 53da703ed8..84cfcfd18f 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -564,7 +564,7 @@ void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns)
>          return;
>      }
>
> -    if (ns && !(cs->gic->gicd_ctlr & GICD_CTLR_DS)) {
> +    if (!ns && !(cs->gic->gicd_ctlr & GICD_CTLR_DS)) {
>          /* If security is enabled we must test the NSACR bits */
>          int nsaccess = gicr_ns_access(cs, irq);

So, before this change:
 * if the access to ICC_SGI[01]R_EL1 attempting to kick off this SGI
   is done in Secure state, we allow it
 * if the GIC has security disabled (GICD_CTLR.DS is 1), we allow it
 * if the access is from NonSecure and the GIC does not have security
   disabled, we check the NSACR bits to see if we should allow it

With this change, we check the NSACR bits for accesses from Secure
state, and we don't check them for accesses from NonSecure.
This doesn't seem to me to match what the spec requires: in version
IHI0069G of the GICv3 spec, section 12.1.10, the tables show that
only accesses from NonSecure are subject to NSACR checks. (This makes
intuitive sense: the GICR_NSACR is the NonSecure Access Control
Register and it is controls NonSecure accesses, not Secure accesses.)

What bug are you trying to fix with this patch ?

thanks
-- PMM