| Message ID | 20220414080311.1084834-3-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: pv: implement lazy destroy for reboot | expand |
On 14/04/2022 10.02, Claudio Imbrenda wrote: > With upcoming patches, protected guests will be able to trigger secure > storage violations in normal operation. > > A secure storage violation is triggered when a protected guest tries to > access secure memory that has been mapped erroneously, or that belongs > to a different protected guest or to the ultravisor. > > With upcoming patches, protected guests will be able to trigger secure > storage violations in normal operation. You've already used this sentence as 1st sentence of the patch description. Looks weird to read it again. Maybe scratch the 1st sentence? > This happens for example if a > protected guest is rebooted with lazy destroy enabled and the new guest > is also protected. > > When the new protected guest touches pages that have not yet been > destroyed, and thus are accounted to the previous protected guest, a > secure storage violation is raised. > > This patch adds handling of secure storage violations for protected > guests. > > This exception is handled by first trying to destroy the page, because > it is expected to belong to a defunct protected guest where a destroy > should be possible. If that fails, a normal export of the page is > attempted. > > Therefore, pages that trigger the exception will be made non-secure > before attempting to use them again for a different secure guest. I'm an complete ignorant here, but isn't this somewhat dangerous? Could it happen that a VM could destroy/export the pages of another secure guest that way? Thomas
On Thu, 5 May 2022 19:10:39 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 14/04/2022 10.02, Claudio Imbrenda wrote: > > With upcoming patches, protected guests will be able to trigger secure > > storage violations in normal operation. > > > > A secure storage violation is triggered when a protected guest tries to > > access secure memory that has been mapped erroneously, or that belongs > > to a different protected guest or to the ultravisor. > > > > With upcoming patches, protected guests will be able to trigger secure > > storage violations in normal operation. > > You've already used this sentence as 1st sentence of the patch description. > Looks weird to read it again. Maybe scratch the 1st sentence? oops! > > > This happens for example if a > > protected guest is rebooted with lazy destroy enabled and the new guest > > is also protected. > > > > When the new protected guest touches pages that have not yet been > > destroyed, and thus are accounted to the previous protected guest, a > > secure storage violation is raised. > > > > This patch adds handling of secure storage violations for protected > > guests. > > > > This exception is handled by first trying to destroy the page, because > > it is expected to belong to a defunct protected guest where a destroy > > should be possible. If that fails, a normal export of the page is > > attempted. > > > > Therefore, pages that trigger the exception will be made non-secure > > before attempting to use them again for a different secure guest. > > I'm an complete ignorant here, but isn't this somewhat dangerous? Could it > happen that a VM could destroy/export the pages of another secure guest that > way? this is a good question, perhaps I should add a comment explaining that the destroy page UVC will only work on protected VMs with no CPUs. Exporting instead is not an issue, if/when the page is needed, it will get imported again. Unless some things went really wrong, but that can only happen in case of a bug in the hypervisor. > > Thomas >
diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h index a2d376b8bce3..b96c1cf750a5 100644 --- a/arch/s390/include/asm/uv.h +++ b/arch/s390/include/asm/uv.h @@ -357,6 +357,7 @@ static inline int is_prot_virt_host(void) } int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb); +int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr); int uv_destroy_owned_page(unsigned long paddr); int uv_convert_from_secure(unsigned long paddr); int uv_convert_owned_from_secure(unsigned long paddr); diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c index a5425075dd25..2754471cc789 100644 --- a/arch/s390/kernel/uv.c +++ b/arch/s390/kernel/uv.c @@ -334,6 +334,61 @@ int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr) } EXPORT_SYMBOL_GPL(gmap_convert_to_secure); +/** + * gmap_destroy_page - Destroy a guest page. + * @gmap the gmap of the guest + * @gaddr the guest address to destroy + * + * An attempt will be made to destroy the given guest page. If the attempt + * fails, an attempt is made to export the page. If both attempts fail, an + * appropriate error is returned. + */ +int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr) +{ + struct vm_area_struct *vma; + unsigned long uaddr; + struct page *page; + int rc; + + rc = -EFAULT; + mmap_read_lock(gmap->mm); + + uaddr = __gmap_translate(gmap, gaddr); + if (IS_ERR_VALUE(uaddr)) + goto out; + vma = vma_lookup(gmap->mm, uaddr); + if (!vma) + goto out; + /* + * Huge pages should not be able to become secure + */ + if (is_vm_hugetlb_page(vma)) + goto out; + + rc = 0; + /* we take an extra reference here */ + page = follow_page(vma, uaddr, FOLL_WRITE | FOLL_GET); + if (IS_ERR_OR_NULL(page)) + goto out; + rc = uv_destroy_owned_page(page_to_phys(page)); + /* + * Fault handlers can race; it is possible that two CPUs will fault + * on the same secure page. One CPU can destroy the page, reboot, + * re-enter secure mode and import it, while the second CPU was + * stuck at the beginning of the handler. At some point the second + * CPU will be able to progress, and it will not be able to destroy + * the page. In that case we do not want to terminate the process, + * we instead try to export the page. + */ + if (rc) + rc = uv_convert_owned_from_secure(page_to_phys(page)); + put_page(page); +out: + mmap_read_unlock(gmap->mm); + return rc; +} +EXPORT_SYMBOL_GPL(gmap_destroy_page); + /* * To be called with the page locked or with an extra reference! This will * prevent gmap_make_secure from touching the page concurrently. Having 2 diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c index e173b6187ad5..af1ac49168fb 100644 --- a/arch/s390/mm/fault.c +++ b/arch/s390/mm/fault.c @@ -837,6 +837,16 @@ NOKPROBE_SYMBOL(do_non_secure_storage_access); void do_secure_storage_violation(struct pt_regs *regs) { + unsigned long gaddr = regs->int_parm_long & __FAIL_ADDR_MASK; + struct gmap *gmap = (struct gmap *)S390_lowcore.gmap; + + /* + * If the VM has been rebooted, its address space might still contain + * secure pages from the previous boot. + * Clear the page so it can be reused. + */ + if (!gmap_destroy_page(gmap, gaddr)) + return; /* * Either KVM messed up the secure guest mapping or the same * page is mapped into multiple secure guests.