[v2] arm gicv3 ITS: Various bug fixes and refactorings

[PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table

Posted by Peter Maydell 4 years ago

In process_its_cmd(), we read an ICID out of the interrupt table
entry, and then use it as an index into the collection table.  Add a
check that it is within range for the collection table first.

This check is not strictly necessary, because:
 * we range check the ICID from the guest before writing it into
   the interrupt table entry, so the the only way to get an
   out of range ICID in process_its_cmd() is if a badly-behaved
   guest is writing directly to the interrupt table memory
 * the collection table is in guest memory, so QEMU won't fall
   over if we read off the end of it

However, it seems clearer to include the check.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index ef6c0f55ff9..b2f6a8c7f00 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -299,6 +299,13 @@ static ItsCmdResult process_its_cmd(GICv3ITSState *s, uint64_t value,
         return CMD_CONTINUE;
     }
 
+    if (icid >= s->ct.num_ids) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid ICID 0x%x in ITE (table corrupted?)\n",
+                      __func__, icid);
+        return CMD_CONTINUE;
+    }
+
     cte_valid = get_cte(s, icid, &cte, &res);
     if (res != MEMTX_OK) {
         return CMD_STALL;
-- 
2.25.1

Re: [PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table

Posted by Alex Bennée 4 years ago

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

> In process_its_cmd(), we read an ICID out of the interrupt table
> entry, and then use it as an index into the collection table.  Add a
> check that it is within range for the collection table first.
>
> This check is not strictly necessary, because:
>  * we range check the ICID from the guest before writing it into
>    the interrupt table entry, so the the only way to get an
>    out of range ICID in process_its_cmd() is if a badly-behaved
>    guest is writing directly to the interrupt table memory
>  * the collection table is in guest memory, so QEMU won't fall
>    over if we read off the end of it
>
> However, it seems clearer to include the check.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée

Re: [PATCH v2 13/13] hw/intc/arm_gicv3_its: Range-check ICID before indexing into collection table

Posted by Richard Henderson 4 years ago

On 1/12/22 04:10, Peter Maydell wrote:
> In process_its_cmd(), we read an ICID out of the interrupt table
> entry, and then use it as an index into the collection table.  Add a
> check that it is within range for the collection table first.
> 
> This check is not strictly necessary, because:
>   * we range check the ICID from the guest before writing it into
>     the interrupt table entry, so the the only way to get an
>     out of range ICID in process_its_cmd() is if a badly-behaved
>     guest is writing directly to the interrupt table memory
>   * the collection table is in guest memory, so QEMU won't fall
>     over if we read off the end of it
> 
> However, it seems clearer to include the check.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 7 +++++++
>   1 file changed, 7 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~