[Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Peter Maydell]

This patchset adds some more secure-only devices to the virt board:
 (1) a 16MB secure-only RAM
 (2) the first flash device is secure-only

The second of these is strictly speaking a breaking change, but I don't
expect it in practice to break anybody:
 (a) there's not much use of the secure support in virt yet
 (b) anything booting a rom image from that flash if TZ is enabled
  will be booting it in Secure mode anyway so will be able to access
  the code -- the only thing that would stop working would be if the
  guest flipped to NS and still expected to be able to access the flash

The second flash device remains NS-accessible (with the expectation that
it will be used for NS UEFI environment variable storage).

In particular, the ATF+OPTEE+UEFI+Linux stack still works fine with
these changes.


NOTE: to get the -bios option to correctly load to the secure-only
flash I had to implement a new function in loader.c. load_image_mr()
is just like load_image_targphys() except that it requests loading
to a MemoryRegion rather than a physaddr. I think we can also use this
to clean up the Sparc cg3 and tcx display devices, which currently take
a qdev property which is "the address I'm going to be mapped at"
purely so they can use load_image_targphys() to load their ROMs.

I have to say I found the loader.c code a bit confusing (it has some
support for "load image to MR" already, but it seems to be tangled
up with the fw_cfg and PC option rom support); review of that
patch in particular appreciated.

thanks
-- PMM

Peter Maydell (4):
  hw/arm/virt: Provide a secure-only RAM if booting in Secure mode
  loader: Add load_image_mr() to load ROM image to a MemoryRegion
  hw/arm/virt: Load bios image to MemoryRegion, not physaddr
  hw/arm/virt: Make first flash device Secure-only if booting secure

 hw/arm/virt.c         | 118 ++++++++++++++++++++++++++++++++++++++------------
 hw/core/loader.c      |  35 +++++++++++++--
 include/hw/arm/virt.h |   1 +
 include/hw/loader.h   |  18 +++++++-
 4 files changed, 138 insertions(+), 34 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] hw/arm/virt: Provide a secure-only RAM if booting in Secure mode

[Peter Maydell]

If we're booting in Secure mode, provide a secure-only RAM
(just 16MB) so that secure firmware has somewhere to run
from that won't be accessible to the Non-secure guest.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c         | 26 ++++++++++++++++++++++++++
 include/hw/arm/virt.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 44bbbea..5bdfe0f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -127,6 +127,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
+    [VIRT_SECURE_MEM] =         { 0x0e000000, 0x01000000 },
     [VIRT_PCIE_MMIO] =          { 0x10000000, 0x2eff0000 },
     [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
     [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
@@ -960,6 +961,30 @@ static void create_platform_bus(VirtBoardInfo *vbi, qemu_irq *pic)
                                 sysbus_mmio_get_region(s, 0));
 }
 
+static void create_secure_ram(VirtBoardInfo *vbi, MemoryRegion *secure_sysmem)
+{
+        MemoryRegion *secram = g_new(MemoryRegion, 1);
+        char *nodename;
+        hwaddr base = vbi->memmap[VIRT_SECURE_MEM].base;
+        hwaddr size = vbi->memmap[VIRT_SECURE_MEM].size;
+
+        memory_region_init_ram(secram, NULL, "virt.secure-ram",
+                               size, &error_fatal);
+        vmstate_register_ram_global(secram);
+        memory_region_add_subregion(secure_sysmem, base, secram);
+
+        nodename = g_strdup_printf("/secram@%" PRIx64, base);
+        qemu_fdt_add_subnode(vbi->fdt, nodename);
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "device_type",
+                                "memory");
+        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                     2, base, 2, size);
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "status", "disabled");
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "secure-status", "okay");
+
+        g_free(nodename);
+}
+
 static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
@@ -1152,6 +1177,7 @@ static void machvirt_init(MachineState *machine)
     create_uart(vbi, pic, VIRT_UART, sysmem);
 
     if (vms->secure) {
+        create_secure_ram(vbi, secure_sysmem);
         create_uart(vbi, pic, VIRT_SECURE_UART, secure_sysmem);
     }
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 1ce7847..ecd8589 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -61,6 +61,7 @@ enum {
     VIRT_PCIE_MMIO_HIGH,
     VIRT_GPIO,
     VIRT_SECURE_UART,
+    VIRT_SECURE_MEM,
 };
 
 typedef struct MemMapEntry {
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] loader: Add load_image_mr() to load ROM image to a MemoryRegion

[Peter Maydell]

