[PATCH] core, inmates: Introduce cell config flag to clean memory on destroy

[Gustavo Lima Chaves]

With the new JAILHOUSE_CELL_CLEAR_MEM (struct jailhouse_cell_desc's)
flag, Jailhouse will cleanup all of the cell's *loadable* memory, on its
destruction, before handing that memory region back to the root cell.
This prevents the latter from accessing data that the former wanted to
keep private.

One could argue that cells without passive communication
region (no JAILHOUSE_CELL_PASSIVE_COMMREG flag) could use a first
attempt to kill them to do any desired cleanup. This does not cover the
cases in which the cell developer still wants passive communication
region (they don't want to bother adding code to read/write to the comms
region address to their logic) but no data leaks whatsoever. This also
covers the case in which a cell goes to parked state and never has the
chance to do such cleanup: with the new flag, when destroyed the root
cell will still be clueless of what happened there memory-wise.

Signed-off-by: Gustavo Lima Chaves <gustavo.l...@intel.com>
---
hypervisor/arch/arm/mach-vexpress.c | 2 +-
hypervisor/arch/x86/ioapic.c | 2 +-
hypervisor/control.c | 28 ++++++++++++++++++-
hypervisor/include/jailhouse/cell-config.h | 1 +
hypervisor/include/jailhouse/paging.h | 3 ++-
hypervisor/paging.c | 43 +++++++++++++++++++++++++-----
hypervisor/pci.c | 7 +++--
7 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/hypervisor/arch/arm/mach-vexpress.c b/hypervisor/arch/arm/mach-vexpress.c
index 6a2af38..0aaae71 100644
--- a/hypervisor/arch/arm/mach-vexpress.c
+++ b/hypervisor/arch/arm/mach-vexpress.c
@@ -65,7 +65,7 @@ int mach_init(void)
if (!sysregs_base)
return -ENOMEM;
root_entry = mmio_read32(sysregs_base + VEXPRESS_FLAGSSET);
- paging_unmap_device(SYSREGS_BASE, sysregs_base, PAGE_SIZE);
+ paging_unmap(SYSREGS_BASE, sysregs_base, PAGE_SIZE);

mach_cell_init(&root_cell);

diff --git a/hypervisor/arch/x86/ioapic.c b/hypervisor/arch/x86/ioapic.c
index bb7dae0..ddc8c48 100644
--- a/hypervisor/arch/x86/ioapic.c
+++ b/hypervisor/arch/x86/ioapic.c
@@ -262,7 +262,7 @@ done:
return 0;

error_paging_destroy:
- paging_unmap_device(irqchip->address, phys_ioapic->reg_base, PAGE_SIZE);
+ paging_unmap(irqchip->address, phys_ioapic->reg_base, PAGE_SIZE);
return err;
}

diff --git a/hypervisor/control.c b/hypervisor/control.c
index b1beed9..bbe6c0b 100644
--- a/hypervisor/control.c
+++ b/hypervisor/control.c
@@ -285,6 +285,20 @@ static int remap_to_root_cell(const struct jailhouse_memory *mem,
return err;
}

+static void cell_wipe_out_mem(const struct jailhouse_memory *mem)
+{
+ void *cell_mem = paging_map(mem->phys_start, mem->size);
+
+ if (!cell_mem) {
+ paging_dump_stats("Paging memory depleted!");
+ panic_printk("FATAL: out of memory on page pools\n");
+ panic_stop();
+ }
+
+ memset(cell_mem, 0, mem->size);
+ paging_unmap(mem->phys_start, cell_mem, mem->size);
+}
+
static void cell_destroy_internal(struct per_cpu *cpu_data, struct cell *cell)
{
const struct jailhouse_memory *mem;
@@ -309,8 +323,20 @@ static void cell_destroy_internal(struct per_cpu *cpu_data, struct cell *cell)
arch_unmap_memory_region(cell, mem);

if (!(mem->flags & (JAILHOUSE_MEM_COMM_REGION |
- JAILHOUSE_MEM_ROOTSHARED)))
+ JAILHOUSE_MEM_ROOTSHARED))) {
+ if (mem->flags & JAILHOUSE_MEM_LOADABLE &&
+ cell->config->flags & JAILHOUSE_CELL_CLEAR_MEM)
+ /*
+ * Clear its memory region when
+ * destroyed. Note that if a cell is
+ * parked, the root cell does *not*
+ * gain access to its memory region
+ * right away, it only happens at this
+ * point.
+ */
+ cell_wipe_out_mem(mem);
remap_to_root_cell(mem, WARN_ON_ERROR);
+ }
}

