Re: [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> In process_its_cmd() and process_mapti() we must check the
> event ID against a limit defined by the size field in the DTE,
> which specifies the number of ID bits minus one. Convert
> this code to our num_foo convention:
>  * change the variable names
>  * use uint64_t and 1ULL when calculating the number
>    of valid event IDs, because DTE.SIZE is 5 bits and
>    so num_eventids may be up to 2^32
>  * fix the off-by-one error in the comparison
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv3_its.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index fa3cdb57554..6d11fa02040 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -225,7 +225,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
>      MemTxResult res = MEMTX_OK;
>      bool dte_valid;
>      uint64_t dte = 0;
> -    uint32_t max_eventid;
> +    uint64_t num_eventids;
>      uint16_t icid = 0;
>      uint32_t pIntid = 0;
>      bool ite_valid = false;
> @@ -258,7 +258,7 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
>      dte_valid = FIELD_EX64(dte, DTE, VALID);
>  
>      if (dte_valid) {
> -        max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);
> +        num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
>  
>          ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
>  
> @@ -299,10 +299,11 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t value, uint32_t offset,
>                        dte_valid ? "valid" : "invalid",
>                        ite_valid ? "valid" : "invalid",
>                        cte_valid ? "valid" : "invalid");
> -    } else if (eventid > max_eventid) {
> +    } else if (eventid >= num_eventids) {
>          qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: invalid command attributes: eventid %d > %d\n",
> -                      __func__, eventid, max_eventid);
> +                      "%s: invalid command attributes: eventid %d >= %"
> +                      PRId64 "\n",
> +                      __func__, eventid, num_eventids);
>      } else {
>          /*
>           * Current implementation only supports rdbase == procnum
> @@ -336,7 +337,8 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
>      AddressSpace *as = &s->gicv3->dma_as;
>      uint32_t devid, eventid;
>      uint32_t pIntid = 0;
> -    uint32_t max_eventid, max_Intid;
> +    uint64_t num_eventids;
> +    uint32_t max_Intid;
>      bool dte_valid;
>      MemTxResult res = MEMTX_OK;
>      uint16_t icid = 0;
> @@ -376,11 +378,11 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value, uint32_t offset,
>          return result;
>      }
>      dte_valid = FIELD_EX64(dte, DTE, VALID);
> -    max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);
> +    num_eventids = 1ULL << (FIELD_EX64(dte, DTE, SIZE) + 1);
>      max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1;
>  
>      if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids)
> -            || !dte_valid || (eventid > max_eventid) ||
> +            || !dte_valid || (eventid >= num_eventids) ||
>              (((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) &&
>               (pIntid != INTID_SPURIOUS))) {

It seems process_mapti has the similar "catch all the errors" if leg
that I split apart in process_its_command to make it easier to see what
failed.

However:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée