| Message ID | 20230925194040.68592-3-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | coverity fixes | expand |
On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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. thanks -- PMM
On 26.09.23 13:37, Peter Maydell wrote: > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy > <vsementsov@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 <vsementsov@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.
On Tue, 26 Sept 2023 at 15:12, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > > On 26.09.23 13:37, Peter Maydell wrote: > > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy > > <vsementsov@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 <vsementsov@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. Correct. We shouldn't do that. But we also should not just assert(). > I don't know what the real hardware should do in this case. Then we should find out... Hopefully the specs will say. If they don't then we can do whatever is a reasonable behaviour (eg treat like some other valid value). thanks -- PMM
On 26.09.23 13:37, Peter Maydell wrote: > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy > <vsementsov@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 <vsementsov@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.
On 26.09.23 21:36, Vladimir Sementsov-Ogievskiy wrote: > On 26.09.23 13:37, Peter Maydell wrote: >> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy >> <vsementsov@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 <vsementsov@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. > (i.e. maximum levels, of course we have page-tables of levels 1..3 and 1..4 correspondingly)
On Tue, 26 Sept 2023 at 19:36, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > > On 26.09.23 13:37, Peter Maydell wrote: > > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy > > <vsementsov@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 <vsementsov@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.maydell@linaro.org> thanks -- PMM
On 26.09.23 21:59, Peter Maydell wrote: > On Tue, 26 Sept 2023 at 19:36, Vladimir Sementsov-Ogievskiy > <vsementsov@yandex-team.ru> wrote: >> >> On 26.09.23 13:37, Peter Maydell wrote: >>> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy >>> <vsementsov@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 <vsementsov@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.maydell@linaro.org> > Thanks!
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]; if ((level == VTD_SL_PD_LEVEL || level == VTD_SL_PDP_LEVEL) && (slpte & VTD_SL_PT_PAGE_SIZE_MASK)) {
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 <vsementsov@yandex-team.ru> --- hw/i386/intel_iommu.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)