On 11.06.2018 09:34, Thomas Huth wrote:
> On 08.06.2018 16:01, Janosch Frank wrote:
>> From: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
>>
>> Tests no-execute (Instruction Execution Protection) DAT protection.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>> + *
>> + * 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
>
Attachment:
signature.asc
Description: OpenPGP digital signature
