Am Tue, 8 Aug 2017 10:44:43 -0700
schrieb Gustavo Lima Chaves <gustavo.lima.cha...@intel.com>:

> * Henning Schild <henning.sch...@siemens.com> [2017-08-08 19:34:32
> +0200]:
> 
> > Am Tue, 8 Aug 2017 09:12:33 -0700
> > schrieb Gustavo Lima Chaves <gustavo.lima.cha...@intel.com>:
> >   
> > > * Henning Schild <henning.sch...@siemens.com> [2017-08-08 11:33:12
> > > +0200]:
> > >   
> > > > Am Mon, 7 Aug 2017 16:24:47 -0700
> > > > schrieb Gustavo Lima Chaves <gustavo.lima.cha...@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).  
> > 
> > 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.  
> 
> 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?

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

> > 
> > Henning
> >   
> > > > 
> > > > 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
> > > >     
> > > > > Signed-off-by: Gustavo Lima Chaves
> > > > > <gustavo.lima.cha...@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,    
> > > > 
> > > > -- 
> > > > 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-dev+unsubscr...@googlegroups.com. For more options,
> > > > visit https://groups.google.com/d/optout.    
> > >   
> >   
> 

-- 
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-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to