On 26.09.23 21:59, Peter Maydell wrote:
On Tue, 26 Sept 2023 at 19:36, Vladimir Sementsov-Ogievskiy
<vsement...@yandex-team.ru> wrote:
On 26.09.23 13:37, Peter Maydell wrote:
On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
<vsement...@yandex-team.ru> wrote:
Add a constant and clear assertion. The assertion also tells Coverity
that we are not going to overflow the array.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
---
hw/i386/intel_iommu.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c0ce896668..2233dbe13a 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1028,12 +1028,17 @@ static dma_addr_t
vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
* vtd_spte_rsvd 4k pages
* vtd_spte_rsvd_large large pages
*/
-static uint64_t vtd_spte_rsvd[5];
-static uint64_t vtd_spte_rsvd_large[5];
+#define VTD_SPTE_RSVD_LEN 5
+static uint64_t vtd_spte_rsvd[VTD_SPTE_RSVD_LEN];
+static uint64_t vtd_spte_rsvd_large[VTD_SPTE_RSVD_LEN];
static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
{
- uint64_t rsvd_mask = vtd_spte_rsvd[level];
+ uint64_t rsvd_mask;
+
+ assert(level < VTD_SPTE_RSVD_LEN);
+
+ rsvd_mask = vtd_spte_rsvd[level];
Looking at the code it is not clear to me why this assertion is
valid. It looks like we are picking up fields from guest-set
configuration (probably in-memory data structures). So we can't
assert() here -- we need to do whatever the real hardware does
if these fields are set to an incorrect value, or at least something
sensible that doesn't crash QEMU.
Finally, seems that assertion is valid. We do check the guest-set configuration:
1. in vtd_decide_config(), we check that s->aw_bits is exactly one of
VTD_HOST_AW_39BIT or VTD_HOST_AW_48BIT.
2. in vtd_init(), in s->cap we set VTD_CAP_SAGAW_39bit (bit 1) and may be
VTD_CAP_SAGAW_48bit (bit 2), but never bit 3 (which would allow 5-level
page-table) or any other bit (i.e. bits 0 and 4 which are reserved).
3. then, as I could follow, both context entry and pasid entry should go through
vtd_is_level_supported(), which checks that level is allowed in s->cap.
So in the code we should work only with levels 3 and 4.
Thanks for working through that. I'm not completely sure if we always
do the level validity check (eg in vtd_dev_to_context_entry() we skip
it if s->root_scalable is true),
when root_scalable is true, level comes from pasid entry, see
vtd_get_iova_level():
static uint32_t vtd_get_iova_level(IntelIOMMUState *s,
VTDContextEntry *ce,
uint32_t pasid)
{
VTDPASIDEntry pe;
if (s->root_scalable) {
vtd_ce_get_rid2pasid_entry(s, ce, &pe, pasid);
return VTD_PE_GET_LEVEL(&pe);
}
return vtd_ce_get_level(ce);
}
and in case of root_scalable=true, in vtd_dev_to_context_entry(), we do check it by
vtd_ce_rid2pasid_check() -> .... -> vtd_get_pe_in_pasid_leaf_table() ->
vtd_is_level_supported()
[but yes, I'm not sure that we correctly check it on _all_ the paths]
but clearly the intention of the code
is to validate the level early. So asserting in this function is fine,
and if the assert ever fires we know we got the validity check wrong
earlier.
A comment something like
/*
* We should have caught a guest-mis-programmed level earlier,
* via vtd_is_level_supported
*/
might help somebody in future if the assert ever does fire.
Could you also add "CID: 1487158, 1487186" to the commit message?
I've just noticed this issue is in our online coverity scan db too
as unresolved.
OK, thanks for checking this.
With those changes,
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Thanks!
--
Best regards,
Vladimir