Re: [PATCH v5 06/17] hw/misc: Add nr_regs and cold_reset_values to NPCM GCR

[Hao Wu <wuhaotsh@google.com>]

[-- Attachment #1: Type: text/plain, Size: 6983 bytes --]

Thanks! I can review the patch.

On Mon, Feb 24, 2025 at 12:52 PM Pierrick Bouvier <
pierrick.bouvier@linaro.org> wrote:

> Hello,
>
> This patch introduces a buffer-overflow, now reported by address sanitizer.
>
> I sent a patch:
> https://lore.kernel.org/qemu-devel/20250224205053.104959-1-
> pierrick.bouvier@linaro.org/T/#u
>
> You're welcome to review it, or fix the problem differently if there is
> a better approach.
>
> Regards,
> Pierrick
>
> On 2/19/25 10:45, Hao Wu wrote:
> > These 2 values are different between NPCM7XX and NPCM8XX
> > GCRs. So we add them to the class and assign different values
> > to them.
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Hao Wu <wuhaotsh@google.com>
> > ---
> >   hw/misc/npcm_gcr.c         | 27 ++++++++++++++++-----------
> >   include/hw/misc/npcm_gcr.h | 13 +++++++++++--
> >   2 files changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/misc/npcm_gcr.c b/hw/misc/npcm_gcr.c
> > index 0959f2e5c4..d89e8c2c3b 100644
> > --- a/hw/misc/npcm_gcr.c
> > +++ b/hw/misc/npcm_gcr.c
> > @@ -66,10 +66,9 @@ enum NPCM7xxGCRRegisters {
> >       NPCM7XX_GCR_SCRPAD          = 0x013c / sizeof(uint32_t),
> >       NPCM7XX_GCR_USB1PHYCTL,
> >       NPCM7XX_GCR_USB2PHYCTL,
> > -    NPCM7XX_GCR_REGS_END,
> >   };
> >
> > -static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = {
> > +static const uint32_t npcm7xx_cold_reset_values[NPCM7XX_GCR_NR_REGS] = {
> >       [NPCM7XX_GCR_PDID]          = 0x04a92750,   /* Poleg A1 */
> >       [NPCM7XX_GCR_MISCPE]        = 0x0000ffff,
> >       [NPCM7XX_GCR_SPSWC]         = 0x00000003,
> > @@ -88,8 +87,9 @@ static uint64_t npcm_gcr_read(void *opaque, hwaddr
> offset, unsigned size)
> >   {
> >       uint32_t reg = offset / sizeof(uint32_t);
> >       NPCMGCRState *s = opaque;
> > +    NPCMGCRClass *c = NPCM_GCR_GET_CLASS(s);
> >
> > -    if (reg >= NPCM7XX_GCR_NR_REGS) {
> > +    if (reg >= c->nr_regs) {
> >           qemu_log_mask(LOG_GUEST_ERROR,
> >                         "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> >                         __func__, offset);
> > @@ -106,11 +106,12 @@ static void npcm_gcr_write(void *opaque, hwaddr
> offset,
> >   {
> >       uint32_t reg = offset / sizeof(uint32_t);
> >       NPCMGCRState *s = opaque;
> > +    NPCMGCRClass *c = NPCM_GCR_GET_CLASS(s);
> >       uint32_t value = v;
> >
> > -    trace_npcm_gcr_write(offset, value);
> > +    trace_npcm_gcr_write(offset, v);
> >
> > -    if (reg >= NPCM7XX_GCR_NR_REGS) {
> > +    if (reg >= c->nr_regs) {
> >           qemu_log_mask(LOG_GUEST_ERROR,
> >                         "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> >                         __func__, offset);
> > @@ -156,10 +157,12 @@ static const struct MemoryRegionOps npcm_gcr_ops =
> {
> >   static void npcm7xx_gcr_enter_reset(Object *obj, ResetType type)
> >   {
> >       NPCMGCRState *s = NPCM_GCR(obj);
> > +    NPCMGCRClass *c = NPCM_GCR_GET_CLASS(obj);
> >
> > -    QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> > -
> > -    memcpy(s->regs, cold_reset_values, sizeof(s->regs));
> > +    g_assert(sizeof(s->regs) >= sizeof(c->cold_reset_values));
> > +    g_assert(sizeof(s->regs) >= c->nr_regs * sizeof(uint32_t));
> > +    memcpy(s->regs, c->cold_reset_values, c->nr_regs *
> sizeof(uint32_t));
> > +    /* These 3 registers are at the same location in both 7xx and 8xx.
> */
> >       s->regs[NPCM7XX_GCR_PWRON] = s->reset_pwron;
> >       s->regs[NPCM7XX_GCR_MDLR] = s->reset_mdlr;
> >       s->regs[NPCM7XX_GCR_INTCR3] = s->reset_intcr3;
> > @@ -224,7 +227,7 @@ static const VMStateDescription vmstate_npcm_gcr = {
> >       .version_id = 1,
> >       .minimum_version_id = 1,
> >       .fields = (const VMStateField[]) {
> > -        VMSTATE_UINT32_ARRAY(regs, NPCMGCRState, NPCM7XX_GCR_NR_REGS),
> > +        VMSTATE_UINT32_ARRAY(regs, NPCMGCRState, NPCM_GCR_MAX_NR_REGS),
> >           VMSTATE_END_OF_LIST(),
> >       },
> >   };
> > @@ -238,7 +241,6 @@ static void npcm_gcr_class_init(ObjectClass *klass,
> void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> >
> > -    QEMU_BUILD_BUG_ON(NPCM7XX_GCR_REGS_END > NPCM7XX_GCR_NR_REGS);
> >       dc->realize = npcm_gcr_realize;
> >       dc->vmsd = &vmstate_npcm_gcr;
> >
> > @@ -247,13 +249,15 @@ static void npcm_gcr_class_init(ObjectClass
> *klass, void *data)
> >
> >   static void npcm7xx_gcr_class_init(ObjectClass *klass, void *data)
> >   {
> > +    NPCMGCRClass *c = NPCM_GCR_CLASS(klass);
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> >       ResettableClass *rc = RESETTABLE_CLASS(klass);
> >
> > -    QEMU_BUILD_BUG_ON(NPCM7XX_GCR_REGS_END != NPCM7XX_GCR_NR_REGS);
> >       dc->desc = "NPCM7xx System Global Control Registers";
> >       rc->phases.enter = npcm7xx_gcr_enter_reset;
> >
> > +    c->nr_regs = NPCM7XX_GCR_NR_REGS;
> > +    c->cold_reset_values = npcm7xx_cold_reset_values;
> >   }
> >
> >   static const TypeInfo npcm_gcr_info[] = {
> > @@ -262,6 +266,7 @@ static const TypeInfo npcm_gcr_info[] = {
> >           .parent             = TYPE_SYS_BUS_DEVICE,
> >           .instance_size      = sizeof(NPCMGCRState),
> >           .instance_init      = npcm_gcr_init,
> > +        .class_size         = sizeof(NPCMGCRClass),
> >           .class_init         = npcm_gcr_class_init,
> >           .abstract           = true,
> >       },
> > diff --git a/include/hw/misc/npcm_gcr.h b/include/hw/misc/npcm_gcr.h
> > index 6d3d00d260..9af24e5cdc 100644
> > --- a/include/hw/misc/npcm_gcr.h
> > +++ b/include/hw/misc/npcm_gcr.h
> > @@ -18,6 +18,7 @@
> >
> >   #include "exec/memory.h"
> >   #include "hw/sysbus.h"
> > +#include "qom/object.h"
> >
> >   /*
> >    * NPCM7XX PWRON STRAP bit fields
> > @@ -53,6 +54,7 @@
> >    * Number of registers in our device state structure. Don't change
> this without
> >    * incrementing the version_id in the vmstate.
> >    */
> > +#define NPCM_GCR_MAX_NR_REGS NPCM7XX_GCR_NR_REGS
> >   #define NPCM7XX_GCR_NR_REGS (0x148 / sizeof(uint32_t))
> >
> >   typedef struct NPCMGCRState {
> > @@ -60,15 +62,22 @@ typedef struct NPCMGCRState {
> >
> >       MemoryRegion iomem;
> >
> > -    uint32_t regs[NPCM7XX_GCR_NR_REGS];
> > +    uint32_t regs[NPCM_GCR_MAX_NR_REGS];
> >
> >       uint32_t reset_pwron;
> >       uint32_t reset_mdlr;
> >       uint32_t reset_intcr3;
> >   } NPCMGCRState;
> >
> > +typedef struct NPCMGCRClass {
> > +    SysBusDeviceClass parent;
> > +
> > +    size_t nr_regs;
> > +    const uint32_t *cold_reset_values;
> > +} NPCMGCRClass;
> > +
> >   #define TYPE_NPCM_GCR "npcm-gcr"
> >   #define TYPE_NPCM7XX_GCR "npcm7xx-gcr"
> > -OBJECT_DECLARE_SIMPLE_TYPE(NPCMGCRState, NPCM_GCR)
> > +OBJECT_DECLARE_TYPE(NPCMGCRState, NPCMGCRClass, NPCM_GCR)
> >
> >   #endif /* NPCM_GCR_H */
>
>

[-- Attachment #2: Type: text/html, Size: 9102 bytes --]