Add a new function load_image_mr(), which behaves like
load_image_targphys() except that it loads the ROM image to
a specified MemoryRegion rather than to a specified physical
address. This is useful when a ROM blob needs to be loaded
to a particular flash or ROM device but the address of that
device in the machine's address space is not known. (For
instance, ROMs in devices, or ROMs which might exist in
a different address space to the system address space.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/loader.c    | 35 +++++++++++++++++++++++++++++++----
 include/hw/loader.h | 18 ++++++++++++++++--
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 3a57415..260b3d6 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -147,6 +147,28 @@ int load_image_targphys(const char *filename,
     return size;
 }
 
+int load_image_mr(const char *filename, MemoryRegion *mr)
+{
+    int size;
+
+    if (!memory_access_is_direct(mr, false)) {
+        /* Can only load an image into RAM or ROM */
+        return -1;
+    }
+
+    size = get_image_size(filename);
+
+    if (size > memory_region_size(mr)) {
+        return -1;
+    }
+    if (size > 0) {
+        if (rom_add_file_mr(filename, mr, -1) < 0) {
+            return -1;
+        }
+    }
+    return size;
+}
+
 void pstrcpy_targphys(const char *name, hwaddr dest, int buf_size,
                       const char *source)
 {
@@ -751,7 +773,7 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom)
+                 bool option_rom, MemoryRegion *mr)
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;
@@ -818,7 +840,12 @@ int rom_add_file(const char *file, const char *fw_dir,
 
         fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
     } else {
-        snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
+        if (mr) {
+            rom->mr = mr;
+            snprintf(devpath, sizeof(devpath), "/rom@%s", file);
+        } else {
+            snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
+        }
     }
 
     add_boot_device_path(bootindex, NULL, devpath);
@@ -892,12 +919,12 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize,
 
 int rom_add_vga(const char *file)
 {
-    return rom_add_file(file, "vgaroms", 0, -1, true);
+    return rom_add_file(file, "vgaroms", 0, -1, true, NULL);
 }
 
 int rom_add_option(const char *file, int32_t bootindex)
 {
-    return rom_add_file(file, "genroms", 0, bootindex, true);
+    return rom_add_file(file, "genroms", 0, bootindex, true, NULL);
 }
 
 static void rom_reset(void *unused)
diff --git a/include/hw/loader.h b/include/hw/loader.h
index f7b43ab..09c3764 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -16,6 +16,18 @@ int load_image(const char *filename, uint8_t *addr); /* deprecated */
 ssize_t load_image_size(const char *filename, void *addr, size_t size);
 int load_image_targphys(const char *filename, hwaddr,
                         uint64_t max_sz);
+/**
+ * load_image_mr: load an image into a memory region
+ * @filename: Path to the image file
+ * @mr: Memory Region to load into
+ *
+ * Load the specified file into the memory region.
+ * The file loaded is registered as a ROM, so its contents will be
+ * reinstated whenever the system is reset.
+ * If the file is larger than the memory region's size the call will fail.
+ * Returns -1 on failure, or the size of the file.
+ */
+int load_image_mr(const char *filename, MemoryRegion *mr);
 
 /* This is the limit on the maximum uncompressed image size that
  * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
@@ -67,7 +79,7 @@ extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom);
+                 bool option_rom, MemoryRegion *mr);
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
@@ -82,9 +94,11 @@ void *rom_ptr(hwaddr addr);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
 
 #define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i, false)
+    rom_add_file(_f, NULL, _a, _i, false, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
+#define rom_add_file_mr(_f, _mr, _i)            \
+    rom_add_file(_f, NULL, 0, _i, false, mr)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] hw/arm/virt: Load bios image to MemoryRegion, not physaddr

[Peter Maydell]

If we're loading a BIOS image into the first flash device,
load it into the flash's memory region specifically, not
into the physical address where the flash resides. This will
make a difference when the flash might be in the Secure
address space rather than the Nonsecure one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 5bdfe0f..391cf3f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -679,13 +679,14 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
 }
 
 static void create_one_flash(const char *name, hwaddr flashbase,
-                             hwaddr flashsize)
+                             hwaddr flashsize, const char *file)
 {
     /* Create and map a single flash device. We use the same
      * parameters as the flash devices on the Versatile Express board.
      */
     DriveInfo *dinfo = drive_get_next(IF_PFLASH);
     DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     const uint64_t sectorlength = 256 * 1024;
 
     if (dinfo) {
@@ -705,19 +706,9 @@ static void create_one_flash(const char *name, hwaddr flashbase,
     qdev_prop_set_string(dev, "name", name);
     qdev_init_nofail(dev);
 
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, flashbase);
-}
-
-static void create_flash(const VirtBoardInfo *vbi)
-{
-    /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
-     * Any file passed via -bios goes in the first of these.
-     */
-    hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2;
-    hwaddr flashbase = vbi->memmap[VIRT_FLASH].base;
-    char *nodename;
+    sysbus_mmio_map(sbd, 0, flashbase);
 
-    if (bios_name) {
+    if (file) {
         char *fn;
         int image_size;
 
@@ -727,21 +718,31 @@ static void create_flash(const VirtBoardInfo *vbi)
                          "but you cannot use both options at once");
             exit(1);
         }
-        fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+        fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
         if (!fn) {
-            error_report("Could not find ROM image '%s'", bios_name);
+            error_report("Could not find ROM image '%s'", file);
             exit(1);
         }
-        image_size = load_image_targphys(fn, flashbase, flashsize);
+        image_size = load_image_mr(fn, sysbus_mmio_get_region(sbd, 0));
         g_free(fn);
         if (image_size < 0) {
-            error_report("Could not load ROM image '%s'", bios_name);
+            error_report("Could not load ROM image '%s'", file);
             exit(1);
         }
     }
