Re: [bug] LTP mmap03 stuck in page fault loop after c46241a370a6 ("powerpc/pkeys: Check vma before returning key fault error to the user")

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 6/26/20 1:17 PM, Aneesh Kumar K.V wrote:
> Hi Jan,
> 
> On 6/26/20 12:29 PM, Jan Stancek wrote:
>> Hi,
>>
>> LTP mmap03 is getting stuck in page fault loop after commit
>>    c46241a370a6 ("powerpc/pkeys: Check vma before returning key fault 
>> error to the user")
>>
>> System is ppc64le P9 lpar [1] running v5.8-rc2-34-g3e08a95294a4.
>>
>> Here's a minimized reproducer:
>> ------------------------- 8< -----------------------------
>> #include <fcntl.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>> #include <sys/mman.h>
>>
>> int main(int ac, char **av)
>> {
>>          int page_sz = getpagesize();
>>          int fildes;
>>          char *addr;
>>
>>          fildes = open("tempfile", O_WRONLY | O_CREAT, 0666);
>>          write(fildes, &fildes, sizeof(fildes));
>>          close(fildes);
>>
>>          fildes = open("tempfile", O_RDONLY);
>>          unlink("tempfile");
>>
>>          addr = mmap(0, page_sz, PROT_EXEC, MAP_FILE | MAP_PRIVATE, 
>> fildes, 0);
>>
>>          printf("%d\n", *addr);
>>          return 0;
>> }
>> ------------------------- >8 -----------------------------
> 
> Thanks for the report. This is execute only key where vma has the 
> implied read permission. So The patch do break this case. I will see how 
> best we can handle  PROT_EXEC and the multi threaded test that required 
> the change in the patch.
> 

Can you check with this change? While checking for access permission we 
are checking against UAMOR value which i think is wrong. We just need to 
look at the AMR and IAMR values to check whether access is permitted or 
not. Even if UAMOR deny the userspace management of the key, we should
do the correct access check.

modified   arch/powerpc/mm/book3s64/pkeys.c
@@ -353,9 +353,6 @@ static bool pkey_access_permitted(int pkey, bool 
write, bool execute)
  	int pkey_shift;
  	u64 amr;

-	if (!is_pkey_enabled(pkey))
-		return true;
-
  	pkey_shift = pkeyshift(pkey);
  	if (execute && !(read_iamr() & (IAMR_EX_BIT << pkey_shift)))
  		return true;