From: Dave Hansen <[email protected]>
Andrey Wagin reported that a simple test case was broken by:
2b5f7d013fc ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
This test case creates an unreadable VMA and my patch assumed
that all writes must be to readable VMAs.
The simplest fix for this is to remove the pkey-related bits
in access_error(). For execute-only support, I believe the
existing version is sufficient because the permissions we
are trying to enforce are entirely expressed in vma->vm_flags.
We just depend on pkeys to get *an* exception, it does not
matter that PF_PK was set, or even what state PKRU is in.
I will re-add the necessary bits with the full pkeys
implementation that includes the new syscalls.
The three cases that matter are:
1. If a write to an execute-only VMA occurs, we will see PF_WRITE
set, but !VM_WRITE on the VMA, and return 1. All execute-only
VMAs have VM_WRITE clear by definition.
2. If a read occurs on a present PTE, we will fall in to the "read,
present" case and return 1.
3. If a read occurs to a non-present PTE, we will miss the "read,
not present" case, because the execute-only VMA will have
VM_EXEC set, and we will properly return 0 allowing the PTE to
be populated.
Test program:
#include <sys/mman.h>
#include <stdlib.h>
int main()
{
int *p;
p = mmap(NULL, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
p[0] = 1;
return 0;
}
Fixes: 62b5f7d013fc ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
Signed-off-by: Dave Hansen <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: Andrey Wagin <[email protected]>,
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
b/arch/x86/mm/fault.c | 18 ------------------
1 file changed, 18 deletions(-)
diff -puN arch/x86/mm/fault.c~pkeys-102-fix-access_error arch/x86/mm/fault.c
--- a/arch/x86/mm/fault.c~pkeys-102-fix-access_error 2016-03-01 10:14:24.436678816 -0800
+++ b/arch/x86/mm/fault.c 2016-03-01 11:31:29.059059324 -0800
@@ -1122,24 +1122,6 @@ access_error(unsigned long error_code, s
/* This is only called for the current mm, so: */
bool foreign = false;
/*
- * Access or read was blocked by protection keys. We do
- * this check before any others because we do not want
- * to, for instance, confuse a protection-key-denied
- * write with one for which we should do a COW.
- */
- if (error_code & PF_PK)
- return 1;
-
- if (!(error_code & PF_INSTR)) {
- /*
- * Assume all accesses require either read or execute
- * permissions. This is not an instruction access, so
- * it requires read permissions.
- */
- if (!(vma->vm_flags & VM_READ))
- return 1;
- }
- /*
* Make sure to check the VMA so that we do not perform
* faults just to hit a PF_PK as soon as we fill in a
* page.
_
On Tue, Mar 01, 2016 at 11:41:33AM -0800, Dave Hansen wrote:
>
> From: Dave Hansen <[email protected]>
>
> Andrey Wagin reported that a simple test case was broken by:
>
> 2b5f7d013fc ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
>
> This test case creates an unreadable VMA and my patch assumed
> that all writes must be to readable VMAs.
>
> The simplest fix for this is to remove the pkey-related bits
> in access_error(). For execute-only support, I believe the
> existing version is sufficient because the permissions we
> are trying to enforce are entirely expressed in vma->vm_flags.
> We just depend on pkeys to get *an* exception, it does not
> matter that PF_PK was set, or even what state PKRU is in.
>
> I will re-add the necessary bits with the full pkeys
> implementation that includes the new syscalls.
>
> The three cases that matter are:
>
> 1. If a write to an execute-only VMA occurs, we will see PF_WRITE
> set, but !VM_WRITE on the VMA, and return 1. All execute-only
> VMAs have VM_WRITE clear by definition.
> 2. If a read occurs on a present PTE, we will fall in to the "read,
> present" case and return 1.
> 3. If a read occurs to a non-present PTE, we will miss the "read,
> not present" case, because the execute-only VMA will have
> VM_EXEC set, and we will properly return 0 allowing the PTE to
> be populated.
>
> Test program:
>
> #include <sys/mman.h>
> #include <stdlib.h>
>
> int main()
> {
> int *p;
> p = mmap(NULL, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> p[0] = 1;
>
> return 0;
> }
>
> Fixes: 62b5f7d013fc ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
> Signed-off-by: Dave Hansen <[email protected]>
> Cc: "Kirill A. Shutemov" <[email protected]>
> Cc: Andrey Wagin <[email protected]>,
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Acked-by: Kirill A. Shutemov <[email protected]>
--
Kirill A. Shutemov
Commit-ID: e21555436f196c241503c7c6240272e57783235c
Gitweb: http://git.kernel.org/tip/e21555436f196c241503c7c6240272e57783235c
Author: Dave Hansen <[email protected]>
AuthorDate: Tue, 1 Mar 2016 11:41:33 -0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 3 Mar 2016 16:34:56 +0100
x86/mm/pkeys: Fix access_error() denial of writes to write-only VMA
Andrey Wagin reported that a simple test case was broken by:
2b5f7d013fc ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
This test case creates an unreadable VMA and my patch assumed
that all writes must be to readable VMAs.
The simplest fix for this is to remove the pkey-related bits
in access_error(). For execute-only support, I believe the
existing version is sufficient because the permissions we
are trying to enforce are entirely expressed in vma->vm_flags.
We just depend on pkeys to get *an* exception, it does not
matter that PF_PK was set, or even what state PKRU is in.
I will re-add the necessary bits with the full pkeys
implementation that includes the new syscalls.
The three cases that matter are:
1. If a write to an execute-only VMA occurs, we will see PF_WRITE
set, but !VM_WRITE on the VMA, and return 1. All execute-only
VMAs have VM_WRITE clear by definition.
2. If a read occurs on a present PTE, we will fall in to the "read,
present" case and return 1.
3. If a read occurs to a non-present PTE, we will miss the "read,
not present" case, because the execute-only VMA will have
VM_EXEC set, and we will properly return 0 allowing the PTE to
be populated.
Test program:
int main()
{
int *p;
p = mmap(NULL, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
p[0] = 1;
return 0;
}
Reported-by: Andrey Wagin <[email protected]>,
Signed-off-by: Dave Hansen <[email protected]>
Acked-by: Kirill A. Shutemov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Kirill A. Shutemov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: 62b5f7d013fc ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/mm/fault.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 5877b92..6138db4 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1101,24 +1101,6 @@ access_error(unsigned long error_code, struct vm_area_struct *vma)
/* This is only called for the current mm, so: */
bool foreign = false;
/*
- * Access or read was blocked by protection keys. We do
- * this check before any others because we do not want
- * to, for instance, confuse a protection-key-denied
- * write with one for which we should do a COW.
- */
- if (error_code & PF_PK)
- return 1;
-
- if (!(error_code & PF_INSTR)) {
- /*
- * Assume all accesses require either read or execute
- * permissions. This is not an instruction access, so
- * it requires read permissions.
- */
- if (!(vma->vm_flags & VM_READ))
- return 1;
- }
- /*
* Make sure to check the VMA so that we do not perform
* faults just to hit a PF_PK as soon as we fill in a
* page.