+}
+
+static void create_flash(const VirtBoardInfo *vbi)
+{
+    /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
+     * Any file passed via -bios goes in the first of these.
+     */
+    hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2;
+    hwaddr flashbase = vbi->memmap[VIRT_FLASH].base;
+    char *nodename;
 
-    create_one_flash("virt.flash0", flashbase, flashsize);
-    create_one_flash("virt.flash1", flashbase + flashsize, flashsize);
+    create_one_flash("virt.flash0", flashbase, flashsize, bios_name);
+    create_one_flash("virt.flash1", flashbase + flashsize, flashsize, NULL);
 
     nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
-- 
1.9.1

[Qemu-devel] [PATCH 4/4] hw/arm/virt: Make first flash device Secure-only if booting secure

[Peter Maydell]

If the virt board is started with the 'secure' property set to
request a Secure setup, then make the first flash device be
visible only to the Secure world.

This is a breaking change, but I don't expect it to be noticed
by anybody, because running TZ-aware guests isn't common and
those guests are generally going to be booting from the flash
and implicitly expecting their Non-secure guests to not touch it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 14 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 391cf3f..4499e2c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -679,7 +679,8 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
 }
 
 static void create_one_flash(const char *name, hwaddr flashbase,
-                             hwaddr flashsize, const char *file)
+                             hwaddr flashsize, const char *file,
+                             MemoryRegion *sysmem)
 {
     /* Create and map a single flash device. We use the same
      * parameters as the flash devices on the Versatile Express board.
@@ -706,7 +707,8 @@ static void create_one_flash(const char *name, hwaddr flashbase,
     qdev_prop_set_string(dev, "name", name);
     qdev_init_nofail(dev);
 
-    sysbus_mmio_map(sbd, 0, flashbase);
+    memory_region_add_subregion(sysmem, flashbase,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
 
     if (file) {
         char *fn;
@@ -732,26 +734,59 @@ static void create_one_flash(const char *name, hwaddr flashbase,
     }
 }
 
-static void create_flash(const VirtBoardInfo *vbi)
+static void create_flash(const VirtBoardInfo *vbi,
+                         MemoryRegion *sysmem,
+                         MemoryRegion *secure_sysmem)
 {
     /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
      * Any file passed via -bios goes in the first of these.
+     * sysmem is the system memory space. secure_sysmem is the secure view
+     * of the system, and the first flash device should be made visible only
+     * there. The second flash device is visible to both secure and nonsecure.
+     * If sysmem == secure_sysmem this means there is no separate Secure
+     * address space and both flash devices are generally visible.
      */
     hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2;
     hwaddr flashbase = vbi->memmap[VIRT_FLASH].base;
     char *nodename;
 
-    create_one_flash("virt.flash0", flashbase, flashsize, bios_name);
-    create_one_flash("virt.flash1", flashbase + flashsize, flashsize, NULL);
+    create_one_flash("virt.flash0", flashbase, flashsize,
+                     bios_name, secure_sysmem);
+    create_one_flash("virt.flash1", flashbase + flashsize, flashsize,
+                     NULL, sysmem);
 
-    nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
-    qemu_fdt_add_subnode(vbi->fdt, nodename);
-    qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible", "cfi-flash");
-    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
-                                 2, flashbase, 2, flashsize,
-                                 2, flashbase + flashsize, 2, flashsize);
-    qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
-    g_free(nodename);
+    if (sysmem == secure_sysmem) {
+        /* Report both flash devices as a single node in the DT */
+        nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
+        qemu_fdt_add_subnode(vbi->fdt, nodename);
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible", "cfi-flash");
+        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                     2, flashbase, 2, flashsize,
+                                     2, flashbase + flashsize, 2, flashsize);
+        qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
+        g_free(nodename);
+    } else {
+        /* Report the devices as separate nodes so we can mark one as
+         * only visible to the secure world.
+         */
+        nodename = g_strdup_printf("/secflash@%" PRIx64, flashbase);
+        qemu_fdt_add_subnode(vbi->fdt, nodename);
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible", "cfi-flash");
+        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                     2, flashbase, 2, flashsize);
+        qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "status", "disabled");
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "secure-status", "okay");
+        g_free(nodename);
+
+        nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
+        qemu_fdt_add_subnode(vbi->fdt, nodename);
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible", "cfi-flash");
+        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                     2, flashbase + flashsize, 2, flashsize);
+        qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
+        g_free(nodename);
+    }
 }
 
 static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
