Re: [RFC 00/11] KVM, EFI, arm64: EFI Runtime Services Sandboxing

[Christoffer Dall]

On Tue, Sep 26, 2017 at 11:14:52PM +0200, Florent Revest wrote:
> On Thu, 2017-08-31 at 11:26 +0200, Christoffer Dall wrote:
> > I wonder if this should be split into two series; one that sets up
> > anything you may need from KVM, and another one that uses that for
> > UEFI.
> > 
> > There's a lot KVM and UEFI intertwined logic and assumptions in patch
> > 10, which makes this series a bit hard to read.
> 
> The way hypercalls are currently handled in handle_hvc required this
> mixed patch. Would some kind of HVC subscription mechanism be suitable
> to have in KVM? (e.g: a function allowing to register a callback on a
> certain HVC function ID) This would allow the 10/11 patch to keep the
> kvm code intact.

Yes, I would have no objections to that if it were relatively
non-invasive at runtime for normal VMs.

> 
> > I'd like some documentation (in the series and in
> > Documentation/virtual/kvm) of how this works, and which hidden
> > assumptions there are. For example, how do you ensure you never
> > attempt to return to userspace?
> 
> I don't think my code ensured this. I'd need to give it a second look.
> 
> >  How many VCPUs do you support?
> 
> You can create as many VCPUs as you would in a "normal VM". Also, each
> VCPU can be ran in a kthread.
> 
> >  Do you support any form of virtual interrupts? How about timers?
> 
> No support for virtual interrupts or timers indeed. The EFI Runtime
> Services sandboxing wouldn't require that.
> 
> > Can a VM access physical devices?
> 
> The very idea of Runtime Services sandboxing requires Internal VMs to
> have access to some of the physical devices.
> 
> >  How do you debug and trace something like this? Can the VM be
> > monitored from userspace?
> 
> There is nothing ready for that.
> 
> > These feel like fundamental questions to me that needs addressing
> > before I can competently review the code.
> > 
> > I think a slightly more concrete motivation and outlining the example
> > of the broken UEFI on Seattle would help paving the way for these
> > patches.
> 
> As far as I can remember, EFI Runtime Services on this platform have
> already been reported to sometimes disable or enable interrupts. Maybe
> someone at ARM has more details about the problem ?
> 

Thanks for answering these questions.  If you or anyone else picks up
this work, we can gather some of the stuff in the thread for
documentation and todo items.

Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs

[Christoffer Dall]

