Message ID | 20240221162636.173136-4-thuth@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | None | expand |
On 21/02/2024 17.26, Thomas Huth wrote: > From: Peter Maydell <peter.maydell@linaro.org> > > QEMU has historically used variable length arrays only very rarely. > Variable length arrays are a potential security issue where an > on-stack dynamic allocation isn't correctly size-checked, especially > when the size comes from the guest. (An example problem of this kind > from the past is CVE-2021-3527). Forbidding them entirely is a > defensive measure against further bugs of this kind. > > Enable -Wvla to prevent any new uses from sneaking into the codebase. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Message-ID: <20240125173211.1786196-3-peter.maydell@linaro.org> > [thuth: rebased to current master branch] > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > meson.build | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/meson.build b/meson.build > index c1dc83e4c0..0ef1654e86 100644 > --- a/meson.build > +++ b/meson.build > @@ -592,6 +592,7 @@ warn_flags = [ > '-Wstrict-prototypes', > '-Wtype-limits', > '-Wundef', > + '-Wvla', > '-Wwrite-strings', > > # Then disable some undesirable warnings Sigh, there's a new warning in the latest master branch: https://gitlab.com/thuth/qemu/-/jobs/6225992174 Caused by commit d65aba828 ("hw/sparc/leon3: implement multiprocessor")... Clément, Philippe, could this maybe be written in a different way that does not trigger a -Wvla warning? Thomas
On 21/2/24 17:59, Thomas Huth wrote: > On 21/02/2024 17.26, Thomas Huth wrote: >> From: Peter Maydell <peter.maydell@linaro.org> >> >> QEMU has historically used variable length arrays only very rarely. >> Variable length arrays are a potential security issue where an >> on-stack dynamic allocation isn't correctly size-checked, especially >> when the size comes from the guest. (An example problem of this kind >> from the past is CVE-2021-3527). Forbidding them entirely is a >> defensive measure against further bugs of this kind. >> >> Enable -Wvla to prevent any new uses from sneaking into the codebase. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> Message-ID: <20240125173211.1786196-3-peter.maydell@linaro.org> >> [thuth: rebased to current master branch] >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> meson.build | 1 + >> 1 file changed, 1 insertion(+) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> diff --git a/meson.build b/meson.build >> index c1dc83e4c0..0ef1654e86 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -592,6 +592,7 @@ warn_flags = [ >> '-Wstrict-prototypes', >> '-Wtype-limits', >> '-Wundef', >> + '-Wvla', >> '-Wwrite-strings', >> # Then disable some undesirable warnings > > Sigh, there's a new warning in the latest master branch: > > https://gitlab.com/thuth/qemu/-/jobs/6225992174 > > Caused by commit d65aba828 ("hw/sparc/leon3: implement multiprocessor")... > Clément, Philippe, could this maybe be written in a different way that > does not trigger a -Wvla warning? Clément, ResetData::entry isn't used, so we can simplify removing the whole ResetData structure, but I'm not sure this is intended: -- >8 -- diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c index 4873b59b6c..1ff6b5d63d 100644 --- a/hw/sparc/leon3.c +++ b/hw/sparc/leon3.c @@ -68,14 +68,6 @@ #define LEON3_APB_PNP_OFFSET (0x800FF000) #define LEON3_AHB_PNP_OFFSET (0xFFFFF000) -typedef struct ResetData { - struct CPUResetData { - int id; - SPARCCPU *cpu; - } info[MAX_CPUS]; - uint32_t entry; /* save kernel entry in case of reset */ -} ResetData; - static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val) { stl_p(code++, 0x82100000); /* mov %g0, %g1 */ @@ -148,17 +140,14 @@ static void write_bootloader(void *ptr, hwaddr kernel_addr) static void leon3_cpu_reset(void *opaque) { - struct CPUResetData *info = (struct CPUResetData *) opaque; - int id = info->id; - ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info); - CPUState *cpu = CPU(s->info[id].cpu); + CPUState *cpu = opaque; CPUSPARCState *env = cpu_env(cpu); cpu_reset(cpu); cpu->halted = cpu->cpu_index != 0; - env->pc = s->entry; - env->npc = s->entry + 4; + env->pc = LEON3_PROM_OFFSET; + env->npc = LEON3_PROM_OFFSET + 4; } static void leon3_cache_control_int(CPUSPARCState *env) @@ -259,7 +248,7 @@ static void leon3_generic_hw_init(MachineState *machine) ram_addr_t ram_size = machine->ram_size; const char *bios_name = machine->firmware ?: LEON3_PROM_FILENAME; const char *kernel_filename = machine->kernel_filename; - SPARCCPU *cpu; + SPARCCPU *cpu[MAX_CPUS]; CPUSPARCState *env; MemoryRegion *address_space_mem = get_system_memory(); MemoryRegion *prom = g_new(MemoryRegion, 1); @@ -267,28 +256,22 @@ static void leon3_generic_hw_init(MachineState *machine) char *filename; int bios_size; int prom_size; - ResetData *reset_info; DeviceState *dev, *irqmpdev; int i; AHBPnp *ahb_pnp; APBPnp *apb_pnp; - reset_info = g_malloc0(sizeof(ResetData)); - for (i = 0; i < machine->smp.cpus; i++) { /* Init CPU */ - cpu = SPARC_CPU(object_new(machine->cpu_type)); - qdev_init_gpio_in_named(DEVICE(cpu), leon3_start_cpu, "start_cpu", 1); - qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1); - qdev_realize(DEVICE(cpu), NULL, &error_fatal); - env = &cpu->env; + cpu[i] = SPARC_CPU(object_new(machine->cpu_type)); + qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_start_cpu, "start_cpu", 1); + qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_set_pil_in, "pil", 1); + qdev_realize(DEVICE(cpu[i]), NULL, &error_fatal); + env = &cpu[i]->env; cpu_sparc_set_id(env, i); - /* Reset data */ - reset_info->info[i].id = i; - reset_info->info[i].cpu = cpu; - qemu_register_reset(leon3_cpu_reset, &reset_info->info[i]); + qemu_register_reset(leon3_cpu_reset, cpu[i]); } ahb_pnp = GRLIB_AHB_PNP(qdev_new(TYPE_GRLIB_AHB_PNP)); @@ -312,13 +295,12 @@ static void leon3_generic_hw_init(MachineState *machine) sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal); for (i = 0; i < machine->smp.cpus; i++) { - cpu = reset_info->info[i].cpu; - env = &cpu->env; + env = &cpu[i]->env; qdev_connect_gpio_out_named(irqmpdev, "grlib-start-cpu", i, - qdev_get_gpio_in_named(DEVICE(cpu), + qdev_get_gpio_in_named(DEVICE(cpu[i]), "start_cpu", 0)); qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", i, - qdev_get_gpio_in_named(DEVICE(cpu), + qdev_get_gpio_in_named(DEVICE(cpu[i]), "pil", 0)); env->irq_manager = irqmpdev; env->qemu_irq_ack = leon3_irq_manager; @@ -396,11 +378,6 @@ static void leon3_generic_hw_init(MachineState *machine) * bootloader. */ write_bootloader(memory_region_get_ram_ptr(prom), entry); - reset_info->entry = LEON3_PROM_OFFSET; - for (i = 0; i < machine->smp.cpus; i++) { - reset_info->info[i].cpu->env.pc = LEON3_PROM_OFFSET; - reset_info->info[i].cpu->env.npc = LEON3_PROM_OFFSET + 4; - } } } --- Regards, Phil.
On 21/2/24 18:27, Philippe Mathieu-Daudé wrote: > Clément, ResetData::entry isn't used, so we can simplify removing > the whole ResetData structure, but I'm not sure this is intended: > > -- >8 -- > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c > index 4873b59b6c..1ff6b5d63d 100644 > --- a/hw/sparc/leon3.c > +++ b/hw/sparc/leon3.c > @@ -68,14 +68,6 @@ > #define LEON3_APB_PNP_OFFSET (0x800FF000) > #define LEON3_AHB_PNP_OFFSET (0xFFFFF000) > > -typedef struct ResetData { > - struct CPUResetData { > - int id; > - SPARCCPU *cpu; > - } info[MAX_CPUS]; > - uint32_t entry; /* save kernel entry in case of reset */ > -} ResetData; > - > static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val) > { > stl_p(code++, 0x82100000); /* mov %g0, %g1 */ > @@ -148,17 +140,14 @@ static void write_bootloader(void *ptr, hwaddr > kernel_addr) > > static void leon3_cpu_reset(void *opaque) > { > - struct CPUResetData *info = (struct CPUResetData *) opaque; > - int id = info->id; > - ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info); > - CPUState *cpu = CPU(s->info[id].cpu); > + CPUState *cpu = opaque; > CPUSPARCState *env = cpu_env(cpu); > > cpu_reset(cpu); > > cpu->halted = cpu->cpu_index != 0; > - env->pc = s->entry; > - env->npc = s->entry + 4; > + env->pc = LEON3_PROM_OFFSET; > + env->npc = LEON3_PROM_OFFSET + 4; > } > > static void leon3_cache_control_int(CPUSPARCState *env) > @@ -259,7 +248,7 @@ static void leon3_generic_hw_init(MachineState > *machine) > ram_addr_t ram_size = machine->ram_size; > const char *bios_name = machine->firmware ?: LEON3_PROM_FILENAME; > const char *kernel_filename = machine->kernel_filename; > - SPARCCPU *cpu; > + SPARCCPU *cpu[MAX_CPUS]; > CPUSPARCState *env; > MemoryRegion *address_space_mem = get_system_memory(); > MemoryRegion *prom = g_new(MemoryRegion, 1); > @@ -267,28 +256,22 @@ static void leon3_generic_hw_init(MachineState > *machine) > char *filename; > int bios_size; > int prom_size; > - ResetData *reset_info; > DeviceState *dev, *irqmpdev; > int i; > AHBPnp *ahb_pnp; > APBPnp *apb_pnp; > > - reset_info = g_malloc0(sizeof(ResetData)); > - > for (i = 0; i < machine->smp.cpus; i++) { > /* Init CPU */ > - cpu = SPARC_CPU(object_new(machine->cpu_type)); > - qdev_init_gpio_in_named(DEVICE(cpu), leon3_start_cpu, > "start_cpu", 1); > - qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1); > - qdev_realize(DEVICE(cpu), NULL, &error_fatal); > - env = &cpu->env; > + cpu[i] = SPARC_CPU(object_new(machine->cpu_type)); > + qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_start_cpu, > "start_cpu", 1); > + qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_set_pil_in, > "pil", 1); > + qdev_realize(DEVICE(cpu[i]), NULL, &error_fatal); > + env = &cpu[i]->env; > > cpu_sparc_set_id(env, i); > > - /* Reset data */ > - reset_info->info[i].id = i; > - reset_info->info[i].cpu = cpu; > - qemu_register_reset(leon3_cpu_reset, &reset_info->info[i]); > + qemu_register_reset(leon3_cpu_reset, cpu[i]); qemu_register_reset(leon3_cpu_reset, CPU(cpu[i])); > } > > ahb_pnp = GRLIB_AHB_PNP(qdev_new(TYPE_GRLIB_AHB_PNP)); > @@ -312,13 +295,12 @@ static void leon3_generic_hw_init(MachineState > *machine) > sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal); > > for (i = 0; i < machine->smp.cpus; i++) { > - cpu = reset_info->info[i].cpu; > - env = &cpu->env; > + env = &cpu[i]->env; > qdev_connect_gpio_out_named(irqmpdev, "grlib-start-cpu", i, > - qdev_get_gpio_in_named(DEVICE(cpu), > + qdev_get_gpio_in_named(DEVICE(cpu[i]), > > "start_cpu", 0)); > qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", i, > - qdev_get_gpio_in_named(DEVICE(cpu), > + qdev_get_gpio_in_named(DEVICE(cpu[i]), > "pil", 0)); > env->irq_manager = irqmpdev; > env->qemu_irq_ack = leon3_irq_manager; > @@ -396,11 +378,6 @@ static void leon3_generic_hw_init(MachineState > *machine) > * bootloader. > */ > write_bootloader(memory_region_get_ram_ptr(prom), entry); > - reset_info->entry = LEON3_PROM_OFFSET; > - for (i = 0; i < machine->smp.cpus; i++) { > - reset_info->info[i].cpu->env.pc = LEON3_PROM_OFFSET; > - reset_info->info[i].cpu->env.npc = LEON3_PROM_OFFSET + 4; > - } > } > } > --- > > Regards, > > Phil.
On 21/02/2024 17.59, Thomas Huth wrote: > On 21/02/2024 17.26, Thomas Huth wrote: >> From: Peter Maydell <peter.maydell@linaro.org> >> >> QEMU has historically used variable length arrays only very rarely. >> Variable length arrays are a potential security issue where an >> on-stack dynamic allocation isn't correctly size-checked, especially >> when the size comes from the guest. (An example problem of this kind >> from the past is CVE-2021-3527). Forbidding them entirely is a >> defensive measure against further bugs of this kind. >> >> Enable -Wvla to prevent any new uses from sneaking into the codebase. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> Message-ID: <20240125173211.1786196-3-peter.maydell@linaro.org> >> [thuth: rebased to current master branch] >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> meson.build | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/meson.build b/meson.build >> index c1dc83e4c0..0ef1654e86 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -592,6 +592,7 @@ warn_flags = [ >> '-Wstrict-prototypes', >> '-Wtype-limits', >> '-Wundef', >> + '-Wvla', >> '-Wwrite-strings', >> # Then disable some undesirable warnings > > Sigh, there's a new warning in the latest master branch: > > https://gitlab.com/thuth/qemu/-/jobs/6225992174 > > Caused by commit d65aba828 ("hw/sparc/leon3: implement multiprocessor")... > Clément, Philippe, could this maybe be written in a different way that does > not trigger a -Wvla warning? I think the DO_UPCAST is wrong here - if I got that right, DO_UPCAST is supposed to check that the second parameter is at the same location as the first type later points to. This is not the case here. I think we rather want container_of() here, so this patch is likely a simple fix: diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c --- a/hw/sparc/leon3.c +++ b/hw/sparc/leon3.c @@ -150,7 +150,7 @@ static void leon3_cpu_reset(void *opaque) { struct CPUResetData *info = (struct CPUResetData *) opaque; int id = info->id; - ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info); + ResetData *s = container_of(info, ResetData, info[id]); CPUState *cpu = CPU(s->info[id].cpu); CPUSPARCState *env = cpu_env(cpu); I can send it as a proper patch, too. Thomas
On Wed, Feb 21, 2024 at 6:27 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 21/2/24 17:59, Thomas Huth wrote: > > On 21/02/2024 17.26, Thomas Huth wrote: > >> From: Peter Maydell <peter.maydell@linaro.org> > >> > >> QEMU has historically used variable length arrays only very rarely. > >> Variable length arrays are a potential security issue where an > >> on-stack dynamic allocation isn't correctly size-checked, especially > >> when the size comes from the guest. (An example problem of this kind > >> from the past is CVE-2021-3527). Forbidding them entirely is a > >> defensive measure against further bugs of this kind. > >> > >> Enable -Wvla to prevent any new uses from sneaking into the codebase. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > >> Message-ID: <20240125173211.1786196-3-peter.maydell@linaro.org> > >> [thuth: rebased to current master branch] > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> meson.build | 1 + > >> 1 file changed, 1 insertion(+) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > >> diff --git a/meson.build b/meson.build > >> index c1dc83e4c0..0ef1654e86 100644 > >> --- a/meson.build > >> +++ b/meson.build > >> @@ -592,6 +592,7 @@ warn_flags = [ > >> '-Wstrict-prototypes', > >> '-Wtype-limits', > >> '-Wundef', > >> + '-Wvla', > >> '-Wwrite-strings', > >> # Then disable some undesirable warnings > > > > Sigh, there's a new warning in the latest master branch: > > > > https://gitlab.com/thuth/qemu/-/jobs/6225992174 > > > > Caused by commit d65aba828 ("hw/sparc/leon3: implement multiprocessor")... > > Clément, Philippe, could this maybe be written in a different way that > > does not trigger a -Wvla warning? > > Clément, ResetData::entry isn't used, so we can simplify removing > the whole ResetData structure, but I'm not sure this is intended: AFAICT, it used to take the kernel entry point before the bootloader part was implemented. But I'm wondering if it could not be one of the issues being the avocado failure. I do want to resurrect it but I missed the time at the moment... I'll do some testing with your patch and see how it goes anyway. > -- >8 -- > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c > index 4873b59b6c..1ff6b5d63d 100644 > --- a/hw/sparc/leon3.c > +++ b/hw/sparc/leon3.c > @@ -68,14 +68,6 @@ > #define LEON3_APB_PNP_OFFSET (0x800FF000) > #define LEON3_AHB_PNP_OFFSET (0xFFFFF000) > > -typedef struct ResetData { > - struct CPUResetData { > - int id; > - SPARCCPU *cpu; > - } info[MAX_CPUS]; > - uint32_t entry; /* save kernel entry in case of reset */ > -} ResetData; > - > static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val) > { > stl_p(code++, 0x82100000); /* mov %g0, %g1 */ > @@ -148,17 +140,14 @@ static void write_bootloader(void *ptr, hwaddr > kernel_addr) > > static void leon3_cpu_reset(void *opaque) > { > - struct CPUResetData *info = (struct CPUResetData *) opaque; > - int id = info->id; > - ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info); > - CPUState *cpu = CPU(s->info[id].cpu); > + CPUState *cpu = opaque; > CPUSPARCState *env = cpu_env(cpu); > > cpu_reset(cpu); > > cpu->halted = cpu->cpu_index != 0; > - env->pc = s->entry; > - env->npc = s->entry + 4; > + env->pc = LEON3_PROM_OFFSET; > + env->npc = LEON3_PROM_OFFSET + 4; > } > > static void leon3_cache_control_int(CPUSPARCState *env) > @@ -259,7 +248,7 @@ static void leon3_generic_hw_init(MachineState *machine) > ram_addr_t ram_size = machine->ram_size; > const char *bios_name = machine->firmware ?: LEON3_PROM_FILENAME; > const char *kernel_filename = machine->kernel_filename; > - SPARCCPU *cpu; > + SPARCCPU *cpu[MAX_CPUS]; > CPUSPARCState *env; > MemoryRegion *address_space_mem = get_system_memory(); > MemoryRegion *prom = g_new(MemoryRegion, 1); > @@ -267,28 +256,22 @@ static void leon3_generic_hw_init(MachineState > *machine) > char *filename; > int bios_size; > int prom_size; > - ResetData *reset_info; > DeviceState *dev, *irqmpdev; > int i; > AHBPnp *ahb_pnp; > APBPnp *apb_pnp; > > - reset_info = g_malloc0(sizeof(ResetData)); > - > for (i = 0; i < machine->smp.cpus; i++) { > /* Init CPU */ > - cpu = SPARC_CPU(object_new(machine->cpu_type)); > - qdev_init_gpio_in_named(DEVICE(cpu), leon3_start_cpu, > "start_cpu", 1); > - qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1); > - qdev_realize(DEVICE(cpu), NULL, &error_fatal); > - env = &cpu->env; > + cpu[i] = SPARC_CPU(object_new(machine->cpu_type)); > + qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_start_cpu, > "start_cpu", 1); > + qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_set_pil_in, > "pil", 1); > + qdev_realize(DEVICE(cpu[i]), NULL, &error_fatal); > + env = &cpu[i]->env; > > cpu_sparc_set_id(env, i); > > - /* Reset data */ > - reset_info->info[i].id = i; > - reset_info->info[i].cpu = cpu; > - qemu_register_reset(leon3_cpu_reset, &reset_info->info[i]); > + qemu_register_reset(leon3_cpu_reset, cpu[i]); > } > > ahb_pnp = GRLIB_AHB_PNP(qdev_new(TYPE_GRLIB_AHB_PNP)); > @@ -312,13 +295,12 @@ static void leon3_generic_hw_init(MachineState > *machine) > sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal); > > for (i = 0; i < machine->smp.cpus; i++) { > - cpu = reset_info->info[i].cpu; > - env = &cpu->env; > + env = &cpu[i]->env; > qdev_connect_gpio_out_named(irqmpdev, "grlib-start-cpu", i, > - qdev_get_gpio_in_named(DEVICE(cpu), > + qdev_get_gpio_in_named(DEVICE(cpu[i]), > > "start_cpu", 0)); > qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", i, > - qdev_get_gpio_in_named(DEVICE(cpu), > + qdev_get_gpio_in_named(DEVICE(cpu[i]), > "pil", 0)); > env->irq_manager = irqmpdev; > env->qemu_irq_ack = leon3_irq_manager; > @@ -396,11 +378,6 @@ static void leon3_generic_hw_init(MachineState > *machine) > * bootloader. > */ > write_bootloader(memory_region_get_ram_ptr(prom), entry); > - reset_info->entry = LEON3_PROM_OFFSET; > - for (i = 0; i < machine->smp.cpus; i++) { > - reset_info->info[i].cpu->env.pc = LEON3_PROM_OFFSET; > - reset_info->info[i].cpu->env.npc = LEON3_PROM_OFFSET + 4; > - } > } > } > --- > > Regards, > > Phil.
diff --git a/meson.build b/meson.build index c1dc83e4c0..0ef1654e86 100644 --- a/meson.build +++ b/meson.build @@ -592,6 +592,7 @@ warn_flags = [ '-Wstrict-prototypes', '-Wtype-limits', '-Wundef', + '-Wvla', '-Wwrite-strings', # Then disable some undesirable warnings