| Message ID | 20241104123839.533442-1-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/riscv/riscv-iommu: Coverity fixes | expand |
On Mon, 4 Nov 2024 at 12:38, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Hi, > > This series fixes two issues detected by Coverity in the riscv-iommu > code that just went upstream. > > Peter, > > I'm fixing only 2 CIDs because the third one is a false positive: > > *** CID 1564781: Integer handling issues (INTEGER_OVERFLOW) > /builds/qemu-project/qemu/hw/riscv/riscv-iommu-pci.c: 97 in riscv_iommu_pci_realize() > 91 > 92 /* Set device id for trace / debug */ > 93 DEVICE(iommu)->id = g_strdup_printf("%02x:%02x.%01x", > 94 pci_dev_bus_num(dev), PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)); > 95 qdev_realize(DEVICE(iommu), NULL, errp); > 96 > >>> CID 1564781: Integer handling issues (INTEGER_OVERFLOW) > >>> Expression "memory_region_size(&iommu->regs_mr) + 4096UL", which is equal to 4095, where "memory_region_size(&iommu->regs_mr)" is known to be equal to 18446744073709551615, overflows the type that receives it, an unsigned integer 64 bits wide. > 97 memory_region_init(&s->bar0, OBJECT(s), "riscv-iommu-bar0", > 98 QEMU_ALIGN_UP(memory_region_size(&iommu->regs_mr), TARGET_PAGE_SIZE)); > 99 memory_region_add_subregion(&s->bar0, 0, &iommu->regs_mr); > 100 > 101 pcie_endpoint_cap_init(dev, 0); > 102 > ---------- > > The reason is that is that iommu->regs_mr is being initialized in riscv_iommu_realize() > with 'RISCV_IOMMU_REG_SIZE': > > memory_region_init_io(&s->regs_mr, OBJECT(dev), &riscv_iommu_mmio_ops, s, > "riscv-iommu-regs", RISCV_IOMMU_REG_SIZE); > > And we're doing "qdev_realize(DEVICE(iommu), NULL, errp);" right before > the snippet Coverity found as problematic so it's guaranteed to be > initialized. I ran it with a debugger and verified that > QEMU_ALIGN_UP(memory_region_size(&iommu->regs_mr), TARGET_PAGE_SIZE) is > in fact equal to 'RISCV_IOMMU_REG_SIZE' at that point, as intended. > > I was going to set it as false positive in Coverity but decided to > verify with you first. If you agree I'll update the ticket. Yes, this is a false positive. Coverity has a habit of assuming that because in some cases a function can return a particular constant value that it's therefore possible at any callsite. I just marked this false-positive in the UI. Thanks for the prompt fixes for the other two! -- PMM
Hi, This series fixes two issues detected by Coverity in the riscv-iommu code that just went upstream. Peter, I'm fixing only 2 CIDs because the third one is a false positive: *** CID 1564781: Integer handling issues (INTEGER_OVERFLOW) /builds/qemu-project/qemu/hw/riscv/riscv-iommu-pci.c: 97 in riscv_iommu_pci_realize() 91 92 /* Set device id for trace / debug */ 93 DEVICE(iommu)->id = g_strdup_printf("%02x:%02x.%01x", 94 pci_dev_bus_num(dev), PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)); 95 qdev_realize(DEVICE(iommu), NULL, errp); 96 >>> CID 1564781: Integer handling issues (INTEGER_OVERFLOW) >>> Expression "memory_region_size(&iommu->regs_mr) + 4096UL", which is equal to 4095, where "memory_region_size(&iommu->regs_mr)" is known to be equal to 18446744073709551615, overflows the type that receives it, an unsigned integer 64 bits wide. 97 memory_region_init(&s->bar0, OBJECT(s), "riscv-iommu-bar0", 98 QEMU_ALIGN_UP(memory_region_size(&iommu->regs_mr), TARGET_PAGE_SIZE)); 99 memory_region_add_subregion(&s->bar0, 0, &iommu->regs_mr); 100 101 pcie_endpoint_cap_init(dev, 0); 102 ---------- The reason is that is that iommu->regs_mr is being initialized in riscv_iommu_realize() with 'RISCV_IOMMU_REG_SIZE': memory_region_init_io(&s->regs_mr, OBJECT(dev), &riscv_iommu_mmio_ops, s, "riscv-iommu-regs", RISCV_IOMMU_REG_SIZE); And we're doing "qdev_realize(DEVICE(iommu), NULL, errp);" right before the snippet Coverity found as problematic so it's guaranteed to be initialized. I ran it with a debugger and verified that QEMU_ALIGN_UP(memory_region_size(&iommu->regs_mr), TARGET_PAGE_SIZE) is in fact equal to 'RISCV_IOMMU_REG_SIZE' at that point, as intended. I was going to set it as false positive in Coverity but decided to verify with you first. If you agree I'll update the ticket. Daniel Henrique Barboza (2): hw/riscv/riscv-iommu: change 'depth' to int hw/riscv/riscv-iommu: fix riscv_iommu_validate_process_ctx() check hw/riscv/riscv-iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)