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

Mon, 24 Feb 2025 12:53:50 -0800 

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.bouv...@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.mayd...@linaro.org>
Signed-off-by: Hao Wu <wuhao...@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 */

Reply via email to