@@ -1171,7 +1206,7 @@ static void machvirt_init(MachineState *machine)
                                          machine->ram_size);
     memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram);
 
-    create_flash(vbi);
+    create_flash(vbi, sysmem, secure_sysmem ? secure_sysmem : sysmem);
 
     create_gic(vbi, pic, gic_version, vms->secure);
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Mark Cave-Ayland]

On 12/02/16 14:45, Peter Maydell wrote:

> NOTE: to get the -bios option to correctly load to the secure-only
> flash I had to implement a new function in loader.c. load_image_mr()
> is just like load_image_targphys() except that it requests loading
> to a MemoryRegion rather than a physaddr. I think we can also use this
> to clean up the Sparc cg3 and tcx display devices, which currently take
> a qdev property which is "the address I'm going to be mapped at"
> purely so they can use load_image_targphys() to load their ROMs.

Yes! This was something I remember having to explicitly work around when
adding the FCode ROMs to these devices as I couldn't otherwise see how
to load the binary directly into the appropriate MemoryRegion.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Peter Maydell]

Ping? Review appreciated especially for the loader.c change...

thanks
-- PMM

On 12 February 2016 at 14:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset adds some more secure-only devices to the virt board:
>  (1) a 16MB secure-only RAM
>  (2) the first flash device is secure-only
>
> The second of these is strictly speaking a breaking change, but I don't
> expect it in practice to break anybody:
>  (a) there's not much use of the secure support in virt yet
>  (b) anything booting a rom image from that flash if TZ is enabled
>   will be booting it in Secure mode anyway so will be able to access
>   the code -- the only thing that would stop working would be if the
>   guest flipped to NS and still expected to be able to access the flash
>
> The second flash device remains NS-accessible (with the expectation that
> it will be used for NS UEFI environment variable storage).
>
> In particular, the ATF+OPTEE+UEFI+Linux stack still works fine with
> these changes.
>
>
> NOTE: to get the -bios option to correctly load to the secure-only
> flash I had to implement a new function in loader.c. load_image_mr()
> is just like load_image_targphys() except that it requests loading
> to a MemoryRegion rather than a physaddr. I think we can also use this
> to clean up the Sparc cg3 and tcx display devices, which currently take
> a qdev property which is "the address I'm going to be mapped at"
> purely so they can use load_image_targphys() to load their ROMs.
>
> I have to say I found the loader.c code a bit confusing (it has some
> support for "load image to MR" already, but it seems to be tangled
> up with the fw_cfg and PC option rom support); review of that
> patch in particular appreciated.
>
> thanks
> -- PMM
>
> Peter Maydell (4):
>   hw/arm/virt: Provide a secure-only RAM if booting in Secure mode
>   loader: Add load_image_mr() to load ROM image to a MemoryRegion
>   hw/arm/virt: Load bios image to MemoryRegion, not physaddr
>   hw/arm/virt: Make first flash device Secure-only if booting secure
>
>  hw/arm/virt.c         | 118 ++++++++++++++++++++++++++++++++++++++------------
>  hw/core/loader.c      |  35 +++++++++++++--
>  include/hw/arm/virt.h |   1 +
>  include/hw/loader.h   |  18 +++++++-
>  4 files changed, 138 insertions(+), 34 deletions(-)

Re: [Qemu-devel] [PATCH 2/4] loader: Add load_image_mr() to load ROM image to a MemoryRegion

[Paolo Bonzini]

On 12/02/2016 15:45, Peter Maydell wrote:
> Add a new function load_image_mr(), which behaves like
> load_image_targphys() except that it loads the ROM image to
> a specified MemoryRegion rather than to a specified physical
> address. This is useful when a ROM blob needs to be loaded
> to a particular flash or ROM device but the address of that
> device in the machine's address space is not known. (For
> instance, ROMs in devices, or ROMs which might exist in
> a different address space to the system address space.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

