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.
But touching vtd_spte_rsvd with level>=5 is even worse than assertion, I think. That's overflows the array. I don't know what the real hardware should do in this case. So, this assertion just stresses that the code is incomplete and makes it easier to find a bug if it comes. -- Best regards, Vladimir
