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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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
>