The patch looks good, in particular it should be fine for the non-fw_cfg
uses of rom->mr.

The fw_cfg interface to loader.c indeed should be turned upside-down so
that the knowledge moves outside rom_add_file (to a rom_add_fwcfg
function for example) and rom_add_file doesn't need to call rom_set_mr.
 Your patch is at least a step in the right direction, because it adds
memory region support in the !fw_cfg case.

So,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 2/4] loader: Add load_image_mr() to load ROM image to a MemoryRegion

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 05:46:28PM +0100, Paolo Bonzini wrote:
> 
> 
> On 12/02/2016 15:45, Peter Maydell wrote:
> > Add a new function load_image_mr(), which behaves like
> > load_image_targphys() except that it loads the ROM image to
> > a specified MemoryRegion rather than to a specified physical
> > address. This is useful when a ROM blob needs to be loaded
> > to a particular flash or ROM device but the address of that
> > device in the machine's address space is not known. (For
> > instance, ROMs in devices, or ROMs which might exist in
> > a different address space to the system address space.)
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> The patch looks good, in particular it should be fine for the non-fw_cfg
> uses of rom->mr.
> 
> The fw_cfg interface to loader.c indeed should be turned upside-down so
> that the knowledge moves outside rom_add_file (to a rom_add_fwcfg
> function for example) and rom_add_file doesn't need to call rom_set_mr.
>  Your patch is at least a step in the right direction, because it adds
> memory region support in the !fw_cfg case.
> 
> So,
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks,
> 
> Paolo

I agree here.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Paolo Bonzini]

On 12/02/2016 15:45, Peter Maydell wrote:
> This patchset adds some more secure-only devices to the virt board:
>  (1) a 16MB secure-only RAM
>  (2) the first flash device is secure-only
> 
> The second of these is strictly speaking a breaking change, but I don't
> expect it in practice to break anybody:
>  (a) there's not much use of the secure support in virt yet
>  (b) anything booting a rom image from that flash if TZ is enabled
>   will be booting it in Secure mode anyway so will be able to access
>   the code -- the only thing that would stop working would be if the
>   guest flipped to NS and still expected to be able to access the flash
> 
> The second flash device remains NS-accessible (with the expectation that
> it will be used for NS UEFI environment variable storage).

I think that, if UEFI secure boot is in use, the UEFI environment
variables should also be only accessible from TrustZone, because they
store the key database.  At least that's how it works on x86, where both
pflash devices have the secure=on flag.

Paolo

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Peter Maydell]

On 7 March 2016 at 22:20, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 12/02/2016 15:45, Peter Maydell wrote:
>> This patchset adds some more secure-only devices to the virt board:
>>  (1) a 16MB secure-only RAM
>>  (2) the first flash device is secure-only
>>
>> The second of these is strictly speaking a breaking change, but I don't
>> expect it in practice to break anybody:
>>  (a) there's not much use of the secure support in virt yet
>>  (b) anything booting a rom image from that flash if TZ is enabled
>>   will be booting it in Secure mode anyway so will be able to access
>>   the code -- the only thing that would stop working would be if the
>>   guest flipped to NS and still expected to be able to access the flash
>>
>> The second flash device remains NS-accessible (with the expectation that
>> it will be used for NS UEFI environment variable storage).
>
> I think that, if UEFI secure boot is in use, the UEFI environment
> variables should also be only accessible from TrustZone, because they
> store the key database.  At least that's how it works on x86, where both
> pflash devices have the secure=on flag.

If I understand the setup that is being used correctly, UEFI runs
in Non-secure, so making the second flash device secure would mean
it could not access it.

Ard, do I have that right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Paolo Bonzini]

On 08/03/2016 00:34, Peter Maydell wrote:
>> > I think that, if UEFI secure boot is in use, the UEFI environment
>> > variables should also be only accessible from TrustZone, because they
>> > store the key database.  At least that's how it works on x86, where both
>> > pflash devices have the secure=on flag.
> If I understand the setup that is being used correctly, UEFI runs
> in Non-secure, so making the second flash device secure would mean
> it could not access it.
> 
> Ard, do I have that right?

The part of UEFI that accesses variables can (optionally) be moved in
secure mode.  If you don't do that, secure boot is not secure at all.
Accesses from non-secure mode do the appropriate marshaling/unmarshaling
to call into the secure driver.

Again---that's what it does on x86, but restricting variable access to
the trusted base is an important part of UEFI secure boot.

Paolo

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Ard Biesheuvel]

