| Message ID | 20210728142631.41860-6-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: pv: implement lazy destroy | expand |
On 7/28/21 4:26 PM, Claudio Imbrenda wrote: > 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(). > + 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. > + 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); >
On Thu, 29 Jul 2021 14:17:11 +0200 Janosch Frank <frankja@linux.ibm.com> wrote: > On 7/28/21 4:26 PM, Claudio Imbrenda wrote: > > 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(). > > > + 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 > > + 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); > > >
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. + */ + addr = __gmap_translate(gmap, addr); + 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);
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(-)