[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks
From: |
Alex Bennée |
Subject: |
Re: [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks |
Date: |
Tue, 18 Jan 2022 17:13:58 +0000 |
User-agent: |
mu4e 1.7.5; emacs 28.0.91 |
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
- [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings, Peter Maydell, 2022/01/11
- [PATCH v2 06/13] hw/intc/arm_gicv3_its: Fix return codes in process_its_cmd(), Peter Maydell, 2022/01/11
- [PATCH v2 08/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapti(), Peter Maydell, 2022/01/11
- [PATCH v2 04/13] hw/intc/arm_gicv3_its: Don't use data if reading command failed, Peter Maydell, 2022/01/11
- [PATCH v2 02/13] hw/intc/arm_gicv3_its: Convert int ID check to num_intids convention, Peter Maydell, 2022/01/11
- [PATCH v2 01/13] hw/intc/arm_gicv3_its: Fix event ID bounds checks, Peter Maydell, 2022/01/11
- [PATCH v2 03/13] hw/intc/arm_gicv3_its: Fix handling of process_its_cmd() return value, Peter Maydell, 2022/01/11
- [PATCH v2 05/13] hw/intc/arm_gicv3_its: Use enum for return value of process_* functions, Peter Maydell, 2022/01/11
- [PATCH v2 07/13] hw/intc/arm_gicv3_its: Refactor process_its_cmd() to reduce nesting, Peter Maydell, 2022/01/11
- [PATCH v2 10/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapd(), Peter Maydell, 2022/01/11
- [PATCH v2 09/13] hw/intc/arm_gicv3_its: Fix return codes in process_mapc(), Peter Maydell, 2022/01/11
- [PATCH v2 11/13] hw/intc/arm_gicv3_its: Factor out "find address of table entry" code, Peter Maydell, 2022/01/11
- [PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table, Peter Maydell, 2022/01/11