Message ID | 20210603183347.1695-5-will@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | kvm/arm64: Initial pKVM user ABI | expand |
On Thu, Jun 03, 2021, Will Deacon wrote: > +Enabling this capability causes all memory slots of the specified VM to be > +unmapped from the host system and put into a state where they are no longer > +configurable. What happens if userspace remaps the hva of the memslot? In other words, how will the implementation ensure the physical memory backing the guest is truly inaccessible? And how will the guest/host share memory? E.g. what if the guest wants to set up a new shared memory region and the host wants that to happen in a new memslot? On a related topic, would the concept of guest-only memory be useful[*]? The idea is to require userspace to map guest-private memory with a dedicated VMA flag. That will allow the host kernel to prevent userspace (or the kernel) from mapping guest-private memory, and will also allow KVM to verify that memory that the guest wants to be private is indeed private. [*] https://lkml.kernel.org/r/20210416154106.23721-14-kirill.shutemov@linux.intel.com > The memory slot corresponding to the ID passed in args[0] is > +populated with the guest firmware image provided by the host firmware. Why take a slot instead of an address range? I assume copying the host firmware into guest memory will utilize existing APIs, i.e. the memslot lookups to resolve the GPA->HVA will Just Work (TM). > +The first vCPU to enter the guest is defined to be the primary vCPU. All other > +vCPUs belonging to the VM are secondary vCPUs. > + > +All vCPUs belonging to a VM with this capability enabled are initialised to a > +pre-determined reset state irrespective of any prior configuration according to > +the KVM_ARM_VCPU_INIT ioctl, with the following exceptions for the primary > +vCPU: > + > + =========== =========== > + Register(s) Reset value > + =========== =========== > + X0-X14: Preserved (see KVM_SET_ONE_REG) What's the intended use case for allowing userspace to set a pile of registers? Giving userspace control of vCPU state is tricky because the state is untrusted. Is the state going to be attested in any way, or is the VMM trusted while the VM is being created and only de-privileged later on? > + X15: Boot protocol version (0) > + X16-X30: Reserved (0) > + PC: IPA base of firmware memory slot > + SP: IPA end of firmware memory slot > + =========== =========== > + ... > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index c5d1f3c87dbd..e1d4a87d18e4 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1349,6 +1349,9 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > bool writable = !(mem->flags & KVM_MEM_READONLY); > int ret = 0; > > + if (kvm_vm_is_protected(kvm)) > + return -EPERM; FWIW, this will miss the benign corner case where KVM_SET_USER_MEMORY_REGION does not actualy change anything about an existing region. A more practical problem is that this check won't happen until KVM has marked the existing region as invalid in the DELETE or MOVE case. So userspace can't completely delete a region, but it can temporarily delete a region by briefly making it in invalid. No idea what the ramifications of that are on arm64. > if (change != KVM_MR_CREATE && change != KVM_MR_MOVE && > change != KVM_MR_FLAGS_ONLY) > return 0; ... > +static int pkvm_vm_ioctl_enable(struct kvm *kvm, u64 slotid) > +{ > + int ret = 0; Deep into the nits: technically unnecessary since ret is guaranteed to be written before being consumed. > + mutex_lock(&kvm->lock); > + if (kvm_vm_is_protected(kvm)) { > + ret = -EPERM; > + goto out_kvm_unlock; > + } > + > + mutex_lock(&kvm->slots_lock); > + ret = pkvm_enable(kvm, slotid); > + if (ret) > + goto out_slots_unlock; > + > + kvm->arch.pkvm.enabled = true; > +out_slots_unlock: > + mutex_unlock(&kvm->slots_lock); > +out_kvm_unlock: > + mutex_unlock(&kvm->lock); > + return ret; > +} ... > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 3fd9a7e9d90c..58ab8508be5e 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1082,6 +1082,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_SGX_ATTRIBUTE 196 > #define KVM_CAP_VM_COPY_ENC_CONTEXT_FROM 197 > #define KVM_CAP_PTP_KVM 198 > +#define KVM_CAP_ARM_PROTECTED_VM 199 Rather than a dedicated (and arm-only) capability for protected VMs, what about adding a capability to retrieve the supported VM types? And obviously make protected VMs a different type that must be specified at VM creation (there's already plumbing for this). Even if there's no current need to know at creation time that a VM will be protected, it's also unlikely to be a burden on userspace and could be nice to have down the road. The late "activation" ioctl() can still be supported, e.g. to start disallowing memslot updates. x86 with TDX would look like: case KVM_CAP_VM_TYPES: r = BIT(KVM_X86_LEGACY_VM); if (kvm_x86_ops.is_vm_type_supported(KVM_X86_TDX_VM)) r |= BIT(KVM_X86_TDX_VM); break; and arm64 would look like? case KVM_CAP_VM_TYPES: r = BIT(KVM_ARM64_LEGACY_VM); if (kvm_get_mode() == KVM_MODE_PROTECTED) r |= BIT(KVM_ARM64_PROTECTED_VM); break; > #ifdef KVM_CAP_IRQ_ROUTING > > -- > 2.32.0.rc0.204.g9fa02ecfa5-goog >
On Thu, Jun 03, 2021 at 07:33:47PM +0100, Will Deacon wrote: > Introduce a new VM capability, KVM_CAP_ARM_PROTECTED_VM, which can be > used to isolate guest memory from the host. For now, the EL2 portion is > missing, so this documents and exposes the user ABI for the host. > > Signed-off-by: Will Deacon <will@kernel.org> > --- > Documentation/virt/kvm/api.rst | 69 ++++++++++++++++++++ > arch/arm64/include/asm/kvm_host.h | 10 +++ > arch/arm64/include/uapi/asm/kvm.h | 9 +++ > arch/arm64/kvm/arm.c | 18 +++--- > arch/arm64/kvm/mmu.c | 3 + > arch/arm64/kvm/pkvm.c | 104 ++++++++++++++++++++++++++++++ > include/uapi/linux/kvm.h | 1 + > 7 files changed, 205 insertions(+), 9 deletions(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 7fcb2fd38f42..dfbaf905c435 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -6362,6 +6362,75 @@ default. > > See Documentation/x86/sgx/2.Kernel-internals.rst for more details. > > +7.26 KVM_CAP_ARM_PROTECTED_VM > +----------------------------- > + > +:Architectures: arm64 > +:Target: VM > +:Parameters: flags is a single KVM_CAP_ARM_PROTECTED_VM_FLAGS_* value > + > +The presence of this capability indicates that KVM supports running in a > +configuration where the host Linux kernel does not have access to guest memory. > +On such a system, a small hypervisor layer at EL2 can configure the stage-2 > +page tables for both the CPU and any DMA-capable devices to protect guest > +memory pages so that they are inaccessible to the host unless access is granted > +explicitly by the guest. > + > +The 'flags' parameter is defined as follows: > + > +7.26.1 KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE > +-------------------------------------------- > + > +:Capability: 'flag' parameter to KVM_CAP_ARM_PROTECTED_VM > +:Architectures: arm64 > +:Target: VM > +:Parameters: args[0] contains memory slot ID to hold guest firmware > +:Returns: 0 on success; negative error code on failure > + > +Enabling this capability causes all memory slots of the specified VM to be > +unmapped from the host system and put into a state where they are no longer > +configurable. The memory slot corresponding to the ID passed in args[0] is > +populated with the guest firmware image provided by the host firmware. As on the prior patch, I don't quite follow the rationale for the guest fw coming from the host fw, and it seems to go against the usual design for VM contents, so I fear it could be a problem in future (even if not in android's specific model for usage). > +The first vCPU to enter the guest is defined to be the primary vCPU. All other > +vCPUs belonging to the VM are secondary vCPUs. > + > +All vCPUs belonging to a VM with this capability enabled are initialised to a > +pre-determined reset state What is that "pre-determined reset state"? e.g. is that just what KVM does today, or is there something more specific (e.g. that might change with the "Boot protocol version" below)? > irrespective of any prior configuration according to > +the KVM_ARM_VCPU_INIT ioctl, with the following exceptions for the primary > +vCPU: > + > + =========== =========== > + Register(s) Reset value > + =========== =========== > + X0-X14: Preserved (see KVM_SET_ONE_REG) > + X15: Boot protocol version (0) What's the "Boot protocol" in this context? Is that just referring to this handover state, or is that something more involved? > + X16-X30: Reserved (0) > + PC: IPA base of firmware memory slot > + SP: IPA end of firmware memory slot > + =========== =========== > + > +Secondary vCPUs belonging to a VM with this capability enabled will return > +-EPERM in response to a KVM_RUN ioctl() if the vCPU was not initialised with > +the KVM_ARM_VCPU_POWER_OFF feature. I assume this means that protected VMs always get a trusted PSCI implementation? It might be worth mentioning so (and worth consdiering if that should always have the SMCCC bits too). > + > +There is no support for AArch32 at any exception level. Is this only going to run on CPUs without AArch32 EL0? ... or does this mean behaviour will be erratic if someone tries to run AArch32 EL0? > + > +It is an error to enable this capability on a VM after issuing a KVM_RUN > +ioctl() on one of its vCPUs. > + > +7.26.2 KVM_CAP_ARM_PROTECTED_VM_FLAGS_INFO > +------------------------------------------ > + > +:Capability: 'flag' parameter to KVM_CAP_ARM_PROTECTED_VM > +:Architectures: arm64 > +:Target: VM > +:Parameters: args[0] contains pointer to 'struct kvm_protected_vm_info' > +:Returns: 0 on success; negative error code on failure > + > +Populates the 'struct kvm_protected_vm_info' pointed to by args[0] with > +information about the protected environment for the VM. > + > 8. Other capabilities. > ====================== > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 7cd7d5c8c4bc..5645af2a1431 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -100,6 +100,11 @@ struct kvm_s2_mmu { > struct kvm_arch_memory_slot { > }; > > +struct kvm_protected_vm { > + bool enabled; > + struct kvm_memory_slot *firmware_slot; > +}; > + > struct kvm_arch { > struct kvm_s2_mmu mmu; > > @@ -132,6 +137,8 @@ struct kvm_arch { > > u8 pfr0_csv2; > u8 pfr0_csv3; > + > + struct kvm_protected_vm pkvm; > }; > > struct kvm_vcpu_fault_info { > @@ -763,6 +770,9 @@ void kvm_arch_free_vm(struct kvm *kvm); > > int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type); > > +int kvm_arm_vm_ioctl_pkvm(struct kvm *kvm, struct kvm_enable_cap *cap); > +#define kvm_vm_is_protected(kvm) (kvm->arch.pkvm.enabled) > + > int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature); > bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu); > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index 24223adae150..cdb3298ba8ae 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -402,6 +402,15 @@ struct kvm_vcpu_events { > #define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS > #define KVM_PSCI_RET_DENIED PSCI_RET_DENIED > > +/* Protected KVM */ > +#define KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE 0 > +#define KVM_CAP_ARM_PROTECTED_VM_FLAGS_INFO 1 > + > +struct kvm_protected_vm_info { > + __u64 firmware_size; > + __u64 __reserved[7]; > +}; Do you have anything in mind for those 7 fields, or was this just a number that sounded big enough? I wonder if it's worth adding an size field, and having a size argument to the ioctl, so that you can discover the full size if we need to grow this, but you can always get a truncated copy if you just need the early fields. Thanks, Mark.
On Fri, Jun 04, 2021 at 03:41:10PM +0100, Mark Rutland wrote: > On Thu, Jun 03, 2021 at 07:33:47PM +0100, Will Deacon wrote: > > +7.26.1 KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE > > +-------------------------------------------- > > + > > +:Capability: 'flag' parameter to KVM_CAP_ARM_PROTECTED_VM > > +:Architectures: arm64 > > +:Target: VM > > +:Parameters: args[0] contains memory slot ID to hold guest firmware > > +:Returns: 0 on success; negative error code on failure > > + > > +Enabling this capability causes all memory slots of the specified VM to be > > +unmapped from the host system and put into a state where they are no longer > > +configurable. The memory slot corresponding to the ID passed in args[0] is > > +populated with the guest firmware image provided by the host firmware. > > As on the prior patch, I don't quite follow the rationale for the guest > fw coming from the host fw, and it seems to go against the usual design > for VM contents, so I fear it could be a problem in future (even if not > in android's specific model for usage). Hopefully my other reply helps here. As I say, it would be trivial to extend the interface to make this optional. > > +The first vCPU to enter the guest is defined to be the primary vCPU. All other > > +vCPUs belonging to the VM are secondary vCPUs. > > + > > +All vCPUs belonging to a VM with this capability enabled are initialised to a > > +pre-determined reset state > > What is that "pre-determined reset state"? e.g. is that just what KVM > does today, or is there something more specific (e.g. that might change > with the "Boot protocol version" below)? Yes, it's the KVM reset state as described by KVM_ARM_VCPU_INIT, as I state later in the sentence: > > irrespective of any prior configuration according to > > +the KVM_ARM_VCPU_INIT ioctl here ^^^ So I should probably reword it. How about: | All vCPUs belonging to a VM with this capability enabled are initialised to | a pre-determined reset state according to the KVM_ARM_VCPU_INIT ioctl, | irrespective of any prior configuration, with the following exceptions | for the primary vCPU: ? > > with the following exceptions for the primary > > +vCPU: > > + > > + =========== =========== > > + Register(s) Reset value > > + =========== =========== > > + X0-X14: Preserved (see KVM_SET_ONE_REG) > > + X15: Boot protocol version (0) > > What's the "Boot protocol" in this context? Is that just referring to > this handover state, or is that something more involved? It's just versioning this state, in case we have to change/extend it in future. If you prefer, I can just say: X15-X30 Reserved (0) instead? > > + X16-X30: Reserved (0) > > + PC: IPA base of firmware memory slot > > + SP: IPA end of firmware memory slot > > + =========== =========== > > + > > +Secondary vCPUs belonging to a VM with this capability enabled will return > > +-EPERM in response to a KVM_RUN ioctl() if the vCPU was not initialised with > > +the KVM_ARM_VCPU_POWER_OFF feature. > > I assume this means that protected VMs always get a trusted PSCI > implementation? It might be worth mentioning so (and worth consdiering > if that should always have the SMCCC bits too). "trusted PSCI implementation" might be stretching it, but the idea is that you can at least ensure that vCPUs won't start executing until the guest is ready for them. I don't think we need any particular SMCCC revision for that. > > + > > +There is no support for AArch32 at any exception level. > > Is this only going to run on CPUs without AArch32 EL0? ... or does this > mean behaviour will be erratic if someone tries to run AArch32 EL0? It means that we don't permit creation of 32-bit vCPUs, or exception return to an AArch32 guest from EL2. Of course, if the guest itself decides to try this and the hardware happens to support it then there's not a lot we can about it. But that's the guest being silly, so we don't need to care. > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > > index 24223adae150..cdb3298ba8ae 100644 > > --- a/arch/arm64/include/uapi/asm/kvm.h > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > @@ -402,6 +402,15 @@ struct kvm_vcpu_events { > > #define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS > > #define KVM_PSCI_RET_DENIED PSCI_RET_DENIED > > > > +/* Protected KVM */ > > +#define KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE 0 > > +#define KVM_CAP_ARM_PROTECTED_VM_FLAGS_INFO 1 > > + > > +struct kvm_protected_vm_info { > > + __u64 firmware_size; > > + __u64 __reserved[7]; > > +}; > > Do you have anything in mind for those 7 fields, or was this just a > number that sounded big enough? I just padded it to a cacheline, as that's plenty enough space to version the thing if we need to. > I wonder if it's worth adding an size field, and having a size argument > to the ioctl, so that you can discover the full size if we need to grow > this, but you can always get a truncated copy if you just need the early > fields. Possibly, but the rest of the KVM UAPI doesn't seem to be designed like that, so I followed what was there already. I defer to the KVM maintainers on this one... Will
Hi Sean, Thanks for having a look. On Thu, Jun 03, 2021 at 08:15:55PM +0000, Sean Christopherson wrote: > On Thu, Jun 03, 2021, Will Deacon wrote: > > +Enabling this capability causes all memory slots of the specified VM to be > > +unmapped from the host system and put into a state where they are no longer > > +configurable. > > What happens if userspace remaps the hva of the memslot? In other words, how > will the implementation ensure the physical memory backing the guest is truly > inaccessible? The protection is done in the host stage-2 page-table, so it doesn't matter how the host configures its stage-1 page-table, the underlying physical page will be inaccessible via any mapping it can create. Does that answer your question? > And how will the guest/host share memory? E.g. what if the guest wants to set > up a new shared memory region and the host wants that to happen in a new memslot? The way we're currently dealing with sharing is that, by default, all guest memory is inaccessible to the host. The guest can then issue hypercalls to open windows back to the host, so these would necessarily happen in pre-existing memslots. > On a related topic, would the concept of guest-only memory be useful[*]? The > idea is to require userspace to map guest-private memory with a dedicated VMA > flag. That will allow the host kernel to prevent userspace (or the kernel) from > mapping guest-private memory, and will also allow KVM to verify that memory that > the guest wants to be private is indeed private. > > [*] https://lkml.kernel.org/r/20210416154106.23721-14-kirill.shutemov@linux.intel.com Interesting. That certainly looks like it could make things more robust, as we're in a pretty nasty position if the host tries to access a guest page. We can either panic the host (which will clearly kill the system) or blow away the guest (which hasn't done anything wrong). Consequently, whatever we can do to avoid the host trying to access guest pages in the first place will make the whole thing more stable. > > The memory slot corresponding to the ID passed in args[0] is > > +populated with the guest firmware image provided by the host firmware. > > Why take a slot instead of an address range? I assume copying the host firmware > into guest memory will utilize existing APIs, i.e. the memslot lookups to resolve > the GPA->HVA will Just Work (TM). Because... initially we tried this funky idea where the firmware would execute in-place from a read-only memslot and then get unmapped when it was done. That didn't work out, so now that we have the copying we absolutely can use an address range. I'll do that for the next version, thanks! > > +The first vCPU to enter the guest is defined to be the primary vCPU. All other > > +vCPUs belonging to the VM are secondary vCPUs. > > + > > +All vCPUs belonging to a VM with this capability enabled are initialised to a > > +pre-determined reset state irrespective of any prior configuration according to > > +the KVM_ARM_VCPU_INIT ioctl, with the following exceptions for the primary > > +vCPU: > > + > > + =========== =========== > > + Register(s) Reset value > > + =========== =========== > > + X0-X14: Preserved (see KVM_SET_ONE_REG) > > What's the intended use case for allowing userspace to set a pile of registers? > Giving userspace control of vCPU state is tricky because the state is untrusted. > Is the state going to be attested in any way, or is the VMM trusted while the > VM is being created and only de-privileged later on? I touched a bit on this in my reply to Mark, but the idea is that we have a firmware image which the host cannot access and which is installed by the pKVM hypervisor when entering a guest for the first time. The state documented here is the state in which the firmware is entered, so having some registers populated by the VMM allows the VMM to communicate with both the guest firmware and whatever the real VM payload is. We don't trust the VMM to ensure the integrity of the VM's memory. For example, let's say the VM payload is an arm64 Linux kernel and we have u-boot as the firmware image. When the primary vCPU enters the guest for the first time, the pKVM hypervisor will copy u-boot into the memslot/address-range specified by KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE and set the registers accordingly. U-boot will then locate the guest payload according to what the VMM has put in the preserved registers. Let's say that x4 points to the start of the guest kernel and x5 to the end; u-boot will verify that the memory range is correctly signed and, if so, will then jump to the kernel entry point. The kernel boot protocol then interprets X0-X3 (See Documentation/arm64/booting.rst), with X0 pointing to the devicetree blob provided by the VMM. In this example, if the VMM doesn't set X4/X5 correctly, then the signature check will fail. The devicetree blob is untrusted and, in subsequent patches, we will harden the guest against that (by offering, for example, hypercalls so that the guest can opt-in to exiting back to the host on a stage-2 translation fault). We'd prefer not to spell out the contract between the VM, firmware and guest payload, since this is really specific to what you're actually running. Instead, just carving out a block of preserved registers means KVM doesn't need to care what they're for. > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > > index c5d1f3c87dbd..e1d4a87d18e4 100644 > > --- a/arch/arm64/kvm/mmu.c > > +++ b/arch/arm64/kvm/mmu.c > > @@ -1349,6 +1349,9 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > > bool writable = !(mem->flags & KVM_MEM_READONLY); > > int ret = 0; > > > > + if (kvm_vm_is_protected(kvm)) > > + return -EPERM; > > FWIW, this will miss the benign corner case where KVM_SET_USER_MEMORY_REGION > does not actualy change anything about an existing region. Yeah, I think that's ok though because nothing happened. > A more practical problem is that this check won't happen until KVM has marked > the existing region as invalid in the DELETE or MOVE case. So userspace can't > completely delete a region, but it can temporarily delete a region by briefly > making it in invalid. No idea what the ramifications of that are on arm64. It's probably worth stating that the current patches I have here are only the host kernel part of the bargain. The real work is going to be happening in the hypervisor, which will be in the charge of the page-tables and blissfully ignorant of memslot flags. In other words, the permission checks here are really for slapping the VMM on the wrist if it does something daft rather then enforcing anything for security -- all that has to happen in the hypervisor. In the case of KVM_MEMSLOT_INVALID being set when a guest exits to the host due to a page-fault, it looks like we'll return to the VMM. I'd just like to get some feedback on the user ABI before we commit to the hypervisor side of things. > > if (change != KVM_MR_CREATE && change != KVM_MR_MOVE && > > change != KVM_MR_FLAGS_ONLY) > > return 0; > > ... > > > +static int pkvm_vm_ioctl_enable(struct kvm *kvm, u64 slotid) > > +{ > > + int ret = 0; > > Deep into the nits: technically unnecessary since ret is guaranteed to be written > before being consumed. Sure thing, I can drop that assignment. > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > > index 3fd9a7e9d90c..58ab8508be5e 100644 > > --- a/include/uapi/linux/kvm.h > > +++ b/include/uapi/linux/kvm.h > > @@ -1082,6 +1082,7 @@ struct kvm_ppc_resize_hpt { > > #define KVM_CAP_SGX_ATTRIBUTE 196 > > #define KVM_CAP_VM_COPY_ENC_CONTEXT_FROM 197 > > #define KVM_CAP_PTP_KVM 198 > > +#define KVM_CAP_ARM_PROTECTED_VM 199 > > Rather than a dedicated (and arm-only) capability for protected VMs, what about > adding a capability to retrieve the supported VM types? And obviously make > protected VMs a different type that must be specified at VM creation (there's > already plumbing for this). Even if there's no current need to know at creation > time that a VM will be protected, it's also unlikely to be a burden on userspace > and could be nice to have down the road. The late "activation" ioctl() can still > be supported, e.g. to start disallowing memslot updates. > > x86 with TDX would look like: > > case KVM_CAP_VM_TYPES: > r = BIT(KVM_X86_LEGACY_VM); > if (kvm_x86_ops.is_vm_type_supported(KVM_X86_TDX_VM)) > r |= BIT(KVM_X86_TDX_VM); > break; > > and arm64 would look like? > > case KVM_CAP_VM_TYPES: > r = BIT(KVM_ARM64_LEGACY_VM); > if (kvm_get_mode() == KVM_MODE_PROTECTED) > r |= BIT(KVM_ARM64_PROTECTED_VM); > break; You're not the first person to mention this, so I'll look into it. We'll still need the cap, not just for deferring activation, but also for querying the firmware parameters. Cheers, Will
Hi Will, On 6/8/21 1:08 PM, Will Deacon wrote: > Hi Sean, > > Thanks for having a look. > > On Thu, Jun 03, 2021 at 08:15:55PM +0000, Sean Christopherson wrote: >> On Thu, Jun 03, 2021, Will Deacon wrote: >>> [..] >> >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >>> index 3fd9a7e9d90c..58ab8508be5e 100644 >>> --- a/include/uapi/linux/kvm.h >>> +++ b/include/uapi/linux/kvm.h >>> @@ -1082,6 +1082,7 @@ struct kvm_ppc_resize_hpt { >>> #define KVM_CAP_SGX_ATTRIBUTE 196 >>> #define KVM_CAP_VM_COPY_ENC_CONTEXT_FROM 197 >>> #define KVM_CAP_PTP_KVM 198 >>> +#define KVM_CAP_ARM_PROTECTED_VM 199 >> Rather than a dedicated (and arm-only) capability for protected VMs, what about >> adding a capability to retrieve the supported VM types? And obviously make >> protected VMs a different type that must be specified at VM creation (there's >> already plumbing for this). Even if there's no current need to know at creation >> time that a VM will be protected, it's also unlikely to be a burden on userspace >> and could be nice to have down the road. The late "activation" ioctl() can still >> be supported, e.g. to start disallowing memslot updates. >> >> x86 with TDX would look like: >> >> case KVM_CAP_VM_TYPES: >> r = BIT(KVM_X86_LEGACY_VM); >> if (kvm_x86_ops.is_vm_type_supported(KVM_X86_TDX_VM)) >> r |= BIT(KVM_X86_TDX_VM); >> break; >> >> and arm64 would look like? >> >> case KVM_CAP_VM_TYPES: >> r = BIT(KVM_ARM64_LEGACY_VM); >> if (kvm_get_mode() == KVM_MODE_PROTECTED) >> r |= BIT(KVM_ARM64_PROTECTED_VM); >> break; > You're not the first person to mention this, so I'll look into it. We'll > still need the cap, not just for deferring activation, but also for querying > the firmware parameters. I had a look at the series that rejects unsupported VCPU features from a protected VM [1], and it got me thinking. With this ABI in mind, it will be permitted for an userspace VMM to do the following: 1. Create a VM -> success. 2. Check the KVM_CAP_ARM_PMU_V3 (cap chosen randomly) on the **VM fd** -> it's available. 3. Set the VCPU feature and create the VCPU -> success. 4. <do other initialization stuff> 5. Turn the VM into a protected VM -> failure, because KVM_ARM_VCPU_PMU_V3 was part of the VCPU features. To me, that looks a bit strange. On the other hand, if KVM knew right from VM creation time that the VM is protected, KVM could have told userspace that that VCPU feature is not supported **for this particular type of VM**. I think that makes for a much cleaner userspace API. Yes, it would still be possible to check the cap on the KVM fd and get success, but we could make the argument that those capabilities represent the capabilities that KVM supports for any type of VM, not this particular type. This can also be used for discovering new features added to pKVM. How do you intend to do that with the current ABI? By adding a version field to struct kvm_protected_vm_info, where each version comes with a fixed set of features that are supported? [1] https://www.spinics.net/lists/kvm/msg245882.html Thanks, Alex
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 7fcb2fd38f42..dfbaf905c435 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -6362,6 +6362,75 @@ default. See Documentation/x86/sgx/2.Kernel-internals.rst for more details. +7.26 KVM_CAP_ARM_PROTECTED_VM +----------------------------- + +:Architectures: arm64 +:Target: VM +:Parameters: flags is a single KVM_CAP_ARM_PROTECTED_VM_FLAGS_* value + +The presence of this capability indicates that KVM supports running in a +configuration where the host Linux kernel does not have access to guest memory. +On such a system, a small hypervisor layer at EL2 can configure the stage-2 +page tables for both the CPU and any DMA-capable devices to protect guest +memory pages so that they are inaccessible to the host unless access is granted +explicitly by the guest. + +The 'flags' parameter is defined as follows: + +7.26.1 KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE +-------------------------------------------- + +:Capability: 'flag' parameter to KVM_CAP_ARM_PROTECTED_VM +:Architectures: arm64 +:Target: VM +:Parameters: args[0] contains memory slot ID to hold guest firmware +:Returns: 0 on success; negative error code on failure + +Enabling this capability causes all memory slots of the specified VM to be +unmapped from the host system and put into a state where they are no longer +configurable. The memory slot corresponding to the ID passed in args[0] is +populated with the guest firmware image provided by the host firmware. + +The first vCPU to enter the guest is defined to be the primary vCPU. All other +vCPUs belonging to the VM are secondary vCPUs. + +All vCPUs belonging to a VM with this capability enabled are initialised to a +pre-determined reset state irrespective of any prior configuration according to +the KVM_ARM_VCPU_INIT ioctl, with the following exceptions for the primary +vCPU: + + =========== =========== + Register(s) Reset value + =========== =========== + X0-X14: Preserved (see KVM_SET_ONE_REG) + X15: Boot protocol version (0) + X16-X30: Reserved (0) + PC: IPA base of firmware memory slot + SP: IPA end of firmware memory slot + =========== =========== + +Secondary vCPUs belonging to a VM with this capability enabled will return +-EPERM in response to a KVM_RUN ioctl() if the vCPU was not initialised with +the KVM_ARM_VCPU_POWER_OFF feature. + +There is no support for AArch32 at any exception level. + +It is an error to enable this capability on a VM after issuing a KVM_RUN +ioctl() on one of its vCPUs. + +7.26.2 KVM_CAP_ARM_PROTECTED_VM_FLAGS_INFO +------------------------------------------ + +:Capability: 'flag' parameter to KVM_CAP_ARM_PROTECTED_VM +:Architectures: arm64 +:Target: VM +:Parameters: args[0] contains pointer to 'struct kvm_protected_vm_info' +:Returns: 0 on success; negative error code on failure + +Populates the 'struct kvm_protected_vm_info' pointed to by args[0] with +information about the protected environment for the VM. + 8. Other capabilities. ====================== diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7cd7d5c8c4bc..5645af2a1431 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -100,6 +100,11 @@ struct kvm_s2_mmu { struct kvm_arch_memory_slot { }; +struct kvm_protected_vm { + bool enabled; + struct kvm_memory_slot *firmware_slot; +}; + struct kvm_arch { struct kvm_s2_mmu mmu; @@ -132,6 +137,8 @@ struct kvm_arch { u8 pfr0_csv2; u8 pfr0_csv3; + + struct kvm_protected_vm pkvm; }; struct kvm_vcpu_fault_info { @@ -763,6 +770,9 @@ void kvm_arch_free_vm(struct kvm *kvm); int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type); +int kvm_arm_vm_ioctl_pkvm(struct kvm *kvm, struct kvm_enable_cap *cap); +#define kvm_vm_is_protected(kvm) (kvm->arch.pkvm.enabled) + int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature); bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu); diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index 24223adae150..cdb3298ba8ae 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -402,6 +402,15 @@ struct kvm_vcpu_events { #define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS #define KVM_PSCI_RET_DENIED PSCI_RET_DENIED +/* Protected KVM */ +#define KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE 0 +#define KVM_CAP_ARM_PROTECTED_VM_FLAGS_INFO 1 + +struct kvm_protected_vm_info { + __u64 firmware_size; + __u64 __reserved[7]; +}; + #endif #endif /* __ARM_KVM_H__ */ diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 8d5e23198dfd..186a0adf6391 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -83,22 +83,19 @@ int kvm_arch_check_processor_compat(void *opaque) int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap) { - int r; - - if (cap->flags) - return -EINVAL; - switch (cap->cap) { case KVM_CAP_ARM_NISV_TO_USER: - r = 0; + if (cap->flags) + return -EINVAL; kvm->arch.return_nisv_io_abort_to_user = true; break; + case KVM_CAP_ARM_PROTECTED_VM: + return kvm_arm_vm_ioctl_pkvm(kvm, cap); default: - r = -EINVAL; - break; + return -EINVAL; } - return r; + return 0; } static int kvm_arm_default_max_vcpus(void) @@ -265,6 +262,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) case KVM_CAP_ARM_PTRAUTH_GENERIC: r = system_has_full_ptr_auth(); break; + case KVM_CAP_ARM_PROTECTED_VM: + r = is_protected_kvm_enabled(); + break; default: r = 0; } diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index c5d1f3c87dbd..e1d4a87d18e4 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -1349,6 +1349,9 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, bool writable = !(mem->flags & KVM_MEM_READONLY); int ret = 0; + if (kvm_vm_is_protected(kvm)) + return -EPERM; + if (change != KVM_MR_CREATE && change != KVM_MR_MOVE && change != KVM_MR_FLAGS_ONLY) return 0; diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c index 7af5d03a3941..cf624350fb27 100644 --- a/arch/arm64/kvm/pkvm.c +++ b/arch/arm64/kvm/pkvm.c @@ -50,3 +50,107 @@ static int __init pkvm_firmware_rmem_init(struct reserved_mem *rmem) } RESERVEDMEM_OF_DECLARE(pkvm_firmware, "linux,pkvm-guest-firmware-memory", pkvm_firmware_rmem_init); + +static int pkvm_init_el2_context(struct kvm *kvm) +{ + kvm_pr_unimpl("Stage-2 protection is not yet implemented\n"); + return -EINVAL; +} + +static int pkvm_init_firmware_slot(struct kvm *kvm, u64 slotid) +{ + struct kvm_memslots *slots; + struct kvm_memory_slot *slot; + + if (slotid >= KVM_MEM_SLOTS_NUM || !pkvm_firmware_mem) + return -EINVAL; + + slots = kvm_memslots(kvm); + if (!slots) + return -ENOENT; + + slot = id_to_memslot(slots, slotid); + if (!slot) + return -ENOENT; + + if (slot->flags) + return -EINVAL; + + if ((slot->npages << PAGE_SHIFT) < pkvm_firmware_mem->size) + return -ENOMEM; + + kvm->arch.pkvm.firmware_slot = slot; + return 0; +} + +static void pkvm_teardown_firmware_slot(struct kvm *kvm) +{ + kvm->arch.pkvm.firmware_slot = NULL; +} + +static int pkvm_enable(struct kvm *kvm, u64 slotid) +{ + int ret; + + ret = pkvm_init_firmware_slot(kvm, slotid); + if (ret) + return ret; + + ret = pkvm_init_el2_context(kvm); + if (ret) + pkvm_teardown_firmware_slot(kvm); + + return ret; +} + +static int pkvm_vm_ioctl_enable(struct kvm *kvm, u64 slotid) +{ + int ret = 0; + + mutex_lock(&kvm->lock); + if (kvm_vm_is_protected(kvm)) { + ret = -EPERM; + goto out_kvm_unlock; + } + + mutex_lock(&kvm->slots_lock); + ret = pkvm_enable(kvm, slotid); + if (ret) + goto out_slots_unlock; + + kvm->arch.pkvm.enabled = true; +out_slots_unlock: + mutex_unlock(&kvm->slots_lock); +out_kvm_unlock: + mutex_unlock(&kvm->lock); + return ret; +} + +static int pkvm_vm_ioctl_info(struct kvm *kvm, + struct kvm_protected_vm_info __user *info) +{ + struct kvm_protected_vm_info kinfo = { + .firmware_size = pkvm_firmware_mem ? + pkvm_firmware_mem->size : + 0, + }; + + return copy_to_user(info, &kinfo, sizeof(kinfo)) ? -EFAULT : 0; +} + +int kvm_arm_vm_ioctl_pkvm(struct kvm *kvm, struct kvm_enable_cap *cap) +{ + if (cap->args[1] || cap->args[2] || cap->args[3]) + return -EINVAL; + + switch (cap->flags) { + case KVM_CAP_ARM_PROTECTED_VM_FLAGS_ENABLE: + return pkvm_vm_ioctl_enable(kvm, cap->args[0]); + case KVM_CAP_ARM_PROTECTED_VM_FLAGS_INFO: + return pkvm_vm_ioctl_info(kvm, (void __user *)cap->args[0]); + default: + return -EINVAL; + } + + return 0; +} diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 3fd9a7e9d90c..58ab8508be5e 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1082,6 +1082,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_SGX_ATTRIBUTE 196 #define KVM_CAP_VM_COPY_ENC_CONTEXT_FROM 197 #define KVM_CAP_PTP_KVM 198 +#define KVM_CAP_ARM_PROTECTED_VM 199 #ifdef KVM_CAP_IRQ_ROUTING
Introduce a new VM capability, KVM_CAP_ARM_PROTECTED_VM, which can be used to isolate guest memory from the host. For now, the EL2 portion is missing, so this documents and exposes the user ABI for the host. Signed-off-by: Will Deacon <will@kernel.org> --- Documentation/virt/kvm/api.rst | 69 ++++++++++++++++++++ arch/arm64/include/asm/kvm_host.h | 10 +++ arch/arm64/include/uapi/asm/kvm.h | 9 +++ arch/arm64/kvm/arm.c | 18 +++--- arch/arm64/kvm/mmu.c | 3 + arch/arm64/kvm/pkvm.c | 104 ++++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 1 + 7 files changed, 205 insertions(+), 9 deletions(-)