* [PATCH 0/2] s390x: pv: Fixes and improvements @ 2020-09-03 13:14 Janosch Frank 2020-09-03 13:14 ` [PATCH 1/2] s390x: uv: Add destroy page call Janosch Frank 2020-09-03 13:14 ` [PATCH 2/2] s390x: Add 3f program exception handler Janosch Frank 0 siblings, 2 replies; 11+ messages in thread From: Janosch Frank @ 2020-09-03 13:14 UTC (permalink / raw) To: linux-s390; +Cc: borntraeger, gor, imbrenda, kvm, david Using the destroy call instead of the export on a VM shutdown, we can clear out a protected guest much faster. The 3f exception can in fact be triggered by userspace and therefore should not panic the whole system, but send a SIGSEGV to the culprit process. Janosch Frank (2): s390x: uv: Add destroy page call s390x: Add 3f program exception handler arch/s390/include/asm/uv.h | 7 +++++++ arch/s390/kernel/pgm_check.S | 2 +- arch/s390/kernel/uv.c | 21 +++++++++++++++++++++ arch/s390/mm/fault.c | 23 +++++++++++++++++++++++ arch/s390/mm/gmap.c | 2 +- 5 files changed, 53 insertions(+), 2 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] s390x: uv: Add destroy page call 2020-09-03 13:14 [PATCH 0/2] s390x: pv: Fixes and improvements Janosch Frank @ 2020-09-03 13:14 ` Janosch Frank 2020-09-04 10:39 ` Heiko Carstens 2020-09-03 13:14 ` [PATCH 2/2] s390x: Add 3f program exception handler Janosch Frank 1 sibling, 1 reply; 11+ messages in thread From: Janosch Frank @ 2020-09-03 13:14 UTC (permalink / raw) To: linux-s390; +Cc: borntraeger, gor, imbrenda, kvm, david We don't need to export pages if we destroy the VM configuration afterwards anyway. Instead we can destroy the page which will zero it and then make it accessible to the host. Destroying is about twice as fast as the export. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- arch/s390/include/asm/uv.h | 7 +++++++ arch/s390/kernel/uv.c | 21 +++++++++++++++++++++ arch/s390/mm/gmap.c | 2 +- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h index cff4b4c99b75..0325fc0469b7 100644 --- a/arch/s390/include/asm/uv.h +++ b/arch/s390/include/asm/uv.h @@ -33,6 +33,7 @@ #define UVC_CMD_DESTROY_SEC_CPU 0x0121 #define UVC_CMD_CONV_TO_SEC_STOR 0x0200 #define UVC_CMD_CONV_FROM_SEC_STOR 0x0201 +#define UVC_CMD_DESTR_SEC_STOR 0x0202 #define UVC_CMD_SET_SEC_CONF_PARAMS 0x0300 #define UVC_CMD_UNPACK_IMG 0x0301 #define UVC_CMD_VERIFY_IMG 0x0302 @@ -344,6 +345,7 @@ static inline int is_prot_virt_host(void) } int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb); +int uv_destroy_page(unsigned long paddr); int uv_convert_from_secure(unsigned long paddr); int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr); @@ -354,6 +356,11 @@ void adjust_to_uv_max(unsigned long *vmax); static inline void setup_uv(void) {} static inline void adjust_to_uv_max(unsigned long *vmax) {} +static inline int uv_destroy_page(unsigned long paddr) +{ + return 0; +} + static inline int uv_convert_from_secure(unsigned long paddr) { return 0; diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c index c296e5c8dbf9..e07a5859f1ea 100644 --- a/arch/s390/kernel/uv.c +++ b/arch/s390/kernel/uv.c @@ -118,6 +118,27 @@ static int uv_pin_shared(unsigned long paddr) return 0; } +/* + * Requests the Ultravisor to destroy a guest page and make it + * accessible to the host. The destroy clears the page instead of + * exporting. + * + * @paddr: Absolute host address of page to be destroyed + */ +int uv_destroy_page(unsigned long paddr) +{ + struct uv_cb_cfs uvcb = { + .header.cmd = UVC_CMD_DESTR_SEC_STOR, + .header.len = sizeof(uvcb), + .paddr = paddr + }; + + if (uv_call(0, (u64)&uvcb)) + return -EINVAL; + return 0; +} + + /* * Requests the Ultravisor to encrypt a guest page and make it * accessible to the host for paging (export). diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index 373542ca1113..cfb0017f33a7 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, pte_t pte = READ_ONCE(*ptep); if (pte_present(pte)) - WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK)); + WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] s390x: uv: Add destroy page call 2020-09-03 13:14 ` [PATCH 1/2] s390x: uv: Add destroy page call Janosch Frank @ 2020-09-04 10:39 ` Heiko Carstens 2020-09-04 11:38 ` Janosch Frank 0 siblings, 1 reply; 11+ messages in thread From: Heiko Carstens @ 2020-09-04 10:39 UTC (permalink / raw) To: Janosch Frank; +Cc: linux-s390, borntraeger, gor, imbrenda, kvm, david On Thu, Sep 03, 2020 at 09:14:34AM -0400, Janosch Frank wrote: > +int uv_destroy_page(unsigned long paddr) > +{ > + struct uv_cb_cfs uvcb = { > + .header.cmd = UVC_CMD_DESTR_SEC_STOR, > + .header.len = sizeof(uvcb), > + .paddr = paddr > + }; > + > + if (uv_call(0, (u64)&uvcb)) > + return -EINVAL; > + return 0; > +} > + > + > /* > * Requests the Ultravisor to encrypt a guest page and make it > * accessible to the host for paging (export). > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index 373542ca1113..cfb0017f33a7 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, > pte_t pte = READ_ONCE(*ptep); > > if (pte_present(pte)) > - WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK)); > + WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that function return void? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] s390x: uv: Add destroy page call 2020-09-04 10:39 ` Heiko Carstens @ 2020-09-04 11:38 ` Janosch Frank 2020-09-04 12:10 ` Heiko Carstens 0 siblings, 1 reply; 11+ messages in thread From: Janosch Frank @ 2020-09-04 11:38 UTC (permalink / raw) To: Heiko Carstens; +Cc: linux-s390, borntraeger, gor, imbrenda, kvm, david [-- Attachment #1.1: Type: text/plain, Size: 1315 bytes --] On 9/4/20 12:39 PM, Heiko Carstens wrote: > On Thu, Sep 03, 2020 at 09:14:34AM -0400, Janosch Frank wrote: >> +int uv_destroy_page(unsigned long paddr) >> +{ >> + struct uv_cb_cfs uvcb = { >> + .header.cmd = UVC_CMD_DESTR_SEC_STOR, >> + .header.len = sizeof(uvcb), >> + .paddr = paddr >> + }; >> + >> + if (uv_call(0, (u64)&uvcb)) >> + return -EINVAL; >> + return 0; >> +} >> + >> + I need to remove one of those \n >> /* >> * Requests the Ultravisor to encrypt a guest page and make it >> * accessible to the host for paging (export). >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >> index 373542ca1113..cfb0017f33a7 100644 >> --- a/arch/s390/mm/gmap.c >> +++ b/arch/s390/mm/gmap.c >> @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, >> pte_t pte = READ_ONCE(*ptep); >> >> if (pte_present(pte)) >> - WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK)); >> + WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); > > Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that > function return void? > If you prefer that, I'll change the patch. I think we'd need a proper print of the return codes of the UVC anyway, the warn isn't very helpful if you want to debug after the fact. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] s390x: uv: Add destroy page call 2020-09-04 11:38 ` Janosch Frank @ 2020-09-04 12:10 ` Heiko Carstens 2020-09-07 9:50 ` Janosch Frank 0 siblings, 1 reply; 11+ messages in thread From: Heiko Carstens @ 2020-09-04 12:10 UTC (permalink / raw) To: Janosch Frank; +Cc: linux-s390, borntraeger, gor, imbrenda, kvm, david On Fri, Sep 04, 2020 at 01:38:53PM +0200, Janosch Frank wrote: > >> * Requests the Ultravisor to encrypt a guest page and make it > >> * accessible to the host for paging (export). > >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > >> index 373542ca1113..cfb0017f33a7 100644 > >> --- a/arch/s390/mm/gmap.c > >> +++ b/arch/s390/mm/gmap.c > >> @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, > >> pte_t pte = READ_ONCE(*ptep); > >> > >> if (pte_present(pte)) > >> - WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK)); > >> + WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); > > > > Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that > > function return void? > > > If you prefer that, I'll change the patch. Seems to be better to me. Otherwise you start to sprinkle WARN_ONs all over the code, _if_ there would be more callers. > I think we'd need a proper print of the return codes of the UVC anyway, > the warn isn't very helpful if you want to debug after the fact. Maybe a new debug feature? Well, but that's something that hasn't do anything with this code. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] s390x: uv: Add destroy page call 2020-09-04 12:10 ` Heiko Carstens @ 2020-09-07 9:50 ` Janosch Frank 0 siblings, 0 replies; 11+ messages in thread From: Janosch Frank @ 2020-09-07 9:50 UTC (permalink / raw) To: Heiko Carstens; +Cc: linux-s390, borntraeger, gor, imbrenda, kvm, david On 9/4/20 2:10 PM, Heiko Carstens wrote: > On Fri, Sep 04, 2020 at 01:38:53PM +0200, Janosch Frank wrote: >>>> * Requests the Ultravisor to encrypt a guest page and make it >>>> * accessible to the host for paging (export). >>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >>>> index 373542ca1113..cfb0017f33a7 100644 >>>> --- a/arch/s390/mm/gmap.c >>>> +++ b/arch/s390/mm/gmap.c >>>> @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, >>>> pte_t pte = READ_ONCE(*ptep); >>>> >>>> if (pte_present(pte)) >>>> - WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK)); >>>> + WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); >>> >>> Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that >>> function return void? >>> >> If you prefer that, I'll change the patch. > > Seems to be better to me. Otherwise you start to sprinkle WARN_ONs all > over the code, _if_ there would be more callers. The other call sites currently don't care about the return codes which is not optimal. I'd prefer to leave it as is and put a debug item on the todo list which takes care of providing more debug data on error. > >> I think we'd need a proper print of the return codes of the UVC anyway, >> the warn isn't very helpful if you want to debug after the fact. > > Maybe a new debug feature? Well, but that's something that hasn't do > anything with this code. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] s390x: Add 3f program exception handler 2020-09-03 13:14 [PATCH 0/2] s390x: pv: Fixes and improvements Janosch Frank 2020-09-03 13:14 ` [PATCH 1/2] s390x: uv: Add destroy page call Janosch Frank @ 2020-09-03 13:14 ` Janosch Frank 2020-09-03 14:20 ` Christian Borntraeger 2020-09-04 10:35 ` Heiko Carstens 1 sibling, 2 replies; 11+ messages in thread From: Janosch Frank @ 2020-09-03 13:14 UTC (permalink / raw) To: linux-s390; +Cc: borntraeger, gor, imbrenda, kvm, david Program exception 3f (secure storage violation) can only be detected when the CPU is running in SIE with a format 4 state description, e.g. running a protected guest. Because of this and because user space partly controls the guest memory mapping and can trigger this exception, we want to send a SIGSEGV to the process running the guest and not panic the kernel. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> CC: <stable@vger.kernel.org> # 5.7+ Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers") Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- arch/s390/kernel/pgm_check.S | 2 +- arch/s390/mm/fault.c | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S index 2c27907a5ffc..9a92638360ee 100644 --- a/arch/s390/kernel/pgm_check.S +++ b/arch/s390/kernel/pgm_check.S @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception) /* 3b */ PGM_CHECK_DEFAULT /* 3c */ PGM_CHECK(do_secure_storage_access) /* 3d */ PGM_CHECK(do_non_secure_storage_access) /* 3e */ -PGM_CHECK_DEFAULT /* 3f */ +PGM_CHECK(do_secure_storage_violation) /* 3f */ PGM_CHECK(monitor_event_exception) /* 40 */ PGM_CHECK_DEFAULT /* 41 */ PGM_CHECK_DEFAULT /* 42 */ diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c index 4c8c063bce5b..20abb7c5c540 100644 --- a/arch/s390/mm/fault.c +++ b/arch/s390/mm/fault.c @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs) } NOKPROBE_SYMBOL(do_non_secure_storage_access); +void do_secure_storage_violation(struct pt_regs *regs) +{ + char buf[TASK_COMM_LEN]; + + /* + * Either KVM messed up the secure guest mapping or the same + * page is mapped into multiple secure guests. + * + * This exception is only triggered when a guest 2 is running + * and can therefore never occur in kernel context. + */ + printk_ratelimited(KERN_WARNING + "Secure storage violation in task: %s, pid %d\n", + get_task_comm(buf, current), task_pid_nr(current)); + send_sig(SIGSEGV, current, 0); +} +NOKPROBE_SYMBOL(do_secure_storage_violation); + #else void do_secure_storage_access(struct pt_regs *regs) { @@ -869,4 +887,9 @@ void do_non_secure_storage_access(struct pt_regs *regs) { default_trap_handler(regs); } + +void do_secure_storage_violation(struct pt_regs *regs) +{ + default_trap_handler(regs); +} #endif -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] s390x: Add 3f program exception handler 2020-09-03 13:14 ` [PATCH 2/2] s390x: Add 3f program exception handler Janosch Frank @ 2020-09-03 14:20 ` Christian Borntraeger 2020-09-04 10:35 ` Heiko Carstens 1 sibling, 0 replies; 11+ messages in thread From: Christian Borntraeger @ 2020-09-03 14:20 UTC (permalink / raw) To: Janosch Frank, linux-s390; +Cc: gor, imbrenda, kvm, david On 03.09.20 15:14, Janosch Frank wrote: > Program exception 3f (secure storage violation) can only be detected > when the CPU is running in SIE with a format 4 state description, > e.g. running a protected guest. Because of this and because user > space partly controls the guest memory mapping and can trigger this > exception, we want to send a SIGSEGV to the process running the guest > and not panic the kernel. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > CC: <stable@vger.kernel.org> # 5.7+ > Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers") > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> I guess we will pick this up via the s390 tree? > --- > arch/s390/kernel/pgm_check.S | 2 +- > arch/s390/mm/fault.c | 23 +++++++++++++++++++++++ > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S > index 2c27907a5ffc..9a92638360ee 100644 > --- a/arch/s390/kernel/pgm_check.S > +++ b/arch/s390/kernel/pgm_check.S > @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception) /* 3b */ > PGM_CHECK_DEFAULT /* 3c */ > PGM_CHECK(do_secure_storage_access) /* 3d */ > PGM_CHECK(do_non_secure_storage_access) /* 3e */ > -PGM_CHECK_DEFAULT /* 3f */ > +PGM_CHECK(do_secure_storage_violation) /* 3f */ > PGM_CHECK(monitor_event_exception) /* 40 */ > PGM_CHECK_DEFAULT /* 41 */ > PGM_CHECK_DEFAULT /* 42 */ > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c > index 4c8c063bce5b..20abb7c5c540 100644 > --- a/arch/s390/mm/fault.c > +++ b/arch/s390/mm/fault.c > @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs) > } > NOKPROBE_SYMBOL(do_non_secure_storage_access); > > +void do_secure_storage_violation(struct pt_regs *regs) > +{ > + char buf[TASK_COMM_LEN]; > + > + /* > + * Either KVM messed up the secure guest mapping or the same > + * page is mapped into multiple secure guests. > + * > + * This exception is only triggered when a guest 2 is running > + * and can therefore never occur in kernel context. > + */ > + printk_ratelimited(KERN_WARNING > + "Secure storage violation in task: %s, pid %d\n", > + get_task_comm(buf, current), task_pid_nr(current)); > + send_sig(SIGSEGV, current, 0); > +} > +NOKPROBE_SYMBOL(do_secure_storage_violation); > + > #else > void do_secure_storage_access(struct pt_regs *regs) > { > @@ -869,4 +887,9 @@ void do_non_secure_storage_access(struct pt_regs *regs) > { > default_trap_handler(regs); > } > + > +void do_secure_storage_violation(struct pt_regs *regs) > +{ > + default_trap_handler(regs); > +} > #endif > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] s390x: Add 3f program exception handler 2020-09-03 13:14 ` [PATCH 2/2] s390x: Add 3f program exception handler Janosch Frank 2020-09-03 14:20 ` Christian Borntraeger @ 2020-09-04 10:35 ` Heiko Carstens 2020-09-04 11:33 ` Janosch Frank 1 sibling, 1 reply; 11+ messages in thread From: Heiko Carstens @ 2020-09-04 10:35 UTC (permalink / raw) To: Janosch Frank; +Cc: linux-s390, borntraeger, gor, imbrenda, kvm, david On Thu, Sep 03, 2020 at 09:14:35AM -0400, Janosch Frank wrote: > Program exception 3f (secure storage violation) can only be detected > when the CPU is running in SIE with a format 4 state description, > e.g. running a protected guest. Because of this and because user > space partly controls the guest memory mapping and can trigger this > exception, we want to send a SIGSEGV to the process running the guest > and not panic the kernel. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > CC: <stable@vger.kernel.org> # 5.7+ > Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers") > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > arch/s390/kernel/pgm_check.S | 2 +- > arch/s390/mm/fault.c | 23 +++++++++++++++++++++++ > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S > index 2c27907a5ffc..9a92638360ee 100644 > --- a/arch/s390/kernel/pgm_check.S > +++ b/arch/s390/kernel/pgm_check.S > @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception) /* 3b */ > PGM_CHECK_DEFAULT /* 3c */ > PGM_CHECK(do_secure_storage_access) /* 3d */ > PGM_CHECK(do_non_secure_storage_access) /* 3e */ > -PGM_CHECK_DEFAULT /* 3f */ > +PGM_CHECK(do_secure_storage_violation) /* 3f */ > PGM_CHECK(monitor_event_exception) /* 40 */ > PGM_CHECK_DEFAULT /* 41 */ > PGM_CHECK_DEFAULT /* 42 */ > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c > index 4c8c063bce5b..20abb7c5c540 100644 > --- a/arch/s390/mm/fault.c > +++ b/arch/s390/mm/fault.c > @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs) > } > NOKPROBE_SYMBOL(do_non_secure_storage_access); > > +void do_secure_storage_violation(struct pt_regs *regs) > +{ > + char buf[TASK_COMM_LEN]; > + > + /* > + * Either KVM messed up the secure guest mapping or the same > + * page is mapped into multiple secure guests. > + * > + * This exception is only triggered when a guest 2 is running > + * and can therefore never occur in kernel context. > + */ > + printk_ratelimited(KERN_WARNING > + "Secure storage violation in task: %s, pid %d\n", > + get_task_comm(buf, current), task_pid_nr(current)); Why get_task_comm() and task_pid_nr() instead of simply current->comm and current->pid? Also: is the dmesg message of any value? > + send_sig(SIGSEGV, current, 0); > +} > +NOKPROBE_SYMBOL(do_secure_storage_violation); Why is this NOKPROBE? Can this deadlock? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] s390x: Add 3f program exception handler 2020-09-04 10:35 ` Heiko Carstens @ 2020-09-04 11:33 ` Janosch Frank 2020-09-04 12:14 ` Heiko Carstens 0 siblings, 1 reply; 11+ messages in thread From: Janosch Frank @ 2020-09-04 11:33 UTC (permalink / raw) To: Heiko Carstens; +Cc: linux-s390, borntraeger, gor, imbrenda, kvm, david [-- Attachment #1.1: Type: text/plain, Size: 3127 bytes --] On 9/4/20 12:35 PM, Heiko Carstens wrote: > On Thu, Sep 03, 2020 at 09:14:35AM -0400, Janosch Frank wrote: >> Program exception 3f (secure storage violation) can only be detected >> when the CPU is running in SIE with a format 4 state description, >> e.g. running a protected guest. Because of this and because user >> space partly controls the guest memory mapping and can trigger this >> exception, we want to send a SIGSEGV to the process running the guest >> and not panic the kernel. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> CC: <stable@vger.kernel.org> # 5.7+ >> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers") >> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> >> --- >> arch/s390/kernel/pgm_check.S | 2 +- >> arch/s390/mm/fault.c | 23 +++++++++++++++++++++++ >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S >> index 2c27907a5ffc..9a92638360ee 100644 >> --- a/arch/s390/kernel/pgm_check.S >> +++ b/arch/s390/kernel/pgm_check.S >> @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception) /* 3b */ >> PGM_CHECK_DEFAULT /* 3c */ >> PGM_CHECK(do_secure_storage_access) /* 3d */ >> PGM_CHECK(do_non_secure_storage_access) /* 3e */ >> -PGM_CHECK_DEFAULT /* 3f */ >> +PGM_CHECK(do_secure_storage_violation) /* 3f */ >> PGM_CHECK(monitor_event_exception) /* 40 */ >> PGM_CHECK_DEFAULT /* 41 */ >> PGM_CHECK_DEFAULT /* 42 */ >> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c >> index 4c8c063bce5b..20abb7c5c540 100644 >> --- a/arch/s390/mm/fault.c >> +++ b/arch/s390/mm/fault.c >> @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs) >> } >> NOKPROBE_SYMBOL(do_non_secure_storage_access); >> >> +void do_secure_storage_violation(struct pt_regs *regs) >> +{ >> + char buf[TASK_COMM_LEN]; >> + >> + /* >> + * Either KVM messed up the secure guest mapping or the same >> + * page is mapped into multiple secure guests. >> + * >> + * This exception is only triggered when a guest 2 is running >> + * and can therefore never occur in kernel context. >> + */ >> + printk_ratelimited(KERN_WARNING >> + "Secure storage violation in task: %s, pid %d\n", >> + get_task_comm(buf, current), task_pid_nr(current)); > > Why get_task_comm() and task_pid_nr() instead of simply current->comm > and current->pid? Normally if there are functions to get data I assume those should be used. > Also: is the dmesg message of any value? Yes, it's import for administrators to know that an exception caused this segfault and not some memory shenanigans. As the exception only occurs if a guest runs in unsupported modes like sharing the memory between two secure guests it's a good first indication what went wrong. > >> + send_sig(SIGSEGV, current, 0); >> +} >> +NOKPROBE_SYMBOL(do_secure_storage_violation);> > Why is this NOKPROBE? Can this deadlock? Right, we don't need to export this at all, this was copied over from the export function above. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] s390x: Add 3f program exception handler 2020-09-04 11:33 ` Janosch Frank @ 2020-09-04 12:14 ` Heiko Carstens 0 siblings, 0 replies; 11+ messages in thread From: Heiko Carstens @ 2020-09-04 12:14 UTC (permalink / raw) To: Janosch Frank; +Cc: linux-s390, borntraeger, gor, imbrenda, kvm, david On Fri, Sep 04, 2020 at 01:33:28PM +0200, Janosch Frank wrote: > >> + printk_ratelimited(KERN_WARNING > >> + "Secure storage violation in task: %s, pid %d\n", > >> + get_task_comm(buf, current), task_pid_nr(current)); > > > > Why get_task_comm() and task_pid_nr() instead of simply current->comm > > and current->pid? > > Normally if there are functions to get data I assume those should be used. Could be used, however I don't see why you need that extra complexity here for both of them. > > Also: is the dmesg message of any value? > Yes, it's import for administrators to know that an exception caused > this segfault and not some memory shenanigans. > > As the exception only occurs if a guest runs in unsupported modes like > sharing the memory between two secure guests it's a good first > indication what went wrong. Yes, fine with me. Just not sure of how help this is when pid namespaces come into play... ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-09-07 9:51 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-03 13:14 [PATCH 0/2] s390x: pv: Fixes and improvements Janosch Frank 2020-09-03 13:14 ` [PATCH 1/2] s390x: uv: Add destroy page call Janosch Frank 2020-09-04 10:39 ` Heiko Carstens 2020-09-04 11:38 ` Janosch Frank 2020-09-04 12:10 ` Heiko Carstens 2020-09-07 9:50 ` Janosch Frank 2020-09-03 13:14 ` [PATCH 2/2] s390x: Add 3f program exception handler Janosch Frank 2020-09-03 14:20 ` Christian Borntraeger 2020-09-04 10:35 ` Heiko Carstens 2020-09-04 11:33 ` Janosch Frank 2020-09-04 12:14 ` Heiko Carstens
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox