Re: [PATCH v2] selftests: powerpc: Add test for execute-disabled pkeys

[Sandipan Das <sandipan@linux.ibm.com>]

Hi Michael,

On 01/06/20 7:29 am, Sandipan Das wrote:
> Hi Michael,
> 
> Thanks for your suggestions. I had a few questions regarding some
> of them.
> 
> On 29/05/20 7:18 am, Michael Ellerman wrote:
>>> [...]
>>> +
>>> +static void pkeyreg_set(unsigned long uamr)
>>> +{
>>> +	asm volatile("isync; mtspr	0xd, %0; isync;" : : "r"(uamr));
>>> +}
>>
>> You can use mtspr() there, but you'll need to add the isync's yourself.
>>
> 
> Would it make sense to add a new macro that adds the CSI instructions?
> Something like this.
> 
> diff --git a/tools/testing/selftests/powerpc/include/reg.h b/tools/testing/selftests/powerpc/include/reg.h
> index 022c5076b2c5..d60f66380cad 100644
> --- a/tools/testing/selftests/powerpc/include/reg.h
> +++ b/tools/testing/selftests/powerpc/include/reg.h
> @@ -15,6 +15,10 @@
>  #define mtspr(rn, v)   asm volatile("mtspr " _str(rn) ",%0" : \
>                                     : "r" ((unsigned long)(v)) \
>                                     : "memory")
> +#define mtspr_csi(rn, v)       ({ \
> +                       asm volatile("isync; mtspr " _str(rn) ",%0; isync;" : \
> +                                   : "r" ((unsigned long)(v)) \
> +                                   : "memory"); })
>  
>  #define mb()           asm volatile("sync" : : : "memory");
>  #define barrier()      asm volatile("" : : : "memory");
> 
> 
> I also noticed that two of the ptrace-related pkey tests were also not
> using CSIs. I will fix those too.
> 
>>> [...]
>>> +	/* The following two cases will avoid SEGV_PKUERR */
>>> +	ftype = -1;
>>> +	fpkey = -1;
>>> +
>>> +	/*
>>> +	 * Read an instruction word from the address when AMR bits
>>> +	 * are not set.
>>
>> You should explain for people who aren't familiar with the ISA that "AMR
>> bits not set" means "read/write access allowed".
>>
>>> +	 *
>>> +	 * This should not generate a fault as having PROT_EXEC
>>> +	 * implicitly allows reads. The pkey currently restricts
>>
>> Whether PROT_EXEC implies read is not well defined (see the man page).
>> If you want to test this case I think you'd be better off specifying
>> PROT_EXEC | PROT_READ explicitly.
>>
> 
> But I guess specifying PROT_EXEC | PROT_READ defeats the purpose? I can
> tweak the passing condition though based on whether READ_IMPLIES_EXEC is
> set in the personality.
> 

Sorry, I read this the other way round. This won't work.

>> [...]
>>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>>> +
>>> +	/* The following three cases will generate SEGV_PKUERR */
>>> +	ftype = PKEY_DISABLE_ACCESS;
>>> +	fpkey = pkey;
>>> +
>>> +	/*
>>> +	 * Read an instruction word from the address when AMR bits
>>> +	 * are set.
>>> +	 *
>>> +	 * This should generate a pkey fault based on AMR bits only
>>> +	 * as having PROT_EXEC implicitly allows reads.
>>
>> Again would be better to specify PROT_READ IMHO.
>>
> 
> I can use a personality check here too.

Same here.

> 
>>> +	 */
>>> +	faults = 1;
>>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>>> +	printf("read from %p, pkey is execute-disabled, access-disabled\n",
>>> +	       (void *) faddr);
>>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>>> +	i = *faddr;
>>> +	FAIL_IF(faults != 0 || fcode != SEGV_PKUERR);
>>> +
>>> +	/*
>>> +	 * Write an instruction word to the address when AMR bits
>>> +	 * are set.
>>> +	 *
>>> +	 * This should generate two faults. First, a pkey fault based
>>> +	 * on AMR bits and then an access fault based on PROT_EXEC.
>>> +	 */
>>> +	faults = 2;
>>
>> Setting faults to the expected value and decrementing it in the fault
>> handler is kind of weird.
>>
>> I think it would be clearer if faults was just a zero-based counter of
>> how many faults we've taken, and then you test that it's == 2 below.
>>
>>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>>> +	printf("write to %p, pkey is execute-disabled, access-disabled\n",
>>> +	       (void *) faddr);
>>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>>> +	*faddr = 0x60000000;	/* nop */
>>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>>
>> ie. FAIL_IF(faults != 2 || ... )
>>
> 
> Agreed, it is weird. IIRC, I did this to make sure that if the test
> kept getting repeated faults at the same address and exceeded the
> maximum number of expected faults i.e. it gets another fault when
> 'faults' is already zero, then the signal handler will attempt to
> let the program continue by giving all permissions to the page and
> also the pkey. Would it make sense to just rename 'faults' to
> something like 'remaining_faults'?
> 
> 
> - Sandipan
>