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), 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.
With those changes,
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
thanks
-- PMM