On 8 March 2016 at 19:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 08/03/2016 00:34, Peter Maydell wrote:
>>> > I think that, if UEFI secure boot is in use, the UEFI environment
>>> > variables should also be only accessible from TrustZone, because they
>>> > store the key database.  At least that's how it works on x86, where both
>>> > pflash devices have the secure=on flag.
>> If I understand the setup that is being used correctly, UEFI runs
>> in Non-secure, so making the second flash device secure would mean
>> it could not access it.
>>
>> Ard, do I have that right?
>
> The part of UEFI that accesses variables can (optionally) be moved in
> secure mode.  If you don't do that, secure boot is not secure at all.
> Accesses from non-secure mode do the appropriate marshaling/unmarshaling
> to call into the secure driver.
>
> Again---that's what it does on x86, but restricting variable access to
> the trusted base is an important part of UEFI secure boot.
>

There is currently no implementation for secure boot that moves the
authentication layers into the secure world, not even on actual
hardware (modulo the closed source solutions that we don't have access
to). So our secure boot implementation is interesting as a reference,
since it has all the moving parts in place, but should not be relied
upon as an actual secure environment.

As far as this QEMU port is concerned, having some flash in secure and
some in non-secure is going to be useful regardless, and 64 MB is
plenty for both the code and the data. So if users of the Trustzone
port (which is disjoint from the KVM port in any case) can tolerate
having the code and the variables in the same pflash file, I could
simply move the code into the second flash, and we could reserve the
first flash for secure (so it sits at physical address 0x0

-- 
Ard.

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Ard Biesheuvel]

On 8 March 2016 at 19:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 March 2016 at 19:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 08/03/2016 00:34, Peter Maydell wrote:
>>>> > I think that, if UEFI secure boot is in use, the UEFI environment
>>>> > variables should also be only accessible from TrustZone, because they
>>>> > store the key database.  At least that's how it works on x86, where both
>>>> > pflash devices have the secure=on flag.
>>> If I understand the setup that is being used correctly, UEFI runs
>>> in Non-secure, so making the second flash device secure would mean
>>> it could not access it.
>>>
>>> Ard, do I have that right?
>>
>> The part of UEFI that accesses variables can (optionally) be moved in
>> secure mode.  If you don't do that, secure boot is not secure at all.
>> Accesses from non-secure mode do the appropriate marshaling/unmarshaling
>> to call into the secure driver.
>>
>> Again---that's what it does on x86, but restricting variable access to
>> the trusted base is an important part of UEFI secure boot.
>>
>
> There is currently no implementation for secure boot that moves the
> authentication layers into the secure world, not even on actual
> hardware (modulo the closed source solutions that we don't have access
> to). So our secure boot implementation is interesting as a reference,
> since it has all the moving parts in place, but should not be relied
> upon as an actual secure environment.
>
> As far as this QEMU port is concerned, having some flash in secure and
> some in non-secure is going to be useful regardless, and 64 MB is
> plenty for both the code and the data. So if users of the Trustzone
> port (which is disjoint from the KVM port in any case) can tolerate
> having the code and the variables in the same pflash file, I could
> simply move the code into the second flash, and we could reserve the
> first flash for secure (so it sits at physical address 0x0
>

Uhm, actually, the code is not even in the flash to begin with. So
having the second bank be non-secure only makes perfect sense imo

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Paolo Bonzini]

On 08/03/2016 13:13, Ard Biesheuvel wrote:
> > As far as this QEMU port is concerned, having some flash in secure and
> > some in non-secure is going to be useful regardless, and 64 MB is
> > plenty for both the code and the data. So if users of the Trustzone
> > port (which is disjoint from the KVM port in any case) can tolerate
> > having the code and the variables in the same pflash file, I could
> > simply move the code into the second flash, and we could reserve the
> > first flash for secure (so it sits at physical address 0x0
>
> Uhm, actually, the code is not even in the flash to begin with. So
> having the second bank be non-secure only makes perfect sense imo

Interesting, where is the code?

Paolo

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Ard Biesheuvel]

On 8 March 2016 at 19:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 08/03/2016 13:13, Ard Biesheuvel wrote:
>> > As far as this QEMU port is concerned, having some flash in secure and
>> > some in non-secure is going to be useful regardless, and 64 MB is
>> > plenty for both the code and the data. So if users of the Trustzone
>> > port (which is disjoint from the KVM port in any case) can tolerate
>> > having the code and the variables in the same pflash file, I could
>> > simply move the code into the second flash, and we could reserve the
>> > first flash for secure (so it sits at physical address 0x0
>>
>> Uhm, actually, the code is not even in the flash to begin with. So
>> having the second bank be non-secure only makes perfect sense imo
>
> Interesting, where is the code?
>

The UEFI code is loaded into DRAM by the secure firmware, and
relocated and executed from there.

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Paolo Bonzini]