pci_cell_exit(cell);
diff --git a/hypervisor/include/jailhouse/cell-config.h b/hypervisor/include/jailhouse/cell-config.h
index 3a003f0..a9bdfdd 100644
--- a/hypervisor/include/jailhouse/cell-config.h
+++ b/hypervisor/include/jailhouse/cell-config.h
@@ -46,6 +46,7 @@

#define JAILHOUSE_CELL_PASSIVE_COMMREG 0x00000001
#define JAILHOUSE_CELL_DEBUG_CONSOLE 0x00000002
+#define JAILHOUSE_CELL_CLEAR_MEM 0x00000004

#define JAILHOUSE_CELL_DESC_SIGNATURE "JHCELL"

diff --git a/hypervisor/include/jailhouse/paging.h b/hypervisor/include/jailhouse/paging.h
index 551f73a..f95f832 100644
--- a/hypervisor/include/jailhouse/paging.h
+++ b/hypervisor/include/jailhouse/paging.h
@@ -243,8 +243,9 @@ int paging_destroy(const struct paging_structures *pg_structs,
unsigned long virt, unsigned long size,
enum paging_coherent coherent);

+void *paging_map(unsigned long phys, unsigned long size);
void *paging_map_device(unsigned long phys, unsigned long size);
-void paging_unmap_device(unsigned long phys, void *virt, unsigned long size);
+void paging_unmap(unsigned long phys, void *virt, unsigned long size);

