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(-)

-- 
2.45.2

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