On 08/03/2016 13:16, Ard Biesheuvel wrote:
> > > > As far as this QEMU port is concerned, having some flash in secure and
> > > > some in non-secure is going to be useful regardless, and 64 MB is
> > > > plenty for both the code and the data. So if users of the Trustzone
> > > > port (which is disjoint from the KVM port in any case) can tolerate
> > > > having the code and the variables in the same pflash file, I could
> > > > simply move the code into the second flash, and we could reserve the
> > > > first flash for secure (so it sits at physical address 0x0
> > >
> > > Uhm, actually, the code is not even in the flash to begin with. So
> > > having the second bank be non-secure only makes perfect sense imo
> >
> > Interesting, where is the code?
>
> The UEFI code is loaded into DRAM by the secure firmware, and
> relocated and executed from there.

And if (as in the closed source implementations) UEFI had access to
TrustZone, would the privileged parts of the code be moved to secure DRAM?

Still, the UEFI code must still be protected from tampering, so it
should also reside in secure flash.

Paolo

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Ard Biesheuvel]

On 8 March 2016 at 19:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 08/03/2016 13:16, Ard Biesheuvel wrote:
>> > > > As far as this QEMU port is concerned, having some flash in secure and
>> > > > some in non-secure is going to be useful regardless, and 64 MB is
>> > > > plenty for both the code and the data. So if users of the Trustzone
>> > > > port (which is disjoint from the KVM port in any case) can tolerate
>> > > > having the code and the variables in the same pflash file, I could
>> > > > simply move the code into the second flash, and we could reserve the
>> > > > first flash for secure (so it sits at physical address 0x0
>> > >
>> > > Uhm, actually, the code is not even in the flash to begin with. So
>> > > having the second bank be non-secure only makes perfect sense imo
>> >
>> > Interesting, where is the code?
>>
>> The UEFI code is loaded into DRAM by the secure firmware, and
>> relocated and executed from there.
>
> And if (as in the closed source implementations) UEFI had access to
> TrustZone, would the privileged parts of the code be moved to secure DRAM?
>

Yes. There are various ways imaginable to implement this on ARM/arm64,
either based on ARM Trusted Firmware, or on a full PI implementation
that owns all privileged exception levels and the secure side.
However, this is an unsolved problem on ARM, i.e., the [S]MM support
is in the paper phase, sadly.

> Still, the UEFI code must still be protected from tampering, so it
> should also reside in secure flash.
>

Yes. We have a fairly good picture of how we should implement it if
there were such things as an SMM runtime and other things that could
help us implement the plumbing below the UEFI Runtime Services in a
way where all the reasoning about the authenticity of the variable
updates occurs in the secure world. The same applies for having tamper
proof flash to put the code. The problem is that none of that has been
specified yet, so while we have some proof of concept draft stuff,
there is no point yet in taking this much further until the architects
make their minds up.

Note that, for KVM, it is unlikely that we will ever support all of
this inside the guest. It makes *much* more sense to lock down the
emulated flash, and implement the UEFI Runtime Services using a thin
layer in UEFI that hooks up to interfaces exposed to the guest by
QEMU.

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Paolo Bonzini]

On 08/03/2016 13:50, Ard Biesheuvel wrote:
> Note that, for KVM, it is unlikely that we will ever support all of
> this inside the guest. It makes *much* more sense to lock down the
> emulated flash, and implement the UEFI Runtime Services using a thin
> layer in UEFI that hooks up to interfaces exposed to the guest by
> QEMU.

Well, it makes a bit less sense if the SMM code is already there for you
to use. :)  More seriously, implementing secure boot on x86 KVM was
"just" a matter of reading the architecture manual and chipset
datasheets, and implementing what they said.  Likewise, the firmware
work can reuse a large part of the work done for bare-metal hardware.
Laszlo would kill me for saying this, :) but in terms of sheer SLOC his
platform enablement patches were dwarfed by the SMM code that Intel
contributed.  The SMM code in turn is _exactly_ the same on bare-metal
and virt.

Designing good PV interfaces is hard, designing secure PV interfaces is
harder; reading a spec is easy.  To me, the only reason to do it in PV
interfaces is that the hardware doesn't allow virtualization of EL3.

If the hardware makes you jump through extra hoops, sometimes it's
necessary, sometimes it's not.  If it's not, rationalizing it is bad.  I
cannot think of a good reason for hardware not to let you virtualize
hypervisor or secure mode, or to force the hypervisor to use two-level
page translation.

