[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL 14/19] hw/intc/arm_gicv3_its: Fix various off-by-one errors
From: |
Peter Maydell |
Subject: |
[PULL 14/19] hw/intc/arm_gicv3_its: Fix various off-by-one errors |
Date: |
Fri, 7 Jan 2022 17:21:37 +0000 |
The ITS code has to check whether various parameters passed in
commands are in-bounds, where the limit is defined in terms of the
number of bits that are available for the parameter. (For example,
the GITS_TYPER.Devbits ID register field specifies the number of
DeviceID bits minus 1, and device IDs passed in the MAPTI and MAPD
command packets must fit in that many bits.)
Currently we have off-by-one bugs in many of these bounds checks.
The typical problem is that we define a max_foo as 1 << n. In
the Devbits example, we set
s->dt.max_ids = 1UL << (GITS_TYPER.Devbits + 1).
However later when we do the bounds check we write
if (devid > s->dt.max_ids) { /* command error */ }
which incorrectly permits a devid of 1 << n.
These bugs will not cause QEMU crashes because the ID values being
checked are only used for accesses into tables held in guest memory
which we access with address_space_*() functions, but they are
incorrect behaviour of our emulation.
Fix them by standardizing on this pattern:
* bounds limits are named num_foos and are the 2^n value
(equal to the number of valid foo values)
* bounds checks are either
if (fooid < num_foos) { good }
or
if (fooid >= num_foos) { bad }
In this commit we fix the handling of the number of IDs
in the device table and the collection table, and the number
of commands that will fit in the command queue.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
include/hw/intc/arm_gicv3_its_common.h | 6 +++---
hw/intc/arm_gicv3_its.c | 26 +++++++++++++-------------
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/include/hw/intc/arm_gicv3_its_common.h
b/include/hw/intc/arm_gicv3_its_common.h
index 85a144b0e49..b32c697207f 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -46,14 +46,14 @@ typedef struct {
bool indirect;
uint16_t entry_sz;
uint32_t page_sz;
- uint32_t max_entries;
- uint32_t max_ids;
+ uint32_t num_entries;
+ uint32_t num_ids;
uint64_t base_addr;
} TableDesc;
typedef struct {
bool valid;
- uint32_t max_entries;
+ uint32_t num_entries;
uint64_t base_addr;
} CmdQDesc;
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 2949157df34..95c1914eb32 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -286,10 +286,10 @@ static bool process_its_cmd(GICv3ITSState *s, uint64_t
value, uint32_t offset,
* In this implementation, in case of guest errors we ignore the
* command and move onto the next command in the queue.
*/
- if (devid > s->dt.max_ids) {
+ if (devid >= s->dt.num_ids) {
qemu_log_mask(LOG_GUEST_ERROR,
- "%s: invalid command attributes: devid %d>%d",
- __func__, devid, s->dt.max_ids);
+ "%s: invalid command attributes: devid %d>=%d",
+ __func__, devid, s->dt.num_ids);
} else if (!dte_valid || !ite_valid || !cte_valid) {
qemu_log_mask(LOG_GUEST_ERROR,
@@ -379,7 +379,7 @@ static bool process_mapti(GICv3ITSState *s, uint64_t value,
uint32_t offset,
max_eventid = 1UL << (FIELD_EX64(dte, DTE, SIZE) + 1);
max_Intid = (1ULL << (GICD_TYPER_IDBITS + 1)) - 1;
- if ((devid > s->dt.max_ids) || (icid > s->ct.max_ids)
+ if ((devid >= s->dt.num_ids) || (icid >= s->ct.num_ids)
|| !dte_valid || (eventid > max_eventid) ||
(((pIntid < GICV3_LPI_INTID_START) || (pIntid > max_Intid)) &&
(pIntid != INTID_SPURIOUS))) {
@@ -497,7 +497,7 @@ static bool process_mapc(GICv3ITSState *s, uint32_t offset)
valid = (value & CMD_FIELD_VALID_MASK);
- if ((icid > s->ct.max_ids) || (rdbase >= s->gicv3->num_cpu)) {
+ if ((icid >= s->ct.num_ids) || (rdbase >= s->gicv3->num_cpu)) {
qemu_log_mask(LOG_GUEST_ERROR,
"ITS MAPC: invalid collection table attributes "
"icid %d rdbase %" PRIu64 "\n", icid, rdbase);
@@ -610,7 +610,7 @@ static bool process_mapd(GICv3ITSState *s, uint64_t value,
uint32_t offset)
valid = (value & CMD_FIELD_VALID_MASK);
- if ((devid > s->dt.max_ids) ||
+ if ((devid >= s->dt.num_ids) ||
(size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
qemu_log_mask(LOG_GUEST_ERROR,
"ITS MAPD: invalid device table attributes "
@@ -649,7 +649,7 @@ static void process_cmdq(GICv3ITSState *s)
wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET);
- if (wr_offset > s->cq.max_entries) {
+ if (wr_offset >= s->cq.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid write offset "
"%d\n", __func__, wr_offset);
@@ -658,7 +658,7 @@ static void process_cmdq(GICv3ITSState *s)
rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET);
- if (rd_offset > s->cq.max_entries) {
+ if (rd_offset >= s->cq.num_entries) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: invalid read offset "
"%d\n", __func__, rd_offset);
@@ -721,7 +721,7 @@ static void process_cmdq(GICv3ITSState *s)
}
if (result) {
rd_offset++;
- rd_offset %= s->cq.max_entries;
+ rd_offset %= s->cq.num_entries;
s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset);
} else {
/*
@@ -824,13 +824,13 @@ static void extract_table_params(GICv3ITSState *s)
td->entry_sz = FIELD_EX64(value, GITS_BASER, ENTRYSIZE) + 1;
td->base_addr = baser_base_addr(value, page_sz);
if (!td->indirect) {
- td->max_entries = (num_pages * page_sz) / td->entry_sz;
+ td->num_entries = (num_pages * page_sz) / td->entry_sz;
} else {
- td->max_entries = (((num_pages * page_sz) /
+ td->num_entries = (((num_pages * page_sz) /
L1TABLE_ENTRY_SIZE) *
(page_sz / td->entry_sz));
}
- td->max_ids = 1ULL << idbits;
+ td->num_ids = 1ULL << idbits;
}
}
@@ -845,7 +845,7 @@ static void extract_cmdq_params(GICv3ITSState *s)
s->cq.valid = FIELD_EX64(value, GITS_CBASER, VALID);
if (s->cq.valid) {
- s->cq.max_entries = (num_pages * GITS_PAGE_SIZE_4K) /
+ s->cq.num_entries = (num_pages * GITS_PAGE_SIZE_4K) /
GITS_CMDQ_ENTRY_SIZE;
s->cq.base_addr = FIELD_EX64(value, GITS_CBASER, PHYADDR);
s->cq.base_addr <<= R_GITS_CBASER_PHYADDR_SHIFT;
--
2.25.1
- [PULL 00/19] target-arm queue, Peter Maydell, 2022/01/07
- [PULL 02/19] target/arm: Add missing FEAT_TLBIOS instructions, Peter Maydell, 2022/01/07
- [PULL 08/19] hw/intc/arm_gicv3_its: Correct setting of TableDesc entry_sz, Peter Maydell, 2022/01/07
- [PULL 11/19] hw/intc/arm_gicv3_its: Use FIELD macros for DTEs, Peter Maydell, 2022/01/07
- [PULL 05/19] hw/intc/arm_gicv3_its: Remove maxids union from TableDesc, Peter Maydell, 2022/01/07
- [PULL 01/19] Add dummy Aspeed AST2600 Display Port MCU (DPMCU), Peter Maydell, 2022/01/07
- [PULL 03/19] hw/intc/arm_gicv3_its: Correct off-by-one bounds check on rdbase, Peter Maydell, 2022/01/07
- [PULL 04/19] hw/intc/arm_gicv3_its: Remove redundant ITS_CTLR_ENABLED define, Peter Maydell, 2022/01/07
- [PULL 06/19] hw/intc/arm_gicv3_its: Don't return early in extract_table_params() loop, Peter Maydell, 2022/01/07
- [PULL 07/19] hw/intc/arm_gicv3_its: Reduce code duplication in extract_table_params(), Peter Maydell, 2022/01/07
- [PULL 14/19] hw/intc/arm_gicv3_its: Fix various off-by-one errors,
Peter Maydell <=
- [PULL 18/19] hw/arm: add i2c muxes to kudo-bmc, Peter Maydell, 2022/01/07
- [PULL 16/19] hw/arm: Add kudo i2c eeproms., Peter Maydell, 2022/01/07
- [PULL 17/19] hw/arm: attach MMC to kudo-bmc, Peter Maydell, 2022/01/07
- [PULL 12/19] hw/intc/arm_gicv3_its: Correct comment about CTE RDBase field size, Peter Maydell, 2022/01/07
- [PULL 10/19] hw/intc/arm_gicv3_its: Correct handling of MAPI, Peter Maydell, 2022/01/07
- [PULL 13/19] hw/intc/arm_gicv3_its: Use FIELD macros for CTEs, Peter Maydell, 2022/01/07
- [PULL 09/19] hw/intc/arm_gicv3_its: Don't misuse GITS_TYPE_PHYSICAL define, Peter Maydell, 2022/01/07
- [PULL 15/19] hw/intc/arm_gicv3_its: Rename max_l2_entries to num_l2_entries, Peter Maydell, 2022/01/07
- [PULL 19/19] hw/arm: kudo add lm75s on bus 13, Peter Maydell, 2022/01/07
- Re: [PULL 00/19] target-arm queue, Richard Henderson, 2022/01/07