aboutsummaryrefslogtreecommitdiff
path: root/hw
diff options
context:
space:
mode:
authorPeter Maydell <peter.maydell@linaro.org>2022-01-07 17:08:00 +0000
committerPeter Maydell <peter.maydell@linaro.org>2022-01-07 17:08:00 +0000
commit80dcd37feb3a249cdd6a96826836e267df5c7077 (patch)
tree1260ca02682c7c64ba96247e6d567d8c468f2a37 /hw
parent437dc0ea982beb11cb9b4df82baf8aefe6af661c (diff)
hw/intc/arm_gicv3_its: Fix various off-by-one errors
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>
Diffstat (limited to 'hw')
-rw-r--r--hw/intc/arm_gicv3_its.c26
1 files changed, 13 insertions, 13 deletions
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 2949157df3..95c1914eb3 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;