| Message ID | 1528466509-23335-1-git-send-email-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08.06.2018 16:01, Janosch Frank wrote: > From: Janosch Frank <frankja@linux.vnet.ibm.com> > > Tests no-execute (Instruction Execution Protection) DAT protection. > > Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> > --- > lib/s390x/interrupt.c | 4 ++++ > lib/s390x/mmu.c | 46 ++++++++++++++++++++++++++++++++++-- > lib/s390x/mmu.h | 20 ++++++++++++++++ > s390x/Makefile | 1 + > s390x/iep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++ > s390x/unittests.cfg | 4 ++++ > 6 files changed, 138 insertions(+), 2 deletions(-) > create mode 100644 lib/s390x/mmu.h > create mode 100644 s390x/iep.c > > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c > index 56c7603..2e2610e 100644 > --- a/lib/s390x/interrupt.c > +++ b/lib/s390x/interrupt.c > @@ -51,6 +51,10 @@ static void fixup_pgm_int(void) > */ > lc->pgm_old_psw.mask &= ~PSW_MASK_PSTATE; > break; > + case PGM_INT_CODE_PROTECTION: > + if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL) > + lc->pgm_old_psw.addr = lc->sw_int_grs[14]; I think you should better also check bit 60 here to make sure that it is 0. Please also add a comment that this r14 handling is special here for the iep.c test (otherwise this could be very confusing for someone who did not look at iep.c first). > + break; > case PGM_INT_CODE_SEGMENT_TRANSLATION: > case PGM_INT_CODE_PAGE_TRANSLATION: > case PGM_INT_CODE_TRACE_TABLE: [...] > diff --git a/s390x/iep.c b/s390x/iep.c > new file mode 100644 > index 0000000..1f07741 > --- /dev/null > +++ b/s390x/iep.c > @@ -0,0 +1,65 @@ > +/* > + * Instruction Execution Prevention (IEP) DAT test. > + * > + * Copyright (c) 2018 IBM Corp > + * > + * Authors: > + * Janosch Frank <frankja@de.ibm.com> > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Library General Public License version 2. > + */ > +#include <libcflat.h> > +#include <vmalloc.h> > +#include <asm/facility.h> > +#include <asm/interrupt.h> > +#include <mmu.h> > +#include <asm/pgtable.h> > +#include <asm-generic/barrier.h> > + > +static void test_iep(void) > +{ > + uint16_t *code; > + uint8_t *iepbuf = 0; Nit: I'd prefer NULL instead of 0 here. > + void (*fn)(void); > + > + /* Enable IEP */ > + ctl_set_bit(0, 20); > + > + /* Get and protect a page with the IEP bit */ > + iepbuf = alloc_page(); > + protect_page(iepbuf, PAGE_ENTRY_IEP); > + mb(); > + > + /* Code branches into r14 which contains the return address. */ > + code = (uint16_t *)iepbuf; > + *code = 0x07fe; > + asm volatile("" : : "m" (code)); What's this asm-volatile statement here good for? Mybe add a comment to the code? > + fn = (void *)code; > + mb(); (having done a lot of ppc stuff in the past, let me ask just to be sure: We don't have to flush the data cache on s390x before doing stuff like this, do we?) > + expect_pgm_int(); > + /* Jump into protected page */ > + fn(); > + check_pgm_int_code(PGM_INT_CODE_PROTECTION); > + unprotect_page(iepbuf, PAGE_ENTRY_IEP); > + ctl_clear_bit(0, 20); > +} > + > +int main(void) > +{ > + bool has_iep = test_facility(130); > + > + report_prefix_push("iep"); > + report_xfail("DAT IEP available", !has_iep, has_iep); > + if (!has_iep) > + goto done; > + > + /* Setup DAT 1:1 mapping and memory management */ > + setup_vm(); > + test_iep(); > + > +done: > + report_prefix_pop(); > + return report_summary(); > +} Apart from the nits, the patch looks quite good to me already. Thomas
On 11.06.2018 09:34, Thomas Huth wrote: > On 08.06.2018 16:01, Janosch Frank wrote: >> From: Janosch Frank <frankja@linux.vnet.ibm.com> >> >> Tests no-execute (Instruction Execution Protection) DAT protection. >> >> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> --- >> lib/s390x/interrupt.c | 4 ++++ >> lib/s390x/mmu.c | 46 ++++++++++++++++++++++++++++++++++-- >> lib/s390x/mmu.h | 20 ++++++++++++++++ >> s390x/Makefile | 1 + >> s390x/iep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> s390x/unittests.cfg | 4 ++++ >> 6 files changed, 138 insertions(+), 2 deletions(-) >> create mode 100644 lib/s390x/mmu.h >> create mode 100644 s390x/iep.c >> >> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c >> index 56c7603..2e2610e 100644 >> --- a/lib/s390x/interrupt.c >> +++ b/lib/s390x/interrupt.c >> @@ -51,6 +51,10 @@ static void fixup_pgm_int(void) >> */ >> lc->pgm_old_psw.mask &= ~PSW_MASK_PSTATE; >> break; >> + case PGM_INT_CODE_PROTECTION: >> + if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL) >> + lc->pgm_old_psw.addr = lc->sw_int_grs[14]; > > I think you should better also check bit 60 here to make sure that it is > 0. Please also add a comment that this r14 handling is special here for > the iep.c test (otherwise this could be very confusing for someone who > did not look at iep.c first). There is currently no case where we have ones in all three bits, but I'll add it. Yes, I'll add a comment. > >> + break; >> case PGM_INT_CODE_SEGMENT_TRANSLATION: >> case PGM_INT_CODE_PAGE_TRANSLATION: >> case PGM_INT_CODE_TRACE_TABLE: > [...] >> diff --git a/s390x/iep.c b/s390x/iep.c >> new file mode 100644 >> index 0000000..1f07741 >> --- /dev/null >> +++ b/s390x/iep.c >> @@ -0,0 +1,65 @@ >> +/* >> + * Instruction Execution Prevention (IEP) DAT test. >> + * >> + * Copyright (c) 2018 IBM Corp >> + * >> + * Authors: >> + * Janosch Frank <frankja@de.ibm.com> >> + * >> + * This code is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU Library General Public License version 2. >> + */ >> +#include <libcflat.h> >> +#include <vmalloc.h> >> +#include <asm/facility.h> >> +#include <asm/interrupt.h> >> +#include <mmu.h> >> +#include <asm/pgtable.h> >> +#include <asm-generic/barrier.h> >> + >> +static void test_iep(void) >> +{ >> + uint16_t *code; >> + uint8_t *iepbuf = 0; > > Nit: I'd prefer NULL instead of 0 here. Right > >> + void (*fn)(void); >> + >> + /* Enable IEP */ >> + ctl_set_bit(0, 20); >> + >> + /* Get and protect a page with the IEP bit */ >> + iepbuf = alloc_page(); >> + protect_page(iepbuf, PAGE_ENTRY_IEP); >> + mb(); >> + >> + /* Code branches into r14 which contains the return address. */ >> + code = (uint16_t *)iepbuf; >> + *code = 0x07fe; >> + asm volatile("" : : "m" (code)); > > What's this asm-volatile statement here good for? Mybe add a comment to > the code? > >> + fn = (void *)code; >> + mb(); > > (having done a lot of ppc stuff in the past, let me ask just to be sure: > We don't have to flush the data cache on s390x before doing stuff like > this, do we?) This was not done out of knowledge, I had some crashes without the barriers a few weeks ago and sprinkled mbs over the code until it worked. Some problems I figured out later and I removed the barriers, I haven't yet tested if I can remove this one. Also I can't give you a general answer on that, I haven't yet finished memorizing the POP :) > >> + expect_pgm_int(); >> + /* Jump into protected page */ >> + fn(); >> + check_pgm_int_code(PGM_INT_CODE_PROTECTION); >> + unprotect_page(iepbuf, PAGE_ENTRY_IEP); >> + ctl_clear_bit(0, 20); >> +} >> + >> +int main(void) >> +{ >> + bool has_iep = test_facility(130); >> + >> + report_prefix_push("iep"); >> + report_xfail("DAT IEP available", !has_iep, has_iep); >> + if (!has_iep) >> + goto done; >> + >> + /* Setup DAT 1:1 mapping and memory management */ >> + setup_vm(); >> + test_iep(); >> + >> +done: >> + report_prefix_pop(); >> + return report_summary(); >> +} > > Apart from the nits, the patch looks quite good to me already. Thanks, will fix and resend. > > Thomas >
On 11.06.2018 09:45, Janosch Frank wrote: > On 11.06.2018 09:34, Thomas Huth wrote: >> On 08.06.2018 16:01, Janosch Frank wrote: >>> From: Janosch Frank <frankja@linux.vnet.ibm.com> >>> >>> Tests no-execute (Instruction Execution Protection) DAT protection. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >>> --- >>> lib/s390x/interrupt.c | 4 ++++ >>> lib/s390x/mmu.c | 46 ++++++++++++++++++++++++++++++++++-- >>> lib/s390x/mmu.h | 20 ++++++++++++++++ >>> s390x/Makefile | 1 + >>> s390x/iep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>> s390x/unittests.cfg | 4 ++++ >>> 6 files changed, 138 insertions(+), 2 deletions(-) >>> create mode 100644 lib/s390x/mmu.h >>> create mode 100644 s390x/iep.c [...] >>> + void (*fn)(void); >>> + >>> + /* Enable IEP */ >>> + ctl_set_bit(0, 20); >>> + >>> + /* Get and protect a page with the IEP bit */ >>> + iepbuf = alloc_page(); >>> + protect_page(iepbuf, PAGE_ENTRY_IEP); >>> + mb(); >>> + >>> + /* Code branches into r14 which contains the return address. */ >>> + code = (uint16_t *)iepbuf; >>> + *code = 0x07fe; >>> + asm volatile("" : : "m" (code)); >> >> What's this asm-volatile statement here good for? Mybe add a comment to >> the code? >> >>> + fn = (void *)code; >>> + mb(); >> >> (having done a lot of ppc stuff in the past, let me ask just to be sure: >> We don't have to flush the data cache on s390x before doing stuff like >> this, do we?) > > This was not done out of knowledge, I had some crashes without the > barriers a few weeks ago and sprinkled mbs over the code until it > worked. Some problems I figured out later and I removed the barriers, I > haven't yet tested if I can remove this one. The mb() is fine here, I think. I just wondered whether an additional cache flush is necessary (it would be necessary on ppc for example). But I guess s390x is strict enough that you don't need an additional cache flush here. Thomas
On 06/11/2018 09:58 AM, Thomas Huth wrote: > On 11.06.2018 09:45, Janosch Frank wrote: >> On 11.06.2018 09:34, Thomas Huth wrote: >>> On 08.06.2018 16:01, Janosch Frank wrote: >>>> From: Janosch Frank <frankja@linux.vnet.ibm.com> >>>> >>>> Tests no-execute (Instruction Execution Protection) DAT protection. >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >>>> --- >>>> lib/s390x/interrupt.c | 4 ++++ >>>> lib/s390x/mmu.c | 46 ++++++++++++++++++++++++++++++++++-- >>>> lib/s390x/mmu.h | 20 ++++++++++++++++ >>>> s390x/Makefile | 1 + >>>> s390x/iep.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> s390x/unittests.cfg | 4 ++++ >>>> 6 files changed, 138 insertions(+), 2 deletions(-) >>>> create mode 100644 lib/s390x/mmu.h >>>> create mode 100644 s390x/iep.c > [...] >>>> + void (*fn)(void); >>>> + >>>> + /* Enable IEP */ >>>> + ctl_set_bit(0, 20); >>>> + >>>> + /* Get and protect a page with the IEP bit */ >>>> + iepbuf = alloc_page(); >>>> + protect_page(iepbuf, PAGE_ENTRY_IEP); >>>> + mb(); >>>> + >>>> + /* Code branches into r14 which contains the return address. */ >>>> + code = (uint16_t *)iepbuf; >>>> + *code = 0x07fe; >>>> + asm volatile("" : : "m" (code)); >>> >>> What's this asm-volatile statement here good for? Mybe add a comment to >>> the code? >>> >>>> + fn = (void *)code; >>>> + mb(); >>> >>> (having done a lot of ppc stuff in the past, let me ask just to be sure: >>> We don't have to flush the data cache on s390x before doing stuff like >>> this, do we?) >> >> This was not done out of knowledge, I had some crashes without the >> barriers a few weeks ago and sprinkled mbs over the code until it >> worked. Some problems I figured out later and I removed the barriers, I >> haven't yet tested if I can remove this one. > > The mb() is fine here, I think. I just wondered whether an additional > cache flush is necessary (it would be necessary on ppc for example). But > I guess s390x is strict enough that you don't need an additional cache > flush here. There is no cache flush for s390. We only need to care about flushing the old TLB (so what needs to be done is an ipte (seems that install_page does that)
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c index 56c7603..2e2610e 100644 --- a/lib/s390x/interrupt.c +++ b/lib/s390x/interrupt.c @@ -51,6 +51,10 @@ static void fixup_pgm_int(void) */ lc->pgm_old_psw.mask &= ~PSW_MASK_PSTATE; break; + case PGM_INT_CODE_PROTECTION: + if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL) + lc->pgm_old_psw.addr = lc->sw_int_grs[14]; + break; case PGM_INT_CODE_SEGMENT_TRANSLATION: case PGM_INT_CODE_PAGE_TRANSLATION: case PGM_INT_CODE_TRACE_TABLE: diff --git a/lib/s390x/mmu.c b/lib/s390x/mmu.c index 288f835..d503db3 100644 --- a/lib/s390x/mmu.c +++ b/lib/s390x/mmu.c @@ -16,6 +16,8 @@ #include <asm/barrier.h> #include <vmalloc.h> +static pgd_t *table_root; + void configure_dat(int enable) { uint64_t mask; @@ -62,7 +64,7 @@ phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *vaddr) ((unsigned long)vaddr & ~PAGE_MASK); } -pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *vaddr) +static pteval_t *set_pte(pgd_t *pgtable, pteval_t val, void *vaddr) { pteval_t *p_pte = get_pte(pgtable, (uintptr_t)vaddr); @@ -70,10 +72,49 @@ pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *vaddr) if (!(*p_pte & PAGE_ENTRY_I)) ipte((uintptr_t)vaddr, p_pte); - *p_pte = __pa(phys); + *p_pte = val; return p_pte; } +pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *vaddr) +{ + return set_pte(pgtable, __pa(phys), vaddr); +} + +void protect_page(void *vaddr, unsigned long prot) +{ + pteval_t *p_pte = get_pte(table_root, (uintptr_t)vaddr); + pteval_t n_pte = *p_pte | prot; + + set_pte(table_root, n_pte, vaddr); +} + +void unprotect_page(void *vaddr, unsigned long prot) +{ + pteval_t *p_pte = get_pte(table_root, (uintptr_t)vaddr); + pteval_t n_pte = *p_pte & ~prot; + + set_pte(table_root, n_pte, vaddr); +} + +void protect_range(void *start, unsigned long len, unsigned long prot) +{ + uintptr_t curr = (uintptr_t)start & PAGE_MASK; + len &= PAGE_MASK; + + for (; len; len -= PAGE_SIZE, curr += PAGE_SIZE) + protect_page((void *)curr, prot); +} + +void unprotect_range(void *start, unsigned long len, unsigned long prot) +{ + uintptr_t curr = (uintptr_t)start & PAGE_MASK; + len &= PAGE_MASK; + + for (; len; len -= PAGE_SIZE, curr += PAGE_SIZE) + unprotect_page((void *)curr, prot); +} + static void setup_identity(pgd_t *pgtable, phys_addr_t start_addr, phys_addr_t end_addr) { @@ -104,5 +145,6 @@ void *setup_mmu(phys_addr_t phys_end){ /* finally enable DAT with the new table */ mmu_enable(page_root); + table_root = page_root; return page_root; } diff --git a/lib/s390x/mmu.h b/lib/s390x/mmu.h new file mode 100644 index 0000000..f5095fa --- /dev/null +++ b/lib/s390x/mmu.h @@ -0,0 +1,20 @@ +/* + * s390x mmu functions + * + * Copyright (c) 2018 IBM Corp + * + * Authors: + * Janosch Frank <frankja@de.ibm.com> + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU Library General Public License version 2. + */ +#ifndef _ASMS390X_MMU_H_ +#define _ASMS390X_MMU_H_ + +void protect_page(void *vaddr, unsigned long prot); +void protect_range(void *start, unsigned long len, unsigned long prot); +void unprotect_page(void *vaddr, unsigned long prot); +void unprotect_range(void *start, unsigned long len, unsigned long prot); + +#endif /* _ASMS390X_MMU_H_ */ diff --git a/s390x/Makefile b/s390x/Makefile index abc3242..d4275a1 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -9,6 +9,7 @@ tests += $(TEST_DIR)/pfmf.elf tests += $(TEST_DIR)/cmm.elf tests += $(TEST_DIR)/vector.elf tests += $(TEST_DIR)/gs.elf +tests += $(TEST_DIR)/iep.elf all: directories test_cases diff --git a/s390x/iep.c b/s390x/iep.c new file mode 100644 index 0000000..1f07741 --- /dev/null +++ b/s390x/iep.c @@ -0,0 +1,65 @@ +/* + * Instruction Execution Prevention (IEP) DAT test. + * + * Copyright (c) 2018 IBM Corp + * + * Authors: + * Janosch Frank <frankja@de.ibm.com> + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU Library General Public License version 2. + */ +#include <libcflat.h> +#include <vmalloc.h> +#include <asm/facility.h> +#include <asm/interrupt.h> +#include <mmu.h> +#include <asm/pgtable.h> +#include <asm-generic/barrier.h> + +static void test_iep(void) +{ + uint16_t *code; + uint8_t *iepbuf = 0; + void (*fn)(void); + + /* Enable IEP */ + ctl_set_bit(0, 20); + + /* Get and protect a page with the IEP bit */ + iepbuf = alloc_page(); + protect_page(iepbuf, PAGE_ENTRY_IEP); + mb(); + + /* Code branches into r14 which contains the return address. */ + code = (uint16_t *)iepbuf; + *code = 0x07fe; + asm volatile("" : : "m" (code)); + fn = (void *)code; + mb(); + + expect_pgm_int(); + /* Jump into protected page */ + fn(); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); + unprotect_page(iepbuf, PAGE_ENTRY_IEP); + ctl_clear_bit(0, 20); +} + +int main(void) +{ + bool has_iep = test_facility(130); + + report_prefix_push("iep"); + report_xfail("DAT IEP available", !has_iep, has_iep); + if (!has_iep) + goto done; + + /* Setup DAT 1:1 mapping and memory management */ + setup_vm(); + test_iep(); + +done: + report_prefix_pop(); + return report_summary(); +} diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index ff7eea1..760402e 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -55,3 +55,7 @@ file = vector.elf [gs] file = gs.elf + +[iep] +file = iep.elf +