On Tue, Sep 26, 2017 at 11:14:45PM +0200, Florent Revest wrote:
> On Thu, 2017-08-31 at 11:23 +0200, Christoffer Dall wrote:
> > > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> > > index 2ea21da..1d2d3df 100644
> > > --- a/virt/kvm/arm/mmu.c
> > > +++ b/virt/kvm/arm/mmu.c
> > > @@ -772,6 +772,11 @@ static void stage2_unmap_memslot(struct kvm
> > > *kvm,
> > > phys_addr_t size = PAGE_SIZE * memslot->npages;
> > > hva_t reg_end = hva + size;
> > > 
> > > +   if (unlikely(!kvm->mm)) {
> > I think you should consider using a predicate so that it's clear that
> > this is for in-kernel VMs and not just some random situation where mm
> > can be NULL.
> 
> Internal VMs should be the only usage when kvm->mm would be NULL.
> However if you'd prefer it otherwise, I'll make sure this condition
> will be made clearer.
> 

My point was then when I see (!kvm->mm) it looks like a bug, but if I
saw is_in_kernel_vm(kvm) then it looks like a feature.

> > So it's unclear to me why we don't need any special casing in
> > kvm_handle_guest_abort, related to MMIO exits etc.  You probably
> > assume that we will never do emulation, but that should be described
> > and addressed somewhere before I can critically review this patch.
> 
> This is indeed what I was assuming. This RFC does not allow MMIO with
> internal VMs. I can not think of a usage when this would be useful. I'd
> make sure this would be documented in an eventual later RFC.
> 

OK, sounds good.  It's important for me as a reviewer to be able to tell
the differenc between 'assumed valid guest behavior' and 'limitations of
in-kernel VM support' which are handled in such and such way.


> > > +static int internal_vm_prep_mem(struct kvm *kvm,
> > > +   const struct
> > > kvm_userspace_memory_region *mem)
> > > +{
> > > +   phys_addr_t addr, end;
> > > +   unsigned long pfn;
> > > +   int ret;
> > > +   struct kvm_mmu_memory_cache cache = { 0 };
> > > +
> > > +   end = mem->guest_phys_addr + mem->memory_size;
> > > +   pfn = __phys_to_pfn(mem->guest_phys_addr);
> > > +   addr = mem->guest_phys_addr;
> > My main concern here is that we don't do any checks on this region
> > and we could be mapping device memory here as well.  Are we intending
> > that to be ok, and are we then relying on the guest to use proper
> > memory attributes ?
> 
> Indeed, being able to map device memory is intended. It is needed for
> Runtime Services sandboxing. It also relies on the guest being
> correctly configured.
> 

So the reason why we wanted to enforce device attribute mappings in
stage 2 was to avoid a guest having the potential to do cached writes to
a device, which would hit at a later time while no longer running the
VM, potentially breaking isolation through manipulation of a device.

This seems to break with that level of isolation, and that property of
in-kernel VMs should be clearly pointed out somewhere.

> > > +
> > > +   for (; addr < end; addr += PAGE_SIZE) {
> > > +   pte_t pte = pfn_pte(pfn, PAGE_S2);
> > > +
> > > +   pte = kvm_s2pte_mkwrite(pte);
> > > +
> > > +   ret = mmu_topup_memory_cache(,
> > > +KVM_MMU_CACHE_MIN_PAGE
> > > S,
> > > +KVM_NR_MEM_OBJS);
> > You should be able to allocate all you need up front instead of doing
> > it in sequences.
> 
> Ok.
> 
> > > 
> > > +   if (ret) {
> > > +   mmu_free_memory_cache();
> > > +   return ret;
> > > +   }
> > > +   spin_lock(>mmu_lock);
> > > +   ret = stage2_set_pte(kvm, , addr, , 0);
> > > +   spin_unlock(>mmu_lock);
> > Since you're likely to allocate some large contiguous chunks here,
> > can you have a look at using section mappings?
> 
> Will do.
> 

Thanks!
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Part1 PATCH v6 07/17] x86/efi: Access EFI data as encrypted when SEV is active

[Brijesh Singh]

From: Tom Lendacky 

EFI data is encrypted when the kernel is run under SEV. Update the
page table references to be sure the EFI memory areas are accessed
encrypted.

Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: Borislav Petkov 
Cc: Andy Lutomirski 
Cc: Matt Fleming 
Cc: Ard Biesheuvel 
Cc: linux-efi@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: x...@kernel.org
Signed-off-by: Tom Lendacky 
Signed-off-by: Brijesh Singh 
Reviewed-by: Borislav Petkov 
---
 arch/x86/platform/efi/efi_64.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 12e83888e5b9..5469c9319f43 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -369,7 +370,11 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, 
unsigned num_pages)
 * as trim_bios_range() will reserve the first page and isolate it away
 * from memory allocators anyway.
 */
-   if (kernel_map_pages_in_pgd(pgd, 0x0, 0x0, 1, _PAGE_RW)) {
+   pf = _PAGE_RW;
+   if (sev_active())
+   pf |= _PAGE_ENC;
+
+   if (kernel_map_pages_in_pgd(pgd, 0x0, 0x0, 1, pf)) {
pr_err("Failed to create 1:1 mapping for the first page!\n");
return 1;
}
@@ -412,6 +417,9 @@ static void __init __map_region(efi_memory_desc_t *md, u64 
va)
if (!(md->attribute & EFI_MEMORY_WB))
flags |= _PAGE_PCD;
 
+   if (sev_active())
+   flags |= _PAGE_ENC;
+
pfn = md->phys_addr >> PAGE_SHIFT;
if (kernel_map_pages_in_pgd(pgd, pfn, va, md->num_pages, flags))
pr_warn("Error mapping PA 0x%llx -> VA 0x%llx!\n",
@@ -538,6 +546,9 @@ static int __init efi_update_mem_attr(struct mm_struct *mm, 
efi_memory_desc_t *m
if (!(md->attribute & EFI_MEMORY_RO))
pf |= _PAGE_RW;
 
+   if (sev_active())
+   pf |= _PAGE_ENC;
+
return efi_update_mappings(md, pf);
 }
 
@@ -589,6 +600,9 @@ void __init efi_runtime_update_mappings(void)
(md->type != EFI_RUNTIME_SERVICES_CODE))
pf |= _PAGE_RW;
 
+   if (sev_active())
+   pf |= _PAGE_ENC;
+
efi_update_mappings(md, pf);
}
 }
-- 
2.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/5] efi: call get_event_log before ExitBootServices

[Jarkko Sakkinen]

On Fri, Oct 13, 2017 at 10:47:46PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 12, 2017 at 05:03:38PM +0200, Javier Martinez Canillas wrote:
> > On Thu, Oct 12, 2017 at 1:38 PM, Jarkko Sakkinen
> >  wrote:
> > 
> > [snip]
> > 
> > >
> > > Now all Thiebaud's patches have been applied to the master of
> > >
> > >   git://git.infradead.org/users/jjs/linux-tpmdd.git
> > >
> > > Testing is still pending.
> > >
> > 
> > I provided my reviewed and tested by tags for the patches but I
> > noticed that weren't picked. Probably my fault though since I answered
> > to the cover letter instead of the individual patches.
> > 
> > > /Jarkko
> > 
> > Best regards,
> > Javier
> 
> I will add it. The master branch is bleeding edge where tags might be
> sometimes (*not* usually) missing. The next branch is the one that goes
> to linux-next.
> 
> I'll check all tags from patchwork before moving any of these to next.
> 
> /Jarkko

Updated.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/5] efi: call get_event_log before ExitBootServices

[Jarkko Sakkinen]

On Wed, Oct 11, 2017 at 02:52:54PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 11, 2017 at 12:54:26PM +1100, James Morris wrote:
> > On Tue, 10 Oct 2017, Jarkko Sakkinen wrote:
> > 
> > > The way I've agreed with James Morris to have my tree is to be rooted to
> > > security trees next branch.
> > > 
> > > James, what actions should we take?
> > 
> > This process has changed recently -- I posted to lsm but forgot to post to 
> > linux-integrity.
> > 
> > http://kernsec.org/pipermail/linux-security-module-archive/2017-September/003356.html
> > 
> > Summary: please track the next-general branch in my tree for your 
> > development, it replaces 'next'.
> > 
> > 
> > - James
> > -- 
> > James Morris
> > 
> 
> Ah I'm subscribed to that list but lately been busy getting a huge patch
> set to platform-driver-x86 [1] for review, which has prioritized out
> reading much else than linux-integrity.
> 
> Thank you. I'll retry the patches tomorrow.
> 
> /Jarkko

Cannot observer binary_bios_measuremens file.

What kind of hardware was used to develop/test this?

I tried it with Kabylake and PTT (firmware TPM).

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html