On Thu, 29 Jul 2021 14:17:11 +0200
Janosch Frank [off-list ref] wrote:

On 7/28/21 4:26 PM, Claudio Imbrenda wrote:

quoted

With upcoming patches, normal guests might touch secure pages.

This patch extends the existing exception handler to convert the
pages to non secure also when the exception is triggered by a
normal guest.

This can happen for example when a secure guest reboots; the first
stage of a secure guest is non secure, and in general a secure guest
can reboot into non-secure mode.

If the secure memory of the previous boot has not been cleared up
completely yet, a non-secure guest might touch secure memory, which
will need to be handled properly.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/mm/fault.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index eb68b4f36927..b89d625ea2ec 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c

@@ -767,6 +767,7 @@ void do_secure_storage_access(struct pt_regs

*regs) struct vm_area_struct *vma;
 	struct mm_struct *mm;
 	struct page *page;
+	struct gmap *gmap;
 	int rc;
 
 	/*

@@ -796,6 +797,16 @@ void do_secure_storage_access(struct pt_regs

*regs) }
 
 	switch (get_fault_type(regs)) {
+	case GMAP_FAULT:
+		gmap = (struct gmap *)S390_lowcore.gmap;
+		/*
+		 * Very unlikely, but if it happens, simply try
again.
+		 * The next attempt will trigger a different
exception.
+		 */  

If we keep this the way it currently is then the comment needs to go
to the EFAULT check since it makes no sense above the
gmap_translate().

quoted

+		addr = __gmap_translate(gmap, addr);  

So we had a valid gmap PTE to end up here where the guest touched a
secure page and triggered the exception. But we suddenly can't
translate the gaddr to a vmaddr because the gmap tracking doesn't
have an entry for the address.

My first instinct is to SIGSEGV the process since I can't come up
with a way out of this situation except for the process to map this
back in. The only reason I can think of that it was removed from the
mapping is malicious intent or a bug.

I think this is needs a VM_FAULT_BADMAP and a do_fault_error() call.

fair enough, the next version will have that

quoted

+		if (addr == -EFAULT)
+			break;
+		fallthrough;
 	case USER_FAULT:
 		mm = current->mm;
 		mmap_read_lock(mm);

@@ -824,7 +835,6 @@ void do_secure_storage_access(struct pt_regs

*regs) if (rc)
 			BUG();
 		break;
-	case GMAP_FAULT:
 	default:
 		do_fault_error(regs, VM_READ | VM_WRITE,
VM_FAULT_BADMAP); WARN_ON_ONCE(1);