[PATCH] ArmPkg: Assign the real INTID value for sanity check

[PATCH] ArmPkg: Assign the real INTID value for sanity check

[Dennis Chen]

Current implementation doesn't assign the INTID value readed from
GICC_IAR to the @InterruptId parameter in case of GICv3, thus make
the sanity check of the INTID in the caller becomes untrustworthy, this
patch is trying to re-assign the @InterruptId to mitigate this issue.

Contributed-under: TianoCore Contribution Agreement 1.0
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Dennis Chen <dennis.chen@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 733488c..6c9ee8b 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -135,27 +135,32 @@ ArmGicAcknowledgeInterrupt (
   OUT UINTN           *InterruptId
   )
 {
-  UINTN Value;
+  UINTN Value, IntID;
   ARM_GIC_ARCH_REVISION Revision;
 
+  // InterruptId is required for the caller to know if a valid or spurious
+  // interrupt has been read
+  ASSERT (InterruptId != NULL);
+
   Revision = ArmGicGetSupportedArchRevision ();
   if (Revision == ARM_GIC_ARCH_REVISION_2) {
     Value = ArmGicV2AcknowledgeInterrupt (GicInterruptInterfaceBase);
-    // InterruptId is required for the caller to know if a valid or spurious
-    // interrupt has been read
-    ASSERT (InterruptId != NULL);
-    if (InterruptId != NULL) {
-      *InterruptId = Value & ARM_GIC_ICCIAR_ACKINTID;
-    }
+    IntID = Value & ARM_GIC_ICCIAR_ACKINTID;
   } else if (Revision == ARM_GIC_ARCH_REVISION_3) {
     Value = ArmGicV3AcknowledgeInterrupt ();
+    // For GICv3, the Value readed from GICC_IAR is the final INTID
+    IntID = Value;
   } else {
     ASSERT_EFI_ERROR (EFI_UNSUPPORTED);
     // Report Spurious interrupt which is what the above controllers would
     // return if no interrupt was available
     Value = 1023;
+    IntID = Value;
   }
 
+  if (InterruptId != NULL)
+	*InterruptId = IntID;
+
   return Value;
 }
 
-- 
2.7.4

Re: [PATCH] ArmPkg: Assign the real INTID value for sanity check

[Ard Biesheuvel]

Hi Dennis,

On 21 October 2016 at 07:40, Dennis Chen <dennis.chen@arm.com> wrote:
> Current implementation doesn't assign the INTID value readed from
> GICC_IAR to the @InterruptId parameter in case of GICv3, thus make
> the sanity check of the INTID in the caller becomes untrustworthy, this
> patch is trying to re-assign the @InterruptId to mitigate this issue.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Dennis Chen <dennis.chen@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 733488c..6c9ee8b 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -135,27 +135,32 @@ ArmGicAcknowledgeInterrupt (
>    OUT UINTN           *InterruptId
>    )
>  {
> -  UINTN Value;
> +  UINTN Value, IntID;
>    ARM_GIC_ARCH_REVISION Revision;
>
> +  // InterruptId is required for the caller to know if a valid or spurious
> +  // interrupt has been read
> +  ASSERT (InterruptId != NULL);
> +

Given the assert here ...

>    Revision = ArmGicGetSupportedArchRevision ();
>    if (Revision == ARM_GIC_ARCH_REVISION_2) {
>      Value = ArmGicV2AcknowledgeInterrupt (GicInterruptInterfaceBase);
> -    // InterruptId is required for the caller to know if a valid or spurious
> -    // interrupt has been read
> -    ASSERT (InterruptId != NULL);
> -    if (InterruptId != NULL) {
> -      *InterruptId = Value & ARM_GIC_ICCIAR_ACKINTID;
> -    }
> +    IntID = Value & ARM_GIC_ICCIAR_ACKINTID;

... I think we can assign *InterruptId directly here, rather than go
via a local ...

>    } else if (Revision == ARM_GIC_ARCH_REVISION_3) {
>      Value = ArmGicV3AcknowledgeInterrupt ();
> +    // For GICv3, the Value readed from GICC_IAR is the final INTID
> +    IntID = Value;

... and here ...

>    } else {
>      ASSERT_EFI_ERROR (EFI_UNSUPPORTED);
>      // Report Spurious interrupt which is what the above controllers would
>      // return if no interrupt was available
>      Value = 1023;
> +    IntID = Value;

... and here ...

>    }
>
> +  if (InterruptId != NULL)
> +       *InterruptId = IntID;
> +

... and drop this hunk.

>    return Value;
>  }
>
> --
> 2.7.4
>