void *paging_get_guest_pages(const struct guest_paging_structures *pg_structs,
unsigned long gaddr, unsigned int num,
diff --git a/hypervisor/paging.c b/hypervisor/paging.c
index a431550..04c18be 100644
--- a/hypervisor/paging.c
+++ b/hypervisor/paging.c
@@ -447,11 +447,40 @@ paging_gvirt2gphys(const struct guest_paging_structures *pg_structs,
}

/**
+ * Map physical memory into hypervisor address space
+ * @param phys Physical address of the memory region.
+ * @param size Size of the memory region.
+ *
+ * @return Virtual mapping address of the memory region or NULL, on
+ * errors.
+ *
+ * @see paging_unmap
+ */
+void *paging_map(unsigned long phys, unsigned long size)
+{
+ void *virt;
+
+ virt = page_alloc(&remap_pool, PAGES(size));
+ if (!virt)
+ return NULL;
+
+ if (paging_create(&hv_paging_structs, phys, size, (unsigned long)virt,
+ PAGE_DEFAULT_FLAGS, PAGING_NON_COHERENT) != 0) {
+ page_free(&remap_pool, virt, PAGES(size));
+ return NULL;
+ }
+
+ return virt;
+}
+
+/**
* Map physical device resource into hypervisor address space
* @param phys Physical address of the resource.
* @param size Size of the resource.
*
* @return Virtual mapping address of the resource or NULL on error.
+ *
+ * @see paging_unmap
*/
void *paging_map_device(unsigned long phys, unsigned long size)
{
@@ -472,17 +501,17 @@ void *paging_map_device(unsigned long phys, unsigned long size)
}

/**
- * Unmap physical device resource from hypervisor address space
- * @param phys Physical address of the resource.
- * @param virt Virtual address of the resource.
- * @param size Size of the resource.
+ * Unmap physical memory from hypervisor address space
+ * @param phys Physical address to be unmapped.
+ * @param virt Virtual address to be unmapped.
+ * @param size Size of memory region.
*
* @note Unmap must use the same parameters as provided to / returned by
- * paging_map_device().
+ * paging_map_device()/paging_map().
*/
-void paging_unmap_device(unsigned long phys, void *virt, unsigned long size)
+void paging_unmap(unsigned long phys, void *virt, unsigned long size)
{
- /* Cannot fail if paired with paging_map_device. */
+ /* Cannot fail if paired with paging_map_device/paging_map. */
paging_destroy(&hv_paging_structs, (unsigned long)virt, size,
PAGING_NON_COHERENT);
page_free(&remap_pool, virt, PAGES(size));
diff --git a/hypervisor/pci.c b/hypervisor/pci.c
index 2f95dd7..be37730 100644
--- a/hypervisor/pci.c
+++ b/hypervisor/pci.c
@@ -634,8 +634,7 @@ static int pci_add_physical_device(struct cell *cell, struct pci_device *device)
return 0;

error_unmap_table:
- paging_unmap_device(device->info->msix_address, device->msix_table,
- size);
+ paging_unmap(device->info->msix_address, device->msix_table, size);
error_remove_dev:
arch_pci_remove_physical_device(device);
return err;
@@ -656,8 +655,8 @@ static void pci_remove_physical_device(struct pci_device *device)
if (!device->msix_table)
return;

- paging_unmap_device(device->info->msix_address, device->msix_table,
- device->info->msix_region_size);
+ paging_unmap(device->info->msix_address, device->msix_table,
+ device->info->msix_region_size);

if (device->msix_vectors != device->msix_vector_array)
page_free(&mem_pool, device->msix_vectors,
--
2.9.4

[Henning Schild]

Am Mon, 7 Aug 2017 16:24:47 -0700
schrieb Gustavo Lima Chaves <gustavo.l...@intel.com>:

> With the new JAILHOUSE_CELL_CLEAR_MEM (struct jailhouse_cell_desc's)
> flag, Jailhouse will cleanup all of the cell's *loadable* memory, on
> its destruction, before handing that memory region back to the root
> cell. This prevents the latter from accessing data that the former
> wanted to keep private.
>
> One could argue that cells without passive communication
> region (no JAILHOUSE_CELL_PASSIVE_COMMREG flag) could use a first
> attempt to kill them to do any desired cleanup. This does not cover
> the cases in which the cell developer still wants passive
> communication region (they don't want to bother adding code to
> read/write to the comms region address to their logic) but no data
> leaks whatsoever. This also covers the case in which a cell goes to
> parked state and never has the chance to do such cleanup: with the
> new flag, when destroyed the root cell will still be clueless of what
> happened there memory-wise.

This is adding unneeded complexity to the hypervisor. As you described
any cell can delay shutdown and clean up itself. If a cell crashes
while "holding the lock" you will have to reboot, no memory will leak to
Linux.

If something can be done outside the hypervisor it should. Not sure how
Jan feels about it, but from me it is a No.

How about putting that feature into the inmate lib?

Henning

[Gustavo Lima Chaves]

* Henning Schild <henning...@siemens.com> [2017-08-08 11:33:12 +0200]:

> Am Mon, 7 Aug 2017 16:24:47 -0700
> schrieb Gustavo Lima Chaves <gustavo.l...@intel.com>:
>
> > With the new JAILHOUSE_CELL_CLEAR_MEM (struct jailhouse_cell_desc's)
> > flag, Jailhouse will cleanup all of the cell's *loadable* memory, on
> > its destruction, before handing that memory region back to the root
> > cell. This prevents the latter from accessing data that the former
> > wanted to keep private.
> >
> > One could argue that cells without passive communication
> > region (no JAILHOUSE_CELL_PASSIVE_COMMREG flag) could use a first
> > attempt to kill them to do any desired cleanup. This does not cover
> > the cases in which the cell developer still wants passive
> > communication region (they don't want to bother adding code to
> > read/write to the comms region address to their logic) but no data
> > leaks whatsoever. This also covers the case in which a cell goes to
> > parked state and never has the chance to do such cleanup: with the
> > new flag, when destroyed the root cell will still be clueless of what
> > happened there memory-wise.
>
> This is adding unneeded complexity to the hypervisor. As you described
> any cell can delay shutdown and clean up itself. If a cell crashes
> while "holding the lock" you will have to reboot, no memory will leak to
> Linux.

Wait a minute, I was talking about the case of *not* using locks for
being killed at all (JAILHOUSE_CELL_PASSIVE_COMMREG). I even find this
design of using the lock and making the hypervisor locked nuts, but
that's another story (we should handle it with some timer, later).

> --
> You received this message because you are subscribed to the Google Groups "Jailhouse" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to jailhouse-de...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
Gustavo Lima Chaves
Intel - Open Source Technology Center

[Henning Schild]

Am Tue, 8 Aug 2017 09:12:33 -0700

The lock is for safety scenarios where a cell is so important that it
can block all modification of the overall system. What the apic demo
shows is just a toy. In a real world scenario the cell holding the lock
is likely to always say no and the trigger to try and change that would
come over a second channel i.e. ivshmem-net.
In that case Linux is just a massive bootloader that provides all
non-safety functionality like UI.

Henning

[Gustavo Lima Chaves]

* Henning Schild <henning...@siemens.com> [2017-08-08 19:34:32 +0200]:

I get you, thanks. But cases in which users are not willing to change
their cell payload to read/write the COMMREG (think other RTOSes in
which one just changes the boot code to adapt itself to Jailhouse but
other than that wants to be good to go) are not welcome?

Now that we're talking about COMMREG, I have another doubt that makes
me scratch my head: are all cells able to set their "cell_state"
field, at the COMMREG memory addressing, at their own will?

[Henning Schild]

Am Tue, 8 Aug 2017 10:44:43 -0700

Well that is a security feature, now you have to trust Linux to not
modify the hypervisor or the configs, might as well trust it to not
access that memory. Or implement your change in the driver?

For safety and security we evaluated using Intel TXT a while ago.

https://www.htwk-leipzig.de/fileadmin/foerderverein/downloads/2015/Postervorlage_2015-Block.pdf

The basic idea is to do something like delayed trusted boot where you
do not have to trust Linux at all. The only thing it has to get right
is to not modify the hypervisor and its inputs, if it fails you can see
that.

> Now that we're talking about COMMREG, I have another doubt that makes
> me scratch my head: are all cells able to set their "cell_state"
> field, at the COMMREG memory addressing, at their own will?

I would have to check in the code. If that is possible and is posing
any sort of problem please open a new thread for that topic.

regards,
Henning

[Jan Kiszka]

On 2017-08-07 19:24, Gustavo Lima Chaves wrote:
> With the new JAILHOUSE_CELL_CLEAR_MEM (struct jailhouse_cell_desc's)
> flag, Jailhouse will cleanup all of the cell's *loadable* memory, on its
> destruction, before handing that memory region back to the root cell.
> This prevents the latter from accessing data that the former wanted to
> keep private.
>
> One could argue that cells without passive communication
> region (no JAILHOUSE_CELL_PASSIVE_COMMREG flag) could use a first
> attempt to kill them to do any desired cleanup. This does not cover the
> cases in which the cell developer still wants passive communication
> region (they don't want to bother adding code to read/write to the comms
> region address to their logic) but no data leaks whatsoever. This also
> covers the case in which a cell goes to parked state and never has the
> chance to do such cleanup: with the new flag, when destroyed the root
> cell will still be clueless of what happened there memory-wise.

I would buy the case of leaking data on crash - if you have a concrete
use case (I heard a couple of times about potential security use cases,
but I'm lacking a confirmation of an implementation). Can you elaborate?

However, I do not buy the reason "more convenient for cell developer"
because that is against the Jailhouse principle "keep the hypervisor
simple" (unless there is a strong reason).

Jan

[Gustavo Lima Chaves]

* Jan Kiszka <jan.k...@web.de> [2017-08-08 20:17:50 -0400]:

> On 2017-08-07 19:24, Gustavo Lima Chaves wrote:
> > With the new JAILHOUSE_CELL_CLEAR_MEM (struct jailhouse_cell_desc's)
> > flag, Jailhouse will cleanup all of the cell's *loadable* memory, on its
> > destruction, before handing that memory region back to the root cell.
> > This prevents the latter from accessing data that the former wanted to
> > keep private.
> >
> > One could argue that cells without passive communication
> > region (no JAILHOUSE_CELL_PASSIVE_COMMREG flag) could use a first
> > attempt to kill them to do any desired cleanup. This does not cover the
> > cases in which the cell developer still wants passive communication
> > region (they don't want to bother adding code to read/write to the comms
> > region address to their logic) but no data leaks whatsoever. This also
> > covers the case in which a cell goes to parked state and never has the
> > chance to do such cleanup: with the new flag, when destroyed the root
> > cell will still be clueless of what happened there memory-wise.
>
> I would buy the case of leaking data on crash - if you have a concrete
> use case (I heard a couple of times about potential security use cases,
> but I'm lacking a confirmation of an implementation). Can you elaborate?

Well, at least on ADAS world, there might be ECU software (that in
Jailhouse world would translate to an inmate) that is kind of
self-contained (senses and actuates on its behalf only, with access to
its I/O) and would only interface with the world outside it with some
sort of watchdog mechanism. The vendor of such software would have the
choice to protect its IP better if noone would have access to the
resulting runtime memory, with the help of the hypervisor.

The feature is indeed for "extreme" cases, but for sure they exist.

>
> However, I do not buy the reason "more convenient for cell developer"
> because that is against the Jailhouse principle "keep the hypervisor
> simple" (unless there is a strong reason).
>
> Jan

[Jan Kiszka]

OK, I'm open to consider the feature at the point your can replace that
"there might be" in the first sentence with some "there is". One "there
is" that case, please make sure to reduce code duplication (paging_map
and paging_map_device are widely identical) and split up that
refactoring from the feature introduction.

I would even lean towards making the wipe feature unconditional then,
just to reduce the variation. If that turns out to be too costly for
other cases, a per mem-region control would be better than a per cell
switch.

Jan

[Gustavo Lima Chaves]

* Jan Kiszka <jan.k...@web.de> [2017-08-11 18:22:05 +0000]:

Very fair, thanks. Bookmarking this to revisit soonish :)