[PATCH 0/3] Replace variable length arrays in ppc KVM code

[PATCH 0/3] Replace variable length arrays in ppc KVM code

[Thomas Huth]

It would be good to enable -Wvla as an additional security feature
when compiling QEMU (see also Peter's explanation here:
https://lore.kernel.org/qemu-devel/20240125173211.1786196-1-peter.maydell@linaro.org/ ).
There are currently only two spots with variable lengths arrays left,
so if we fix those, we can finally enable the -Wvla compiler switch.

Peter Maydell (1):
  meson: Enable -Wvla

Thomas Huth (2):
  target/ppc/kvm: Replace variable length array in kvmppc_save_htab()
  target/ppc/kvm: Replace variable length array in kvmppc_read_hptes()

 meson.build      | 1 +
 target/ppc/kvm.c | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.43.2

[PATCH 1/3] target/ppc/kvm: Replace variable length array in kvmppc_save_htab()

[Thomas Huth]

To be able to compile QEMU with -Wvla (to prevent potential security
issues), we need to get rid of the variable length array in the
kvmppc_save_htab() function. Replace it with a heap allocation instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/ppc/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 26fa9d0575..e7e39c3091 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2688,7 +2688,7 @@ int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
 {
     int64_t starttime = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-    uint8_t buf[bufsize];
+    g_autofree uint8_t *buf = g_malloc(bufsize);
     ssize_t rc;
 
     do {
-- 
2.43.2

Re: [PATCH 1/3] target/ppc/kvm: Replace variable length array in kvmppc_save_htab()

[Peter Maydell]

On Wed, 21 Feb 2024 at 16:26, Thomas Huth <thuth@redhat.com> wrote:
>
> To be able to compile QEMU with -Wvla (to prevent potential security
> issues), we need to get rid of the variable length array in the
> kvmppc_save_htab() function. Replace it with a heap allocation instead.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/ppc/kvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 26fa9d0575..e7e39c3091 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2688,7 +2688,7 @@ int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
>  {
>      int64_t starttime = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -    uint8_t buf[bufsize];
> +    g_autofree uint8_t *buf = g_malloc(bufsize);
>      ssize_t rc;
>

This works, so
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but you could also drop the bufsize argument, because there are only
two callers and they both pass MAX_KVM_BUF_SIZE, and then declare the
array as fixed size with "uint8_t buf[MAX_KVM_BUF_SIZE]".

thanks
-- PMM

Re: [PATCH 1/3] target/ppc/kvm: Replace variable length array in kvmppc_save_htab()

[Thomas Huth]

On 21/02/2024 17.29, Peter Maydell wrote:
> On Wed, 21 Feb 2024 at 16:26, Thomas Huth <thuth@redhat.com> wrote:
>>
>> To be able to compile QEMU with -Wvla (to prevent potential security
>> issues), we need to get rid of the variable length array in the
>> kvmppc_save_htab() function. Replace it with a heap allocation instead.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   target/ppc/kvm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 26fa9d0575..e7e39c3091 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2688,7 +2688,7 @@ int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
>>   int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
>>   {
>>       int64_t starttime = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> -    uint8_t buf[bufsize];
>> +    g_autofree uint8_t *buf = g_malloc(bufsize);
>>       ssize_t rc;
>>
> 
> This works, so
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> but you could also drop the bufsize argument, because there are only
> two callers and they both pass MAX_KVM_BUF_SIZE, and then declare the
> array as fixed size with "uint8_t buf[MAX_KVM_BUF_SIZE]".

Yes, that's an alternative ... my thinking was that MAX_KVM_BUF_SIZE = 2048 
is already a rather big buffer which should maybe rather be allocated on the 
heap than the stack? But I don't mind too much, so if ppc folks prefer the 
stack allocation, I can change the patch, too.

  Thomas

[PATCH 2/3] target/ppc/kvm: Replace variable length array in kvmppc_read_hptes()

[Thomas Huth]

HPTES_PER_GROUP is 8 and HASH_PTE_SIZE_64 is 16, so we don't waste
too many bytes by always allocating the maximum amount of bytes on
the stack here to get rid of the variable length array.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/ppc/kvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index e7e39c3091..bcf30a5400 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2770,9 +2770,9 @@ void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
     while (i < n) {
         struct kvm_get_htab_header *hdr;
         int m = n < HPTES_PER_GROUP ? n : HPTES_PER_GROUP;
-        char buf[sizeof(*hdr) + m * HASH_PTE_SIZE_64];
+        char buf[sizeof(*hdr) + HPTES_PER_GROUP * HASH_PTE_SIZE_64];
 
-        rc = read(fd, buf, sizeof(buf));
+        rc = read(fd, buf, sizeof(*hdr) + m * HASH_PTE_SIZE_64);
         if (rc < 0) {
             hw_error("kvmppc_read_hptes: Unable to read HPTEs");
         }
-- 
2.43.2

Re: [PATCH 2/3] target/ppc/kvm: Replace variable length array in kvmppc_read_hptes()

[Peter Maydell]

On Wed, 21 Feb 2024 at 16:26, Thomas Huth <thuth@redhat.com> wrote:
>
> HPTES_PER_GROUP is 8 and HASH_PTE_SIZE_64 is 16, so we don't waste
> too many bytes by always allocating the maximum amount of bytes on
> the stack here to get rid of the variable length array.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/ppc/kvm.c | 4 ++--

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 3/3] meson: Enable -Wvla

[Thomas Huth]

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
-- 
2.43.2

Re: [PATCH 3/3] meson: Enable -Wvla

[Thomas Huth]

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

Re: [PATCH 3/3] meson: Enable -Wvla

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 3/3] meson: Enable -Wvla

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 3/3] meson: Enable -Wvla

[Clément Chigot]

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.

Re: [PATCH 3/3] meson: Enable -Wvla

[Thomas Huth]

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