Hi all,
This series implements the Permission Overlay Extension introduced in 2022
VMSA enhancements [1]. It is based on v6.11-rc4.
Changes since v4[2]:
- Added Acks and R-bs, thanks!
- KVM:
- Move POR_EL{0,1} handling inside TCR_EL2 blocks
- Add visibility functions for registers [4]
- Make ID_AA64MMFR3_EL1 writable
- use system_supports_poe() more consistently
- use BIT instead of hex constants
- fix off-by-one in arch_max_pkey() macro
- add PKEY_DISABLE_EXECUTE and PKEY_DISABLE_READ
- Update some comments and commit messages.
- No change to when we save/restore POR_EL0 for signals!
Conflicts with GCS:
- Uses the same (last) bit in HWCAP2
- Uses the same VM_HIGH_ARCH_5
Conflicts with arm64 KVM:
- Maz has taken patch 8 into one of his own series
- I have taken and modified a patch from Maz (patch 9)
The Permission Overlay Extension allows to constrain permissions on memory
regions. This can be used from userspace (EL0) without a system call or TLB
invalidation.
POE is used to implement the Memory Protection Keys [3] Linux syscall.
The first few patches add the basic framework, then the PKEYS interface is
implemented, and then the selftests are made to work on arm64.
I have tested the modified protection_keys test on x86_64, but not PPC.
I haven't build tested the x86/ppc arch changes.
Thanks,
Joey
[1] https://community.arm.com/arm-community-blogs/b/architectures-and-processors-blog/posts/arm-a-profile-architecture-2022
[2] https://lore.kernel.org/linux-arm-kernel/20240503130147.1154804-1-joey.gouly@arm.com/
[3] Documentation/core-api/protection-keys.rst
[4] https://lore.kernel.org/linux-arm-kernel/20240806-kvm-arm64-get-reg-list-v2-0-1d3fbc7b6844@kernel.org/
Joey Gouly (30):
powerpc/mm: add ARCH_PKEY_BITS to Kconfig
x86/mm: add ARCH_PKEY_BITS to Kconfig
mm: use ARCH_PKEY_BITS to define VM_PKEY_BITN
arm64: disable trapping of POR_EL0 to EL2
arm64: cpufeature: add Permission Overlay Extension cpucap
arm64: context switch POR_EL0 register
KVM: arm64: Save/restore POE registers
KVM: arm64: make kvm_at() take an OP_AT_*
KVM: arm64: use `at s1e1a` for POE
KVM: arm64: Sanitise ID_AA64MMFR3_EL1
arm64: enable the Permission Overlay Extension for EL0
arm64: re-order MTE VM_ flags
arm64: add POIndex defines
arm64: convert protection key into vm_flags and pgprot values
arm64: mask out POIndex when modifying a PTE
arm64: handle PKEY/POE faults
arm64: add pte_access_permitted_no_overlay()
arm64: implement PKEYS support
arm64: add POE signal support
arm64/ptrace: add support for FEAT_POE
arm64: enable POE and PIE to coexist
arm64: enable PKEY support for CPUs with S1POE
arm64: add Permission Overlay Extension Kconfig
kselftest/arm64: move get_header()
selftests: mm: move fpregs printing
selftests: mm: make protection_keys test work on arm64
kselftest/arm64: add HWCAP test for FEAT_S1POE
kselftest/arm64: parse POE_MAGIC in a signal frame
kselftest/arm64: Add test case for POR_EL0 signal frame records
KVM: selftests: get-reg-list: add Permission Overlay registers
Documentation/arch/arm64/elf_hwcaps.rst | 2 +
arch/arm64/Kconfig | 23 +++
arch/arm64/include/asm/cpufeature.h | 6 +
arch/arm64/include/asm/el2_setup.h | 10 +-
arch/arm64/include/asm/hwcap.h | 1 +
arch/arm64/include/asm/kvm_asm.h | 3 +-
arch/arm64/include/asm/kvm_host.h | 4 +
arch/arm64/include/asm/mman.h | 10 +-
arch/arm64/include/asm/mmu.h | 1 +
arch/arm64/include/asm/mmu_context.h | 46 +++++-
arch/arm64/include/asm/pgtable-hwdef.h | 10 ++
arch/arm64/include/asm/pgtable-prot.h | 8 +-
arch/arm64/include/asm/pgtable.h | 34 ++++-
arch/arm64/include/asm/pkeys.h | 108 ++++++++++++++
arch/arm64/include/asm/por.h | 33 +++++
arch/arm64/include/asm/processor.h | 1 +
arch/arm64/include/asm/sysreg.h | 3 +
arch/arm64/include/asm/traps.h | 1 +
arch/arm64/include/asm/vncr_mapping.h | 1 +
arch/arm64/include/uapi/asm/hwcap.h | 1 +
arch/arm64/include/uapi/asm/mman.h | 9 ++
arch/arm64/include/uapi/asm/sigcontext.h | 7 +
arch/arm64/kernel/cpufeature.c | 23 +++
arch/arm64/kernel/cpuinfo.c | 1 +
arch/arm64/kernel/process.c | 28 ++++
arch/arm64/kernel/ptrace.c | 46 ++++++
arch/arm64/kernel/signal.c | 62 ++++++++
arch/arm64/kernel/traps.c | 6 +
arch/arm64/kvm/hyp/include/hyp/fault.h | 5 +-
arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 27 ++++
arch/arm64/kvm/sys_regs.c | 25 +++-
arch/arm64/mm/fault.c | 55 ++++++-
arch/arm64/mm/mmap.c | 11 ++
arch/arm64/mm/mmu.c | 45 ++++++
arch/arm64/tools/cpucaps | 1 +
arch/powerpc/Kconfig | 4 +
arch/x86/Kconfig | 4 +
fs/proc/task_mmu.c | 2 +
include/linux/mm.h | 20 ++-
include/uapi/linux/elf.h | 1 +
tools/testing/selftests/arm64/abi/hwcap.c | 14 ++
.../testing/selftests/arm64/signal/.gitignore | 1 +
.../arm64/signal/testcases/poe_siginfo.c | 86 +++++++++++
.../arm64/signal/testcases/testcases.c | 27 +---
.../arm64/signal/testcases/testcases.h | 28 +++-
.../selftests/kvm/aarch64/get-reg-list.c | 14 ++
tools/testing/selftests/mm/Makefile | 2 +-
tools/testing/selftests/mm/pkey-arm64.h | 139 ++++++++++++++++++
tools/testing/selftests/mm/pkey-helpers.h | 8 +
tools/testing/selftests/mm/pkey-powerpc.h | 3 +
tools/testing/selftests/mm/pkey-x86.h | 4 +
tools/testing/selftests/mm/protection_keys.c | 109 ++++++++++++--
52 files changed, 1060 insertions(+), 63 deletions(-)
create mode 100644 arch/arm64/include/asm/pkeys.h
create mode 100644 arch/arm64/include/asm/por.h
create mode 100644 tools/testing/selftests/arm64/signal/testcases/poe_siginfo.c
create mode 100644 tools/testing/selftests/mm/pkey-arm64.h
--
2.25.1
The new config option specifies how many bits are in each PKEY.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <redacted>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
Cc: "Naveen N. Rao" <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/Kconfig | 4 ++++
1 file changed, 4 insertions(+)
The new config option specifies how many bits are in each PKEY.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: x86@kernel.org
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
---
arch/x86/Kconfig | 4 ++++
1 file changed, 4 insertions(+)
This indicates if the system supports POE. This is a CPUCAP_BOOT_CPU_FEATURE
as the boot CPU will enable POE if it has it, so secondary CPUs must also
have this feature.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Anshuman Khandual <redacted>
---
arch/arm64/kernel/cpufeature.c | 9 +++++++++
arch/arm64/tools/cpucaps | 1 +
2 files changed, 10 insertions(+)
POR_EL0 is a register that can be modified by userspace directly,
so it must be context switched.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 6 ++++++
arch/arm64/include/asm/processor.h | 1 +
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kernel/process.c | 28 ++++++++++++++++++++++++++++
4 files changed, 38 insertions(+)
@@ -1077,6 +1077,9 @@#define POE_RXW UL(0x7)#define POE_MASK UL(0xf)+/* Initial value for Permission Overlay Extension for EL0 */+#define POR_EL0_INIT POE_RXW+#define ARM64_FEATURE_FIELD_BITS 4/* Defined for compatibility only, do not add new users. */
From: Will Deacon <will@kernel.org> Date: 2024-08-23 14:45:40
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
POR_EL0 is a register that can be modified by userspace directly,
so it must be context switched.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 6 ++++++
arch/arm64/include/asm/processor.h | 1 +
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kernel/process.c | 28 ++++++++++++++++++++++++++++
4 files changed, 38 insertions(+)
[...]
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Will
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
--
Catalin
From: Will Deacon <will@kernel.org> Date: 2024-08-23 17:08:44
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
Hmm, I wondered if that was the case. It's a bit weird though, because:
- There's a window between switch_mm() and switch_to() where you might
reasonably expect to be able to execute uaccess routines
- kthread_use_mm() doesn't/can't look at this at all
- GUP obviously doesn't care
So what do we actually gain by having the uaccess routines honour this?
Will
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
Hmm, I wondered if that was the case. It's a bit weird though, because:
- There's a window between switch_mm() and switch_to() where you might
reasonably expect to be able to execute uaccess routines
I don't think we can have any uaccess between these two switches (a
uaccess could fault, that's a pretty weird state between these two).
- kthread_use_mm() doesn't/can't look at this at all
No, but a kthread would have it's own, most permissive, POR_EL0.
- GUP obviously doesn't care
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
--
Catalin
From: Will Deacon <will@kernel.org> Date: 2024-08-27 11:38:12
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
Hmm, I wondered if that was the case. It's a bit weird though, because:
- There's a window between switch_mm() and switch_to() where you might
reasonably expect to be able to execute uaccess routines
I don't think we can have any uaccess between these two switches (a
uaccess could fault, that's a pretty weird state between these two).
quoted
- kthread_use_mm() doesn't/can't look at this at all
No, but a kthread would have it's own, most permissive, POR_EL0.
quoted
- GUP obviously doesn't care
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in
that case?
Will
On Tue, Aug 27, 2024 at 12:38:04PM +0100, Will Deacon wrote:
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
[...]
quoted
quoted
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in
that case?
Yes, it will retry and this should be fine (I assume you are only
talking about the dropping ISB in the context switch).
For the case of running with a more permissive stale POR_EL0, arguably it's
slightly more predictable for the user but, OTOH, some syscalls like
readv() could be routed through GUP with no checks. As with MTE, we
don't guarantee uaccesses honour the user permissions.
That said, at some point we should sanitise this path anyway and have a
single ISB at the end. In the meantime, I'm fine with dropping the ISB
here.
--
Catalin
On Mon, Sep 02, 2024 at 08:08:08PM +0100, Catalin Marinas wrote:
On Tue, Aug 27, 2024 at 12:38:04PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
[...]
quoted
quoted
quoted
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in
that case?
Yes, it will retry and this should be fine (I assume you are only
talking about the dropping ISB in the context switch).
For the case of running with a more permissive stale POR_EL0, arguably it's
slightly more predictable for the user but, OTOH, some syscalls like
readv() could be routed through GUP with no checks. As with MTE, we
don't guarantee uaccesses honour the user permissions.
That said, at some point we should sanitise this path anyway and have a
single ISB at the end. In the meantime, I'm fine with dropping the ISB
here.
From: Will Deacon <will@kernel.org> Date: 2024-09-04 10:23:02
On Tue, Sep 03, 2024 at 03:54:13PM +0100, Joey Gouly wrote:
quoted hunk
On Mon, Sep 02, 2024 at 08:08:08PM +0100, Catalin Marinas wrote:
quoted
On Tue, Aug 27, 2024 at 12:38:04PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
[...]
quoted
quoted
quoted
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in
that case?
Yes, it will retry and this should be fine (I assume you are only
talking about the dropping ISB in the context switch).
For the case of running with a more permissive stale POR_EL0, arguably it's
slightly more predictable for the user but, OTOH, some syscalls like
readv() could be routed through GUP with no checks. As with MTE, we
don't guarantee uaccesses honour the user permissions.
That said, at some point we should sanitise this path anyway and have a
single ISB at the end. In the meantime, I'm fine with dropping the ISB
here.
@@ -515,11 +515,8 @@ static void permission_overlay_switch(struct task_struct *next)return;current->thread.por_el0=read_sysreg_s(SYS_POR_EL0);-if(current->thread.por_el0!=next->thread.por_el0){+if(current->thread.por_el0!=next->thread.por_el0)write_sysreg_s(next->thread.por_el0,SYS_POR_EL0);-/* ISB required for kernel uaccess routines when chaning POR_EL0 */-isb();-}}
What about the one in flush_poe()? I'm inclined to drop that as well.
Will, do you want me to re-send the series with this and the permissions
diff from the other thread [1],
or you ok with applying them when you pull it in?
I'll have a crack now, but if it fails miserably then I'll let you know.
Will
On Wed, Sep 04, 2024 at 11:22:54AM +0100, Will Deacon wrote:
On Tue, Sep 03, 2024 at 03:54:13PM +0100, Joey Gouly wrote:
quoted
On Mon, Sep 02, 2024 at 08:08:08PM +0100, Catalin Marinas wrote:
quoted
On Tue, Aug 27, 2024 at 12:38:04PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
[...]
quoted
quoted
quoted
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in
that case?
Yes, it will retry and this should be fine (I assume you are only
talking about the dropping ISB in the context switch).
For the case of running with a more permissive stale POR_EL0, arguably it's
slightly more predictable for the user but, OTOH, some syscalls like
readv() could be routed through GUP with no checks. As with MTE, we
don't guarantee uaccesses honour the user permissions.
That said, at some point we should sanitise this path anyway and have a
single ISB at the end. In the meantime, I'm fine with dropping the ISB
here.
@@ -515,11 +515,8 @@ static void permission_overlay_switch(struct task_struct *next)return;current->thread.por_el0=read_sysreg_s(SYS_POR_EL0);-if(current->thread.por_el0!=next->thread.por_el0){+if(current->thread.por_el0!=next->thread.por_el0)write_sysreg_s(next->thread.por_el0,SYS_POR_EL0);-/* ISB required for kernel uaccess routines when chaning POR_EL0 */-isb();-}}
What about the one in flush_poe()? I'm inclined to drop that as well.
Yes I guess that one can be removed too. Catalin any comments?
quoted
Will, do you want me to re-send the series with this and the permissions
diff from the other thread [1],
or you ok with applying them when you pull it in?
I'll have a crack now, but if it fails miserably then I'll let you know.
From: Will Deacon <will@kernel.org> Date: 2024-09-04 11:43:10
On Wed, Sep 04, 2024 at 12:32:21PM +0100, Joey Gouly wrote:
On Wed, Sep 04, 2024 at 11:22:54AM +0100, Will Deacon wrote:
quoted
On Tue, Sep 03, 2024 at 03:54:13PM +0100, Joey Gouly wrote:
quoted
On Mon, Sep 02, 2024 at 08:08:08PM +0100, Catalin Marinas wrote:
quoted
On Tue, Aug 27, 2024 at 12:38:04PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
[...]
quoted
quoted
quoted
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in
that case?
Yes, it will retry and this should be fine (I assume you are only
talking about the dropping ISB in the context switch).
For the case of running with a more permissive stale POR_EL0, arguably it's
slightly more predictable for the user but, OTOH, some syscalls like
readv() could be routed through GUP with no checks. As with MTE, we
don't guarantee uaccesses honour the user permissions.
That said, at some point we should sanitise this path anyway and have a
single ISB at the end. In the meantime, I'm fine with dropping the ISB
here.
@@ -515,11 +515,8 @@ static void permission_overlay_switch(struct task_struct *next)return;current->thread.por_el0=read_sysreg_s(SYS_POR_EL0);-if(current->thread.por_el0!=next->thread.por_el0){+if(current->thread.por_el0!=next->thread.por_el0)write_sysreg_s(next->thread.por_el0,SYS_POR_EL0);-/* ISB required for kernel uaccess routines when chaning POR_EL0 */-isb();-}}
What about the one in flush_poe()? I'm inclined to drop that as well.
Yes I guess that one can be removed too. Catalin any comments?
quoted
quoted
Will, do you want me to re-send the series with this and the permissions
diff from the other thread [1],
or you ok with applying them when you pull it in?
I'll have a crack now, but if it fails miserably then I'll let you know.
Right, there's quite a lot I need to do:
- Uncorrupt your patches
- Fix the conflict in the kvm selftests
- Drop the unnecessary ISBs
- Fix the ESR checking
- Fix the el2_setup labels
- Reorder the patches
- Drop the patch that is already in kvmarm
Working on it...
Will
On Wed, Sep 04, 2024 at 12:43:02PM +0100, Will Deacon wrote:
On Wed, Sep 04, 2024 at 12:32:21PM +0100, Joey Gouly wrote:
quoted
On Wed, Sep 04, 2024 at 11:22:54AM +0100, Will Deacon wrote:
quoted
On Tue, Sep 03, 2024 at 03:54:13PM +0100, Joey Gouly wrote:
quoted
On Mon, Sep 02, 2024 at 08:08:08PM +0100, Catalin Marinas wrote:
quoted
On Tue, Aug 27, 2024 at 12:38:04PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 07:40:52PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 06:08:36PM +0100, Will Deacon wrote:
quoted
On Fri, Aug 23, 2024 at 05:41:06PM +0100, Catalin Marinas wrote:
quoted
On Fri, Aug 23, 2024 at 03:45:32PM +0100, Will Deacon wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:49PM +0100, Joey Gouly wrote:
quoted
+static void permission_overlay_switch(struct task_struct *next)
+{
+ if (!system_supports_poe())
+ return;
+
+ current->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
+ if (current->thread.por_el0 != next->thread.por_el0) {
+ write_sysreg_s(next->thread.por_el0, SYS_POR_EL0);
+ /* ISB required for kernel uaccess routines when chaning POR_EL0 */
nit: typo "chaning".
But more substantially, is this just to prevent spurious faults in the
context of a new thread using a stale value for POR_EL0?
Not just prevent faults but enforce the permissions from the new
thread's POR_EL0. The kernel may continue with a uaccess routine from
here, we can't tell.
[...]
quoted
quoted
quoted
So what do we actually gain by having the uaccess routines honour this?
I guess where it matters is more like not accidentally faulting because
the previous thread had more restrictive permissions.
That's what I wondered initially, but won't the fault handler retry in
that case?
Yes, it will retry and this should be fine (I assume you are only
talking about the dropping ISB in the context switch).
For the case of running with a more permissive stale POR_EL0, arguably it's
slightly more predictable for the user but, OTOH, some syscalls like
readv() could be routed through GUP with no checks. As with MTE, we
don't guarantee uaccesses honour the user permissions.
That said, at some point we should sanitise this path anyway and have a
single ISB at the end. In the meantime, I'm fine with dropping the ISB
here.
@@ -515,11 +515,8 @@ static void permission_overlay_switch(struct task_struct *next)return;current->thread.por_el0=read_sysreg_s(SYS_POR_EL0);-if(current->thread.por_el0!=next->thread.por_el0){+if(current->thread.por_el0!=next->thread.por_el0)write_sysreg_s(next->thread.por_el0,SYS_POR_EL0);-/* ISB required for kernel uaccess routines when chaning POR_EL0 */-isb();-}}
What about the one in flush_poe()? I'm inclined to drop that as well.
Yes I guess that one can be removed too. Catalin any comments?
quoted
quoted
Will, do you want me to re-send the series with this and the permissions
diff from the other thread [1],
or you ok with applying them when you pull it in?
I'll have a crack now, but if it fails miserably then I'll let you know.
Right, there's quite a lot I need to do:
- Uncorrupt your patches
- Fix the conflict in the kvm selftests
- Drop the unnecessary ISBs
- Fix the ESR checking
- Fix the el2_setup labels
- Reorder the patches
- Drop the patch that is already in kvmarm
Working on it...
Sorry! I'm happy to rebase onto some arm64 branch if that will help, just let me know.
From: Will Deacon <will@kernel.org> Date: 2024-09-04 16:18:07
On Wed, Sep 04, 2024 at 01:55:03PM +0100, Joey Gouly wrote:
On Wed, Sep 04, 2024 at 12:43:02PM +0100, Will Deacon wrote:
quoted
Right, there's quite a lot I need to do:
- Uncorrupt your patches
- Fix the conflict in the kvm selftests
- Drop the unnecessary ISBs
- Fix the ESR checking
- Fix the el2_setup labels
- Reorder the patches
- Drop the patch that is already in kvmarm
Working on it...
Sorry! I'm happy to rebase onto some arm64 branch if that will help, just let me know.
Please have a look at for-next/poe (also merged into for-next/core and
for-kernelci) and let me know what I got wrong!
For Marc: I reordered the series so the KVM bits (and deps) are all the
beginning, should you need them. The branch is based on a merge of the
shared branch you created previously.
Cheers,
Will
From: Marc Zyngier <maz@kernel.org> Date: 2024-09-04 17:05:29
On Wed, 04 Sep 2024 17:17:58 +0100,
Will Deacon [off-list ref] wrote:
On Wed, Sep 04, 2024 at 01:55:03PM +0100, Joey Gouly wrote:
quoted
On Wed, Sep 04, 2024 at 12:43:02PM +0100, Will Deacon wrote:
quoted
Right, there's quite a lot I need to do:
- Uncorrupt your patches
- Fix the conflict in the kvm selftests
- Drop the unnecessary ISBs
- Fix the ESR checking
- Fix the el2_setup labels
- Reorder the patches
- Drop the patch that is already in kvmarm
Working on it...
Sorry! I'm happy to rebase onto some arm64 branch if that will help, just let me know.
Please have a look at for-next/poe (also merged into for-next/core and
for-kernelci) and let me know what I got wrong!
For Marc: I reordered the series so the KVM bits (and deps) are all the
beginning, should you need them. The branch is based on a merge of the
shared branch you created previously.
I just had a quick check, and while there is a small conflict with
kvmarm/next, it is extremely minor (small clash in the vcpu_sysreg,
for which the resolving order doesn't matter), and not worth dragging
additional patches in the shared branch.
However, if KVM's own S1PIE series [1] ends up being merged (which I'd
really like), I will definitely have to pull the prefix in, as this is
a bit more involved conflict wise.
Thanks,
M.
[1] http://lore.kernel.org/all/20240903153834.1909472-1-maz@kernel.org
--
Without deviation from the norm, progress is not possible.
On Wed, Sep 04, 2024 at 05:17:58PM +0100, Will Deacon wrote:
On Wed, Sep 04, 2024 at 01:55:03PM +0100, Joey Gouly wrote:
quoted
On Wed, Sep 04, 2024 at 12:43:02PM +0100, Will Deacon wrote:
quoted
Right, there's quite a lot I need to do:
- Uncorrupt your patches
- Fix the conflict in the kvm selftests
- Drop the unnecessary ISBs
- Fix the ESR checking
- Fix the el2_setup labels
- Reorder the patches
- Drop the patch that is already in kvmarm
Working on it...
Sorry! I'm happy to rebase onto some arm64 branch if that will help, just let me know.
Please have a look at for-next/poe (also merged into for-next/core and
for-kernelci) and let me know what I got wrong!
I pulled for-next/poe and ran the test and it works fine. Also looked at the
diff of my branch against your branch, and it looks fine too.
Thanks for your work to get this merged!
For Marc: I reordered the series so the KVM bits (and deps) are all the
beginning, should you need them. The branch is based on a merge of the
shared branch you created previously.
Cheers,
Will
From: Kevin Brodsky <hidden> Date: 2024-09-11 15:02:01
On 22/08/2024 17:10, Joey Gouly wrote:
quoted hunk
@@ -371,6 +382,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (system_supports_tpidr2()) p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);+ if (system_supports_poe())+ p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
Here we are only reloading POR_EL0's value if the target is a user
thread. However, as this series stands, POR_EL0 is also relevant to
kthreads, because any uaccess or GUP done from a kthread will also be
checked against POR_EL0. This is especially important in cases like the
io_uring kthread, which accesses the memory of the user process that
spawned it. To prevent such a kthread from inheriting a stale value of
POR_EL0, it seems that we should reload POR_EL0's value in all cases
(user and kernel thread).
Other approaches could also be considered (e.g. resetting POR_EL0 to
unrestricted when creating a kthread), see my reply on v4 [1].
Kevin
[1]
https://lore.kernel.org/linux-arm-kernel/b4f8b351-4c83-43b4-bfbe-8f67f3f56fb9@arm.com/
From: Dave Hansen <hidden> Date: 2024-09-11 15:34:16
On 9/11/24 08:01, Kevin Brodsky wrote:
On 22/08/2024 17:10, Joey Gouly wrote:
quoted
@@ -371,6 +382,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (system_supports_tpidr2()) p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);+ if (system_supports_poe())+ p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
Here we are only reloading POR_EL0's value if the target is a user
thread. However, as this series stands, POR_EL0 is also relevant to
kthreads, because any uaccess or GUP done from a kthread will also be
checked against POR_EL0. This is especially important in cases like the
io_uring kthread, which accesses the memory of the user process that
spawned it. To prevent such a kthread from inheriting a stale value of
POR_EL0, it seems that we should reload POR_EL0's value in all cases
(user and kernel thread).
The problem with this is trying to figure out which POR_EL0 to use. The
kthread could have been spawned ages ago and might not have a POR_EL0
which is very different from the current value of any of the threads in
the process right now.
There's also no great way for a kthread to reach out and grab an updated
value. It's all completely inherently racy.
Other approaches could also be considered (e.g. resetting POR_EL0 to
unrestricted when creating a kthread), see my reply on v4 [1].
I kinda think this is the only way to go. It's the only sensible,
predictable way. I _think_ it's what x86 will end up doing with PKRU,
but there's been enough churn there that I'd need to go double check
what happens in practice.
Either way, it would be nice to get an io_uring test in here that
actually spawns kthreads:
tools/testing/selftests/mm/protection_keys.c
From: Will Deacon <will@kernel.org> Date: 2024-09-12 10:50:26
Hi Dave,
On Wed, Sep 11, 2024 at 08:33:54AM -0700, Dave Hansen wrote:
On 9/11/24 08:01, Kevin Brodsky wrote:
quoted
On 22/08/2024 17:10, Joey Gouly wrote:
quoted
@@ -371,6 +382,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (system_supports_tpidr2()) p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);+ if (system_supports_poe())+ p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
Here we are only reloading POR_EL0's value if the target is a user
thread. However, as this series stands, POR_EL0 is also relevant to
kthreads, because any uaccess or GUP done from a kthread will also be
checked against POR_EL0. This is especially important in cases like the
io_uring kthread, which accesses the memory of the user process that
spawned it. To prevent such a kthread from inheriting a stale value of
POR_EL0, it seems that we should reload POR_EL0's value in all cases
(user and kernel thread).
The problem with this is trying to figure out which POR_EL0 to use. The
kthread could have been spawned ages ago and might not have a POR_EL0
which is very different from the current value of any of the threads in
the process right now.
There's also no great way for a kthread to reach out and grab an updated
value. It's all completely inherently racy.
quoted
Other approaches could also be considered (e.g. resetting POR_EL0 to
unrestricted when creating a kthread), see my reply on v4 [1].
I kinda think this is the only way to go. It's the only sensible,
predictable way. I _think_ it's what x86 will end up doing with PKRU,
but there's been enough churn there that I'd need to go double check
what happens in practice.
I agree.
Either way, it would be nice to get an io_uring test in here that
actually spawns kthreads:
tools/testing/selftests/mm/protection_keys.c
It would be good to update Documentation/core-api/protection-keys.rst
as well, since the example with read() raises more questions than it
answers!
Kevin, Joey -- I've got this series queued in arm64 as-is, so perhaps
you could send some patches on top so we can iron this out in time for
6.12? I'll also be at LPC next week if you're about.
Cheers,
Will
On Thu, Sep 12, 2024 at 11:50:18AM +0100, Will Deacon wrote:
Hi Dave,
On Wed, Sep 11, 2024 at 08:33:54AM -0700, Dave Hansen wrote:
quoted
On 9/11/24 08:01, Kevin Brodsky wrote:
quoted
On 22/08/2024 17:10, Joey Gouly wrote:
quoted
@@ -371,6 +382,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (system_supports_tpidr2()) p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);+ if (system_supports_poe())+ p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
Here we are only reloading POR_EL0's value if the target is a user
thread. However, as this series stands, POR_EL0 is also relevant to
kthreads, because any uaccess or GUP done from a kthread will also be
checked against POR_EL0. This is especially important in cases like the
io_uring kthread, which accesses the memory of the user process that
spawned it. To prevent such a kthread from inheriting a stale value of
POR_EL0, it seems that we should reload POR_EL0's value in all cases
(user and kernel thread).
The problem with this is trying to figure out which POR_EL0 to use. The
kthread could have been spawned ages ago and might not have a POR_EL0
which is very different from the current value of any of the threads in
the process right now.
There's also no great way for a kthread to reach out and grab an updated
value. It's all completely inherently racy.
quoted
Other approaches could also be considered (e.g. resetting POR_EL0 to
unrestricted when creating a kthread), see my reply on v4 [1].
I kinda think this is the only way to go. It's the only sensible,
predictable way. I _think_ it's what x86 will end up doing with PKRU,
but there's been enough churn there that I'd need to go double check
what happens in practice.
I agree.
quoted
Either way, it would be nice to get an io_uring test in here that
actually spawns kthreads:
tools/testing/selftests/mm/protection_keys.c
It would be good to update Documentation/core-api/protection-keys.rst
as well, since the example with read() raises more questions than it
answers!
Kevin, Joey -- I've got this series queued in arm64 as-is, so perhaps
you could send some patches on top so we can iron this out in time for
6.12? I'll also be at LPC next week if you're about.
I found the code in arch/x86 that does this, I must have missed this previously.
arch/x86/kernel/process.c: int copy_thread()
/* Kernel thread ? */
if (unlikely(p->flags & PF_KTHREAD)) {
p->thread.pkru = pkru_get_init_value();
memset(childregs, 0, sizeof(struct pt_regs));
kthread_frame_init(frame, args->fn, args->fn_arg);
return 0;
}
I can send a similar patch for arm64. I have no idea how to write io_uring
code, so looking for examples I can work with to get a test written. Might just
send the arm64 fix first, if that's fine?
Thanks,
Joey
From: Will Deacon <will@kernel.org> Date: 2024-09-13 15:15:08
On Thu, Sep 12, 2024 at 01:48:35PM +0100, Joey Gouly wrote:
On Thu, Sep 12, 2024 at 11:50:18AM +0100, Will Deacon wrote:
quoted
On Wed, Sep 11, 2024 at 08:33:54AM -0700, Dave Hansen wrote:
quoted
On 9/11/24 08:01, Kevin Brodsky wrote:
quoted
On 22/08/2024 17:10, Joey Gouly wrote:
quoted
@@ -371,6 +382,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (system_supports_tpidr2()) p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);+ if (system_supports_poe())+ p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
Here we are only reloading POR_EL0's value if the target is a user
thread. However, as this series stands, POR_EL0 is also relevant to
kthreads, because any uaccess or GUP done from a kthread will also be
checked against POR_EL0. This is especially important in cases like the
io_uring kthread, which accesses the memory of the user process that
spawned it. To prevent such a kthread from inheriting a stale value of
POR_EL0, it seems that we should reload POR_EL0's value in all cases
(user and kernel thread).
The problem with this is trying to figure out which POR_EL0 to use. The
kthread could have been spawned ages ago and might not have a POR_EL0
which is very different from the current value of any of the threads in
the process right now.
There's also no great way for a kthread to reach out and grab an updated
value. It's all completely inherently racy.
quoted
Other approaches could also be considered (e.g. resetting POR_EL0 to
unrestricted when creating a kthread), see my reply on v4 [1].
I kinda think this is the only way to go. It's the only sensible,
predictable way. I _think_ it's what x86 will end up doing with PKRU,
but there's been enough churn there that I'd need to go double check
what happens in practice.
I agree.
quoted
Either way, it would be nice to get an io_uring test in here that
actually spawns kthreads:
tools/testing/selftests/mm/protection_keys.c
It would be good to update Documentation/core-api/protection-keys.rst
as well, since the example with read() raises more questions than it
answers!
Kevin, Joey -- I've got this series queued in arm64 as-is, so perhaps
you could send some patches on top so we can iron this out in time for
6.12? I'll also be at LPC next week if you're about.
I found the code in arch/x86 that does this, I must have missed this previously.
arch/x86/kernel/process.c: int copy_thread()
/* Kernel thread ? */
if (unlikely(p->flags & PF_KTHREAD)) {
p->thread.pkru = pkru_get_init_value();
memset(childregs, 0, sizeof(struct pt_regs));
kthread_frame_init(frame, args->fn, args->fn_arg);
return 0;
}
I can send a similar patch for arm64. I have no idea how to write io_uring
code, so looking for examples I can work with to get a test written. Might just
send the arm64 fix first, if that's fine?
I think fix + documentation is what we need before 6.12, but you've still
got plenty of time after the merge window.
Cheers,
Will
@@ -371,6 +382,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) if (system_supports_tpidr2()) p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);+ if (system_supports_poe())+ p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
Here we are only reloading POR_EL0's value if the target is a user
thread. However, as this series stands, POR_EL0 is also relevant to
kthreads, because any uaccess or GUP done from a kthread will also be
checked against POR_EL0. This is especially important in cases like the
io_uring kthread, which accesses the memory of the user process that
spawned it. To prevent such a kthread from inheriting a stale value of
POR_EL0, it seems that we should reload POR_EL0's value in all cases
(user and kernel thread).
The problem with this is trying to figure out which POR_EL0 to use. The
kthread could have been spawned ages ago and might not have a POR_EL0
which is very different from the current value of any of the threads in
the process right now.
There's also no great way for a kthread to reach out and grab an updated
value. It's all completely inherently racy.
quoted
Other approaches could also be considered (e.g. resetting POR_EL0 to
unrestricted when creating a kthread), see my reply on v4 [1].
I kinda think this is the only way to go. It's the only sensible,
predictable way. I _think_ it's what x86 will end up doing with PKRU,
but there's been enough churn there that I'd need to go double check
what happens in practice.
that is also what powerpc does.
/* usage of kthread_use_mm() should inherit the
* AMR value of the operating address space. But, the AMR value is
* thread-specific and we inherit the address space and not thread
* access restrictions. Because of this ignore AMR value when accessing
* userspace via kernel thread.
*/
static __always_inline u64 current_thread_amr(void)
{
if (current->thread.regs)
return current->thread.regs->amr;
return default_amr;
}
-aneesh
@@ -16,9 +16,15 @@#include<asm/kvm_hyp.h>#include<asm/kvm_mmu.h>+staticinlineboolctxt_has_s1poe(structkvm_cpu_context*ctxt);+staticinlinevoid__sysreg_save_common_state(structkvm_cpu_context*ctxt){ctxt_sys_reg(ctxt,MDSCR_EL1)=read_sysreg(mdscr_el1);++// POR_EL0 can affect uaccess, so must be saved/restored early.+if(ctxt_has_s1poe(ctxt))+ctxt_sys_reg(ctxt,POR_EL0)=read_sysreg_s(SYS_POR_EL0);}staticinlinevoid__sysreg_save_user_state(structkvm_cpu_context*ctxt)
@@ -120,6 +140,10 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)staticinlinevoid__sysreg_restore_common_state(structkvm_cpu_context*ctxt){write_sysreg(ctxt_sys_reg(ctxt,MDSCR_EL1),mdscr_el1);++// POR_EL0 can affect uaccess, so must be saved/restored early.+if(ctxt_has_s1poe(ctxt))+write_sysreg_s(ctxt_sys_reg(ctxt,POR_EL0),SYS_POR_EL0);}staticinlinevoid__sysreg_restore_user_state(structkvm_cpu_context*ctxt)
From: Will Deacon <will@kernel.org> Date: 2024-08-23 13:48:19
On Thu, Aug 22, 2024 at 04:10:51PM +0100, Joey Gouly wrote:
To allow using newer instructions that current assemblers don't know about,
replace the `at` instruction with the underlying SYS instruction.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/kvm_asm.h | 3 ++-
arch/arm64/kvm/hyp/include/hyp/fault.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
Marc -- what would you like to do with this patch? I think the POE series
is really close now, so ideally I'd queue the lot on a branch in arm64
and you could pull the first ~10 patches into kvmarm if you need 'em.
Would what work for you, or did you have something else in mind (since
this one is also included in your series adding nv support for AT).
Cheers,
Will
From: Marc Zyngier <maz@kernel.org> Date: 2024-08-23 14:24:05
On Fri, 23 Aug 2024 14:48:11 +0100,
Will Deacon [off-list ref] wrote:
On Thu, Aug 22, 2024 at 04:10:51PM +0100, Joey Gouly wrote:
quoted
To allow using newer instructions that current assemblers don't know about,
replace the `at` instruction with the underlying SYS instruction.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/kvm_asm.h | 3 ++-
arch/arm64/kvm/hyp/include/hyp/fault.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
Marc -- what would you like to do with this patch? I think the POE series
is really close now, so ideally I'd queue the lot on a branch in arm64
and you could pull the first ~10 patches into kvmarm if you need 'em.
Would what work for you, or did you have something else in mind (since
this one is also included in your series adding nv support for AT).
Yup, that works for me. I can take that prefix as the base for the AT
series and drop my copy of this patch.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
From: Marc Zyngier <maz@kernel.org> Date: 2024-08-30 08:01:22
Hi Will,
On Fri, 23 Aug 2024 14:48:11 +0100,
Will Deacon [off-list ref] wrote:
On Thu, Aug 22, 2024 at 04:10:51PM +0100, Joey Gouly wrote:
quoted
To allow using newer instructions that current assemblers don't know about,
replace the `at` instruction with the underlying SYS instruction.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/kvm_asm.h | 3 ++-
arch/arm64/kvm/hyp/include/hyp/fault.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
Marc -- what would you like to do with this patch? I think the POE series
is really close now, so ideally I'd queue the lot on a branch in arm64
and you could pull the first ~10 patches into kvmarm if you need 'em.
Would what work for you, or did you have something else in mind (since
this one is also included in your series adding nv support for AT).
Is there any progress on this front? I am quite eager to queue the AT
series, but the dependency on this patch is preventing me to do so.
I can see there are outstanding questions on the POE series, so I was
wondering if we should consider reversing the dependency: I can create
a stable branch with this single patch, which you can pull as a prefix
of the POE series.
Please let me know what you prefer.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
From: Will Deacon <will@kernel.org> Date: 2024-08-30 09:05:31
Hey Marc,
On Fri, Aug 30, 2024 at 09:01:18AM +0100, Marc Zyngier wrote:
On Fri, 23 Aug 2024 14:48:11 +0100,
Will Deacon [off-list ref] wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:51PM +0100, Joey Gouly wrote:
quoted
To allow using newer instructions that current assemblers don't know about,
replace the `at` instruction with the underlying SYS instruction.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/kvm_asm.h | 3 ++-
arch/arm64/kvm/hyp/include/hyp/fault.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
Marc -- what would you like to do with this patch? I think the POE series
is really close now, so ideally I'd queue the lot on a branch in arm64
and you could pull the first ~10 patches into kvmarm if you need 'em.
Would what work for you, or did you have something else in mind (since
this one is also included in your series adding nv support for AT).
Is there any progress on this front? I am quite eager to queue the AT
series, but the dependency on this patch is preventing me to do so.
I can see there are outstanding questions on the POE series, so I was
wondering if we should consider reversing the dependency: I can create
a stable branch with this single patch, which you can pull as a prefix
of the POE series.
That sounds like a good idea. The uaccess discussion seems to have
stalled and I don't really want to merge the series without concluding
that.
So please go ahead with this single patch and I'll pull it in if things
start moving again.
Will
From: Marc Zyngier <maz@kernel.org> Date: 2024-08-30 11:59:01
On Fri, 30 Aug 2024 10:05:22 +0100,
Will Deacon [off-list ref] wrote:
Hey Marc,
On Fri, Aug 30, 2024 at 09:01:18AM +0100, Marc Zyngier wrote:
quoted
On Fri, 23 Aug 2024 14:48:11 +0100,
Will Deacon [off-list ref] wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:51PM +0100, Joey Gouly wrote:
quoted
To allow using newer instructions that current assemblers don't know about,
replace the `at` instruction with the underlying SYS instruction.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/kvm_asm.h | 3 ++-
arch/arm64/kvm/hyp/include/hyp/fault.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
Marc -- what would you like to do with this patch? I think the POE series
is really close now, so ideally I'd queue the lot on a branch in arm64
and you could pull the first ~10 patches into kvmarm if you need 'em.
Would what work for you, or did you have something else in mind (since
this one is also included in your series adding nv support for AT).
Is there any progress on this front? I am quite eager to queue the AT
series, but the dependency on this patch is preventing me to do so.
I can see there are outstanding questions on the POE series, so I was
wondering if we should consider reversing the dependency: I can create
a stable branch with this single patch, which you can pull as a prefix
of the POE series.
That sounds like a good idea. The uaccess discussion seems to have
stalled and I don't really want to merge the series without concluding
that.
So please go ahead with this single patch and I'll pull it in if things
start moving again.
FWIW (mainly for Marc): you seem to be missing the 'a/' and 'b/'
prefixes here, so my git would't accept the change when I tried to
apply locally for testing.
Will
FWIW (mainly for Marc): you seem to be missing the 'a/' and 'b/'
prefixes here, so my git would't accept the change when I tried to
apply locally for testing.
Seems like a spurious '--no-prefix' was added at patch formatting
time, That clashes with git-apply's default '-p1', which strips the
first component of the path.
There's probably a way to pass '-p0' to 'git am', but I don't feel
like trawling the git documentation by such a temperature...
M.
--
Without deviation from the norm, progress is not possible.
FWIW (mainly for Marc): you seem to be missing the 'a/' and 'b/'
prefixes here, so my git would't accept the change when I tried to
apply locally for testing.
Seems like a spurious '--no-prefix' was added at patch formatting
time, That clashes with git-apply's default '-p1', which strips the
first component of the path.
I had --no-prefix in my .git/config for diffs, but I didn't realise that also
applied to git format-patch, sorry for that. I have removed it now.
If you want me to resend v5, or something else, let me know.
There's probably a way to pass '-p0' to 'git am', but I don't feel
like trawling the git documentation by such a temperature...
M.
related to uaccess: Catalin is away, sure when he's back, so I'm hoping we can
resolve that when he's around.
Thanks,
Joey
FEAT_ATS1E1A introduces a new instruction: `at s1e1a`.
This is an address translation, without permission checks.
POE allows read permissions to be removed from S1 by the guest. This means
that an `at` instruction could fail, and not get the IPA.
Switch to using `at s1e1a` so that KVM can get the IPA regardless of S1
permissions.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/hyp/include/hyp/fault.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Add the missing sanitisation of ID_AA64MMFR3_EL1, making sure we
solely expose S1POE and TCRX (we currently don't support anything
else).
[joey: Took Marc's patch for S1PIE, and changed it for S1POE]
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
arch/arm64/kvm/sys_regs.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Expose a HWCAP and ID_AA64MMFR3_EL1_S1POE to userspace, so they can be used to
check if the CPU supports the feature.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Anshuman Khandual <redacted>
---
Documentation/arch/arm64/elf_hwcaps.rst | 2 ++
arch/arm64/include/asm/hwcap.h | 1 +
arch/arm64/include/uapi/asm/hwcap.h | 1 +
arch/arm64/kernel/cpufeature.c | 14 ++++++++++++++
arch/arm64/kernel/cpuinfo.c | 1 +
5 files changed, 19 insertions(+)
@@ -2348,6 +2350,14 @@ static void cpu_enable_mops(const struct arm64_cpu_capabilities *__unused)sysreg_clear_set(sctlr_el1,0,SCTLR_EL1_MSCEn);}+#ifdef CONFIG_ARM64_POE+staticvoidcpu_enable_poe(conststructarm64_cpu_capabilities*__unused)+{+sysreg_clear_set(REG_TCR2_EL1,0,TCR2_EL1x_E0POE);+sysreg_clear_set(CPACR_EL1,0,CPACR_ELx_E0POE);+}+#endif+/* Internal helper functions to match cpu capability type */staticboolcpucap_late_cpu_optional(conststructarm64_cpu_capabilities*cap)
Modify arch_calc_vm_prot_bits() and vm_get_page_prot() such that the pkey
value is set in the vm_flags and then into the pgprot value.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/mman.h | 10 +++++++++-
arch/arm64/mm/mmap.c | 11 +++++++++++
2 files changed, 20 insertions(+), 1 deletion(-)
When a PTE is modified, the POIndex must be masked off so that it can be modified.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Anshuman Khandual <redacted>
---
arch/arm64/include/asm/pgtable.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
If a memory fault occurs that is due to an overlay/pkey fault, report that to
userspace with a SEGV_PKUERR.
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
arch/arm64/include/asm/traps.h | 1 +
arch/arm64/kernel/traps.c | 6 ++++
arch/arm64/mm/fault.c | 55 +++++++++++++++++++++++++++++++++-
3 files changed, 61 insertions(+), 1 deletion(-)
@@ -486,6 +487,23 @@ static void do_bad_area(unsigned long far, unsigned long esr,}}+staticboolfault_from_pkey(unsignedlongesr,structvm_area_struct*vma,+unsignedintmm_flags)+{+unsignedlongiss2=ESR_ELx_ISS2(esr);++if(!system_supports_poe())+returnfalse;++if(iss2&ESR_ELx_Overlay)+returntrue;++return!arch_vma_access_permitted(vma,+mm_flags&FAULT_FLAG_WRITE,+mm_flags&FAULT_FLAG_INSTRUCTION,+false);+}+staticboolis_el0_instruction_abort(unsignedlongesr){returnESR_ELx_EC(esr)==ESR_ELx_EC_IABT_LOW;
@@ -511,6 +529,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,unsignedlongaddr=untagged_addr(far);structvm_area_struct*vma;intsi_code;+intpkey=-1;if(kprobe_page_fault(regs,esr))return0;
@@ -575,6 +594,16 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,count_vm_vma_lock_event(VMA_LOCK_SUCCESS);gotobad_area;}++if(fault_from_pkey(esr,vma,mm_flags)){+pkey=vma_pkey(vma);+vma_end_read(vma);+fault=0;+si_code=SEGV_PKUERR;+count_vm_vma_lock_event(VMA_LOCK_SUCCESS);+gotobad_area;+}+fault=handle_mm_fault(vma,addr,mm_flags|FAULT_FLAG_VMA_LOCK,regs);if(!(fault&(VM_FAULT_RETRY|VM_FAULT_COMPLETED)))vma_end_read(vma);
@@ -610,7 +639,16 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,gotobad_area;}+if(fault_from_pkey(esr,vma,mm_flags)){+pkey=vma_pkey(vma);+mmap_read_unlock(mm);+fault=0;+si_code=SEGV_PKUERR;+gotobad_area;+}+fault=handle_mm_fault(vma,addr,mm_flags,regs);+/* Quick path to respond to signals */if(fault_signal_pending(fault,regs)){if(!user_mode(regs))
@@ -669,8 +707,23 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,arm64_force_sig_mceerr(BUS_MCEERR_AR,far,lsb,inf->name);}else{+/*+*Thepkeyvaluethatwereturntouserspacecanbedifferent+*fromthepkeythatcausedthefault.+*+*1.T1:mprotect_key(foo,PAGE_SIZE,pkey=4);+*2.T1:setPOR_EL0todenyaccesstopkey=4,touches,page+*3.T1:faults...+*4.T2:mprotect_key(foo,PAGE_SIZE,pkey=5);+*5.T1:entersfaulthandler,takesmmap_lock,etc...+*6.T1:reacheshere,seesvma_pkey(vma)=5,whenwereally+*faultedonaptewithitspkey=4.+*//* Something tried to access memory that out of memory map */-arm64_force_sig_fault(SIGSEGV,si_code,far,inf->name);+if(si_code==SEGV_PKUERR)+arm64_force_sig_fault_pkey(far,inf->name,pkey);+else+arm64_force_sig_fault(SIGSEGV,si_code,far,inf->name);}return0;
From: Mark Brown <broonie@kernel.org> Date: 2024-08-29 17:55:16
On Thu, Aug 22, 2024 at 04:10:59PM +0100, Joey Gouly wrote:
+static bool fault_from_pkey(unsigned long esr, struct vm_area_struct *vma,
+ unsigned int mm_flags)
+{
+ unsigned long iss2 = ESR_ELx_ISS2(esr);
+
+ if (!system_supports_poe())
+ return false;
+
+ if (iss2 & ESR_ELx_Overlay)
+ return true;
Does this need an is_data_abort() && is_instruction_abort() check?
Overlay doesn't appear to be defined for all exception types and it
wasn't clear enough to me that the callers have done this check.
On Thu, Aug 29, 2024 at 06:55:07PM +0100, Mark Brown wrote:
On Thu, Aug 22, 2024 at 04:10:59PM +0100, Joey Gouly wrote:
quoted
+static bool fault_from_pkey(unsigned long esr, struct vm_area_struct *vma,
+ unsigned int mm_flags)
+{
+ unsigned long iss2 = ESR_ELx_ISS2(esr);
+
+ if (!system_supports_poe())
+ return false;
+
+ if (iss2 & ESR_ELx_Overlay)
+ return true;
Does this need an is_data_abort() && is_instruction_abort() check?
Overlay doesn't appear to be defined for all exception types and it
wasn't clear enough to me that the callers have done this check.
The only callers are in do_page_fault(), which should only be data or
instruction aborts. I talked with Catalin and he said it's fine to not check
again here.
I can add a permissions check though:
commit 033270f5a9462e998b4dee11fc91b43ac7929756
Author: Joey Gouly [off-list ref]
Date: Tue Sep 3 15:45:59 2024 +0100
fixup! arm64: handle PKEY/POE faults
Since the ESR_EL1 documentation says:
If a memory access generates a Data Abort for a Permission fault, then this field holds information
about the fault.
Thanks,
Joey
On Tue, Sep 03, 2024 at 03:50:46PM +0100, Joey Gouly wrote:
quoted hunk
On Thu, Aug 29, 2024 at 06:55:07PM +0100, Mark Brown wrote:
quoted
On Thu, Aug 22, 2024 at 04:10:59PM +0100, Joey Gouly wrote:
quoted
+static bool fault_from_pkey(unsigned long esr, struct vm_area_struct *vma,
+ unsigned int mm_flags)
+{
+ unsigned long iss2 = ESR_ELx_ISS2(esr);
+
+ if (!system_supports_poe())
+ return false;
+
+ if (iss2 & ESR_ELx_Overlay)
+ return true;
Does this need an is_data_abort() && is_instruction_abort() check?
Overlay doesn't appear to be defined for all exception types and it
wasn't clear enough to me that the callers have done this check.
The only callers are in do_page_fault(), which should only be data or
instruction aborts. I talked with Catalin and he said it's fine to not check
again here.
I can add a permissions check though:
commit 033270f5a9462e998b4dee11fc91b43ac7929756
Author: Joey Gouly [off-list ref]
Date: Tue Sep 3 15:45:59 2024 +0100
fixup! arm64: handle PKEY/POE faults
Since the ESR_EL1 documentation says:
If a memory access generates a Data Abort for a Permission fault, then this field holds information
about the fault.
Sorry, I was a bit too eager with that patch. The previous patch was bailing
out before the vma-backed checks could take place.
It should be:
commit 7b67b149f2f492e907b27521c95639f4ea208221 (HEAD -> permission_overlay_v6)
Author: Joey Gouly [off-list ref]
Date: Tue Sep 3 15:45:59 2024 +0100
fixup! arm64: handle PKEY/POE faults