Thanks! I can review the patch.
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 */