1. Patchew
  2. QEMU
  3. View series

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

Tianrui Wei posted 1 patch 2 years, 9 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/SY4PR01MB6798FDE7B97E478254D6B955F6139@SY4PR01MB6798.ausprd01.prod.outlook.com
hw/intc/arm_gicv3_redist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] hw/intc/arm_gicv3: Fix GICv3 redistributor security checking
Posted by Tianrui Wei 2 years, 9 months ago 
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);
 
-- 
2.32.0
Re: [PATCH] hw/intc/arm_gicv3: Fix GICv3 redistributor security checking
Posted by Peter Maydell 2 years, 9 months ago 
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
Re: [PATCH] hw/intc/arm_gicv3: Fix GICv3 redistributor security checking
Posted by Tianrui Wei 2 years, 9 months ago 
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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.