On Wed, Feb 21, 2024 at 6:27 PM Philippe Mathieu-Daudé
<phi...@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.mayd...@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.mayd...@linaro.org>
> >> Message-ID: <20240125173211.1786196-3-peter.mayd...@linaro.org>
> >> [thuth: rebased to current master branch]
> >> Signed-off-by: Thomas Huth <th...@redhat.com>
> >> ---
> >> meson.build | 1 +
> >> 1 file changed, 1 insertion(+)
>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <phi...@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.
