diff mbox series

[v4,19/22] target/arm: Create tagged ram when MTE is enabled

Message ID 20190307170440.3113-20-richard.henderson@linaro.org
State New
Headers show
Series [v4,01/22] target/arm: Add MTE_ACTIVE to tb_flags | expand

Commit Message

Richard Henderson March 7, 2019, 5:04 p.m. UTC 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/cpu.h |  4 ++++
 hw/arm/virt.c    | 33 +++++++++++++++++++++++++++++++++
 target/arm/cpu.c | 21 ++++++++++++++++++---
 3 files changed, 55 insertions(+), 3 deletions(-)

-- 
2.17.2

Comments

Peter Maydell July 22, 2019, 4:03 p.m. UTC | #1 
On Thu, 7 Mar 2019 at 17:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---


> @@ -1182,16 +1194,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)

>      init_cpreg_list(cpu);

>

>  #ifndef CONFIG_USER_ONLY

> +    cs->num_ases = 1;

>      if (cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY)) {

>          cs->num_ases = 2;

> -

>          if (!cpu->secure_memory) {

>              cpu->secure_memory = cs->memory;

>          }

>          cpu_address_space_init(cs, ARMASIdx_S, "cpu-secure-memory",

>                                 cpu->secure_memory);

> -    } else {

> -        cs->num_ases = 1;

> +    }

> +    if (cpu->tag_memory != NULL) {

> +        cs->num_ases = 3;

> +        cpu_address_space_init(cs, ARMASIdx_TAG, "cpu-tag-memory",

> +                               cpu->tag_memory);

>      }

>      cpu_address_space_init(cs, ARMASIdx_NS, "cpu-memory", cs->memory);


This code will result in an array overrun in cpu_address_space_init(),
because that function allocates the cpu->cpu_ases array the first
time it is called, using the value of cpu->num_ases at that point.
So if  we call it for "cpu-secure-memory" when cpu->num_ases is 2,
the later call for "cpu-tag-memory" won't reallocate the array to
have size 3, it will just index off the end of it.

You should be able to see this with valgrind for the case of
the 'virt' board with EL3 enabled:

 valgrind ./build/x86/aarch64-softmmu/qemu-system-aarch64 -cpu max -M
virt,secure=on

==16861== Invalid write of size 8
==16861==    at 0x4120FA: cpu_address_space_init (exec.c:906)
==16861==    by 0x636F80: arm_cpu_realizefn (cpu.c:1617)
==16861==    by 0x80B920: device_set_realized (qdev.c:834)
==16861==    by 0xA9A94B: property_set_bool (object.c:2076)
==16861==    by 0xA98BC7: object_property_set (object.c:1268)
==16861==    by 0xA9BC8D: object_property_set_qobject (qom-qobject.c:26)
==16861==    by 0xA98EAC: object_property_set_bool (object.c:1334)
==16861==    by 0x5A0D0F: machvirt_init (virt.c:1677)
==16861==    by 0x81544B: machine_run_board_init (machine.c:1148)
==16861==    by 0x767B7E: main (vl.c:4348)
==16861==  Address 0x2c257720 is 0 bytes after a block of size 384 alloc'd
==16861==    at 0x4C31B25: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16861==    by 0x6100B10: g_malloc0 (in
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.5600.4)
==16861==    by 0x4120BE: cpu_address_space_init (exec.c:902)
==16861==    by 0x636F30: arm_cpu_realizefn (cpu.c:1612)
==16861==    by 0x80B920: device_set_realized (qdev.c:834)
==16861==    by 0xA9A94B: property_set_bool (object.c:2076)
==16861==    by 0xA98BC7: object_property_set (object.c:1268)
==16861==    by 0xA9BC8D: object_property_set_qobject (qom-qobject.c:26)
==16861==    by 0xA98EAC: object_property_set_bool (object.c:1334)
==16861==    by 0x5A0D0F: machvirt_init (virt.c:1677)
==16861==    by 0x81544B: machine_run_board_init (machine.c:1148)
==16861==    by 0x767B7E: main (vl.c:4348)


thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e24d1e082c..6d60d2f37d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -768,6 +768,9 @@  struct ARMCPU {
     /* MemoryRegion to use for secure physical accesses */
     MemoryRegion *secure_memory;
 
+    /* MemoryRegion to use for allocation tag accesses */
+    MemoryRegion *tag_memory;
+
     /* For v8M, pointer to the IDAU interface provided by board/SoC */
     Object *idau;
 
@@ -2904,6 +2907,7 @@  int cpu_mmu_index(CPUARMState *env, bool ifetch);
 typedef enum ARMASIdx {
     ARMASIdx_NS = 0,
     ARMASIdx_S = 1,
+    ARMASIdx_TAG = 2,
 } ARMASIdx;
 
 /* Return the Exception Level targeted by debug exceptions. */
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c7fb5348ae..5be76fc2ee 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1265,6 +1265,21 @@  static void create_secure_ram(VirtMachineState *vms,
     g_free(nodename);
 }
 
+static void create_tag_ram(VirtMachineState *vms, MachineState *machine,
+                           MemoryRegion *tag_sysmem)
+{
+    MemoryRegion *tagram = g_new(MemoryRegion, 1);
+    hwaddr base = vms->memmap[VIRT_MEM].base / 32;
+    hwaddr size = machine->ram_size / 32;
+
+    memory_region_init_ram(tagram, NULL, "mach-virt.tag", size, &error_fatal);
+    memory_region_add_subregion(tag_sysmem, base, tagram);
+
+    /* ??? Do we really need an fdt entry, or is MemTag enabled sufficient.  */
+    /* ??? We appear to need secure tag mem to go with secure mem.  */
+    /* ??? Does that imply we need a fourth address space?  */
+}
+
 static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtMachineState *board = container_of(binfo, VirtMachineState,
@@ -1423,6 +1438,7 @@  static void machvirt_init(MachineState *machine)
     qemu_irq pic[NUM_IRQS];
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *secure_sysmem = NULL;
+    MemoryRegion *tag_sysmem = NULL;
     int n, virt_max_cpus;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
@@ -1584,6 +1600,20 @@  static void machvirt_init(MachineState *machine)
                                      "secure-memory", &error_abort);
         }
 
+        /*
+         * The cpu adds the property iff MemTag is supported.
+         * If it is, we must allocate the ram to back that up.
+         */
+        if (object_property_find(cpuobj, "tag-memory", NULL)) {
+            if (!tag_sysmem) {
+                tag_sysmem = g_new(MemoryRegion, 1);
+                memory_region_init(tag_sysmem, OBJECT(machine),
+                                   "tag-memory", UINT64_MAX / 32);
+            }
+            object_property_set_link(cpuobj, OBJECT(tag_sysmem),
+                                     "tag-memory", &error_abort);
+        }
+
         object_property_set_bool(cpuobj, true, "realized", &error_fatal);
         object_unref(cpuobj);
     }
@@ -1626,6 +1656,9 @@  static void machvirt_init(MachineState *machine)
         create_secure_ram(vms, secure_sysmem);
         create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
     }
+    if (tag_sysmem) {
+        create_tag_ram(vms, machine, tag_sysmem);
+    }
 
     vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 96f0ff0ec7..96506cf56d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -870,6 +870,18 @@  void arm_cpu_post_init(Object *obj)
 
     qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
                              &error_abort);
+
+#ifndef CONFIG_USER_ONLY
+    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) &&
+        cpu_isar_feature(aa64_mte, cpu)) {
+        object_property_add_link(obj, "tag-memory",
+                                 TYPE_MEMORY_REGION,
+                                 (Object **)&cpu->tag_memory,
+                                 qdev_prop_allow_set_link_before_realize,
+                                 OBJ_PROP_LINK_STRONG,
+                                 &error_abort);
+    }
+#endif
 }
 
 static void arm_cpu_finalizefn(Object *obj)
@@ -1182,16 +1194,19 @@  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     init_cpreg_list(cpu);
 
 #ifndef CONFIG_USER_ONLY
+    cs->num_ases = 1;
     if (cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY)) {
         cs->num_ases = 2;
-
         if (!cpu->secure_memory) {
             cpu->secure_memory = cs->memory;
         }
         cpu_address_space_init(cs, ARMASIdx_S, "cpu-secure-memory",
                                cpu->secure_memory);
-    } else {
-        cs->num_ases = 1;
+    }
+    if (cpu->tag_memory != NULL) {
+        cs->num_ases = 3;
+        cpu_address_space_init(cs, ARMASIdx_TAG, "cpu-tag-memory",
+                               cpu->tag_memory);
     }
     cpu_address_space_init(cs, ARMASIdx_NS, "cpu-memory", cs->memory);