Paolo

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Peter Maydell]

On 8 March 2016 at 20:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
>  I cannot think of a good reason for hardware not to let you virtualize
> hypervisor or secure mode

Regardless of whether you can think of a good reason or not,
it doesn't let you do it...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Peter Maydell]

On 8 March 2016 at 19:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The UEFI code is loaded into DRAM by the secure firmware, and
> relocated and executed from there.

Incidentally, since we're using the semihosting API to do this at
the moment, this makes the whole thing completely dependent on the
nonsecure guest not being badly behaved, because NS EL1 can happily
make its own semihosting calls which do interesting things like
read and write arbitrary host files.

Presumably one could in theory configure ATF to use something
more sensible (like having the various blobs it loads be
stuffed into the flash with it, or loaded off an emulated disk
or something). But the current use case is as a development
tool for the firmware and secure apps and so on, not something
intended to contain malicious code. Making the flash, ram, etc
secure-only is worthwhile because it exposes plausible bugs
in the guest code being developed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Laszlo Ersek]

On 03/08/16 14:06, Paolo Bonzini wrote:
> On 08/03/2016 13:50, Ard Biesheuvel wrote:
>> Note that, for KVM, it is unlikely that we will ever support all of
>> this inside the guest. It makes *much* more sense to lock down the
>> emulated flash, and implement the UEFI Runtime Services using a thin
>> layer in UEFI that hooks up to interfaces exposed to the guest by
>> QEMU.
> 
> Well, it makes a bit less sense if the SMM code is already there for you
> to use. :)  More seriously, implementing secure boot on x86 KVM was
> "just" a matter of reading the architecture manual and chipset
> datasheets, and implementing what they said.  Likewise, the firmware
> work can reuse a large part of the work done for bare-metal hardware.
> Laszlo would kill me for saying this, :) but in terms of sheer SLOC his
> platform enablement patches were dwarfed by the SMM code that Intel
> contributed.  The SMM code in turn is _exactly_ the same on bare-metal
> and virt.

Your statement about the SLOC proportions is correct. And, while I could
try to depict (again) the challenges that regardless surfaced in the
platform enablement, this is not the right forum, so I'll save it. :)

However: despite reusing the core SMM code identically in the guest,
there is at least one stark behavioral difference: in QEMU the SMI is
raised only on the processor that triggers it. This exercises paths in
the core SMM code where processors have to count down timeouts and bring
each other in, and these busy loops are very visible to an interactive
user in certain circumstances.

For example, Windows installers seem to be absolutely crazy about
massaging UEFI variables -- the rotating animation rather crawls than
rotates for a minute. I traced KVM just the other day while the
installer was in this phase, and 2 VCPUs together produced about 30-50
"(entering|leaving) SMM" messages per second.

Laszlo

> Designing good PV interfaces is hard, designing secure PV interfaces is
> harder; reading a spec is easy.  To me, the only reason to do it in PV
> interfaces is that the hardware doesn't allow virtualization of EL3.
> 
> If the hardware makes you jump through extra hoops, sometimes it's
> necessary, sometimes it's not.  If it's not, rationalizing it is bad.  I
> cannot think of a good reason for hardware not to let you virtualize
> hypervisor or secure mode, or to force the hypervisor to use two-level
> page translation.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Paolo Bonzini]

On 09/03/2016 15:06, Laszlo Ersek wrote:
> However: despite reusing the core SMM code identically in the guest,
> there is at least one stark behavioral difference: in QEMU the SMI is
> raised only on the processor that triggers it. This exercises paths in
> the core SMM code where processors have to count down timeouts and bring
> each other in, and these busy loops are very visible to an interactive
> user in certain circumstances.

Right, and that's unfortunately something that's hard to change because
SeaBIOS relies on it. :((

Paolo

Re: [Qemu-devel] [PATCH 0/4] virt: provide secure-only RAM and first flash

[Laszlo Ersek]

On 03/09/16 15:07, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 15:06, Laszlo Ersek wrote:
>> However: despite reusing the core SMM code identically in the guest,
>> there is at least one stark behavioral difference: in QEMU the SMI is
>> raised only on the processor that triggers it. This exercises paths in
>> the core SMM code where processors have to count down timeouts and bring
>> each other in, and these busy loops are very visible to an interactive
>> user in certain circumstances.
> 
> Right, and that's unfortunately something that's hard to change because
> SeaBIOS relies on it. :((

I recall (and am still thankful for) the relevant patch receiving the
due attention and investigation on the list, so I'm not bitter about it
or anything.

Just a bit worried about users asking about the animation :)

Laszlo