Hi Dennis,
On 21 October 2016 at 07:40, Dennis Chen <dennis.c...@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.biesheu...@linaro.org>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>
> Signed-off-by: Dennis Chen <dennis.c...@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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
