Re: [PULL 34/56] hw/intc/grlib_irqmp: add ncpus property

[Clément Chigot]

On Fri, Mar 8, 2024 at 2:27 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 15 Feb 2024 at 18:04, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
> >
> > From: Clément Chigot <chigot@adacore.com>
> >
> > This adds a "ncpus" property to the "grlib-irqmp" device to be used
> > later, this required a little refactoring of how we initialize the
> > device (ie: use realize instead of init).
> >
> > Co-developed-by: Frederic Konrad <konrad.frederic@yahoo.fr>
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Message-ID: <20240131085047.18458-3-chigot@adacore.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Hi; Coverity points out a bug in this commit (CID 1534914):
>
>
> > -static void grlib_irqmp_init(Object *obj)
> > +static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
> >  {
> > -    IRQMP *irqmp = GRLIB_IRQMP(obj);
> > -    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > +    IRQMP *irqmp = GRLIB_IRQMP(dev);
> >
> > -    qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
> > -    qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
> > -    memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
> > +    if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
> > +        error_setg(errp, "Invalid ncpus properties: "
> > +                   "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
> > +                   IRQMP_MAX_CPU);
> > +    }
>
> We detect the out-of-range 'ncpus' value, but forget the "return"
> statement, so execution will continue onward regardless, and
> overrun the irqmp->irq[] array when we call qdev_init_gpio_out_named().

Indeed, I'll send a patch.
Thanks for pointing that out.

Clément

> > +
> > +    qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
> > +    qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
> > +    memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, 
> > irqmp,
> >                            "irqmp", IRQMP_REG_SIZE);
> >
> >      irqmp->state = g_malloc0(sizeof *irqmp->state);
> >
> > -    sysbus_init_mmio(dev, &irqmp->iomem);
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
> >  }
>
> thanks
> -- PMM