arm64: kernel: Add support for User Access Override
v2 cover arm64: kernel: Add support for User Access Override
This series adds support for User Access Override (UAO; part of the ARMv8.2 Extensions[0]). When enabled, this causes the get_user() accessors to use the unprivileged load/store instructions. When addr_limit is set to KERNEL_DS, we set the override bit allowing privileged access.
Because the unprivileged instructions don't trip PAN, the last patch changes which 'alternative' values are swapped in, allowing PAN to be left enabled during get_user() and friends.
This series can be retrieved from:git://linux-arm.org/linux-jm.git -b uao/v2
Comments welcome,
James
Since v1:
* Moved id register definition into the boiler-plate patch.
* Moved cpu_enable_uao() into mm/fault.c
[0] https://community.arm.com/groups/processors/blog/2016/01/05/armv8-a-architecture-evolution
[v1] https://lwn.net/Articles/674445/
James Morse (5):
arm64: cpufeature: Change read_cpuid() to use sysreg's mrs_s macro
arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
arm64: kernel: Add support for User Access Override
arm64: cpufeature: Test 'matches' pointer to find the end of the list
arm64: kernel: Don't toggle PAN on systems with UAO
arch/arm64/Kconfig | 21 +++++++ arch/arm64/include/asm/alternative.h | 72 +++++++++++++++++++++++ arch/arm64/include/asm/cpu.h | 1 + arch/arm64/include/asm/cpufeature.h | 6 +- arch/arm64/include/asm/cputype.h | 20 ++++--- arch/arm64/include/asm/processor.h | 1 + arch/arm64/include/asm/sysreg.h | 7 +++ arch/arm64/include/asm/thread_info.h | 6 ++ arch/arm64/include/asm/uaccess.h | 52 +++++++++++------ arch/arm64/include/uapi/asm/ptrace.h | 1 + arch/arm64/kernel/cpufeature.c | 107 +++++++++++++++++++++++------------ arch/arm64/kernel/cpuinfo.c | 55 +++++++++--------- arch/arm64/kernel/process.c | 20 +++++++ arch/arm64/lib/clear_user.S | 12 ++-- arch/arm64/lib/copy_from_user.S | 12 ++-- arch/arm64/lib/copy_in_user.S | 20 +++---- arch/arm64/lib/copy_to_user.S | 12 ++-- arch/arm64/mm/context.c | 2 +- arch/arm64/mm/fault.c | 34 +++++++++-- 19 files changed, 337 insertions(+), 124 deletions(-)
Signature · 1 line
2.6.2
PATCH v2 1/5 arm64: cpufeature: Change read_cpuid() to use sysreg's mrs_s macro
Older assemblers may not have support for newer feature registers. To get round this, sysreg.h provides a 'mrs_s' macro that takes a register encoding and generates the raw instruction.
Change read_cpuid() to use mrs_s in all cases so that new registers don't have to be a special case. Including sysreg.h means we need to move the include and definition of read_cpuid() after the #ifndef __ASSEMBLY__ to avoid syntax errors in vmlinux.lds.
--- arch/arm64/include/asm/cpufeature.h | 2 +- arch/arm64/include/asm/cputype.h | 20 +++++++------ arch/arm64/kernel/cpufeature.c | 58 ++++++++++++++++++------------------- arch/arm64/kernel/cpuinfo.c | 54 +++++++++++++++++----------------- arch/arm64/mm/context.c | 2 +- 5 files changed, 69 insertions(+), 67 deletions(-)
arch/arm64/include/asm/cpufeature.h +1 -1
@@ -176,7 +176,7 @@ u64 read_system_reg(u32 id);static inline bool cpu_supports_mixed_endian_el0 (void ) { -return id_aa64mmfr0_mixed_endian_el0 (read_cpuid (ID_AA64MMFR0_EL1 )); +return id_aa64mmfr0_mixed_endian_el0 (read_cpuid (SYS_ID_AA64MMFR0_EL1 )); }static inline bool system_supports_mixed_endian_el0 (void )
arch/arm64/include/asm/cputype.h +11 -9
@@ -32,12 +32,6 @@#define MPIDR_AFFINITY_LEVEL (mpidr, level) \ ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK) -#define read_cpuid (reg) ({ \ - u64 __val; \ - asm("mrs %0, " #reg : "=r" (__val)); \ - __val; \ -}) -#define MIDR_REVISION_MASK 0xf#define MIDR_REVISION (midr) ((midr) & MIDR_REVISION_MASK)#define MIDR_PARTNUM_SHIFT 4
@@ -77,6 +71,14 @@#ifndef __ASSEMBLY__ +#include <asm/sysreg.h> + +#define read_cpuid (reg) ({ \ + u64 __val; \ + asm("mrs_s %0, " __stringify(reg) : "=r" (__val)); \ + __val; \ +}) +/* * The CPU ID never changes at run time, so we might as well tell the * compiler that it's constant. Use this function to read the CPU ID
@@ -84,12 +86,12 @@*/ static inline u32 __attribute_const__ read_cpuid_id(void) { -return read_cpuid (MIDR_EL1 ); +return read_cpuid (SYS_MIDR_EL1 ); } static inline u64 __attribute_const__ read_cpuid_mpidr(void) { -return read_cpuid (MPIDR_EL1 ); +return read_cpuid (SYS_MPIDR_EL1 ); } static inline unsigned int __attribute_const__ read_cpuid_implementor(void)
@@ -104,7 +106,7 @@ static inline unsigned int __attribute_const__ read_cpuid_part_number(void) static inline u32 __attribute_const__ read_cpuid_cachetype(void) { -return read_cpuid (CTR_EL0 ); +return read_cpuid (SYS_CTR_EL0 ); }#endif /* __ASSEMBLY__ */
arch/arm64/kernel/cpufeature.c +29 -29
64 lines not on this page · raw message
arch/arm64/kernel/cpuinfo.c +27 -27
62 lines not on this page · raw message
arch/arm64/mm/context.c +1 -1
@@ -187,7 +187,7 @@ switch_mm_fastpath:static int asids_init (void ) { -int fld =cpuid_feature_extract_field (read_cpuid (ID_AA64MMFR0_EL1 ),4 ); +int fld =cpuid_feature_extract_field (read_cpuid (SYS_ID_AA64MMFR0_EL1 ),4 );switch (fld) {default :
Signature · 1 line
2.6.2
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
In reply to James MorseARMv8.2 adds a new feature register id_aa64mmfr2. This patch adds the cpu feature boiler plate used by the actual features in later patches.
--- arch/arm64/include/asm/cpu.h | 1 + arch/arm64/include/asm/sysreg.h | 4 ++++ arch/arm64/kernel/cpufeature.c | 10 ++++++++++ arch/arm64/kernel/cpuinfo.c | 1 + 4 files changed, 16 insertions(+)
arch/arm64/include/asm/cpu.h +1 -0
@@ -36,6 +36,7 @@ struct cpuinfo_arm64 {u64 reg_id_aa64isar1;u64 reg_id_aa64mmfr0;u64 reg_id_aa64mmfr1; +u64 reg_id_aa64mmfr2;u64 reg_id_aa64pfr0;u64 reg_id_aa64pfr1;
arch/arm64/include/asm/sysreg.h +4 -0
@@ -72,6 +72,7 @@#define SYS_ID_AA64MMFR0_EL1 sys_reg(3, 0, 0, 7, 0)#define SYS_ID_AA64MMFR1_EL1 sys_reg(3, 0, 0, 7, 1) +#define SYS_ID_AA64MMFR2_EL1 sys_reg(3, 0, 0, 7, 2)#define SYS_CNTFRQ_EL0 sys_reg(3, 3, 14, 0, 0)#define SYS_CTR_EL0 sys_reg(3, 3, 0, 0, 1)
@@ -137,6 +138,9 @@#define ID_AA64MMFR1_VMIDBITS_SHIFT 4#define ID_AA64MMFR1_HADBS_SHIFT 0 +/* id_aa64mmfr2 */ +#define ID_AA64MMFR2_UAO_SHIFT 4 +/* id_aa64dfr0 */ #define ID_AA64DFR0_CTX_CMPS_SHIFT 28#define ID_AA64DFR0_WRPS_SHIFT 20
arch/arm64/kernel/cpufeature.c +10 -0
@@ -123,6 +123,11 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = { ARM64_FTR_END, }; +static struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { +ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,ID_AA64MMFR2_UAO_SHIFT ,4 ,0 ), +ARM64_FTR_END , +}; +static struct arm64_ftr_bits ftr_ctr[] = {U_ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,31 ,1 ,1 ),/* RAO */ ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,28 ,3 ,0 ),
@@ -284,6 +289,7 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {/* Op1 = 0, CRn = 0, CRm = 7 */ ARM64_FTR_REG (SYS_ID_AA64MMFR0_EL1 , ftr_id_aa64mmfr0),ARM64_FTR_REG (SYS_ID_AA64MMFR1_EL1 , ftr_id_aa64mmfr1), +ARM64_FTR_REG (SYS_ID_AA64MMFR2_EL1 , ftr_id_aa64mmfr2),/* Op1 = 3, CRn = 0, CRm = 0 */ ARM64_FTR_REG(SYS_CTR_EL0, ftr_ctr),
@@ -408,6 +414,7 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)init_cpu_ftr_reg (SYS_ID_AA64ISAR1_EL1 , info->reg_id_aa64isar1);init_cpu_ftr_reg (SYS_ID_AA64MMFR0_EL1 , info->reg_id_aa64mmfr0);init_cpu_ftr_reg (SYS_ID_AA64MMFR1_EL1 , info->reg_id_aa64mmfr1); +init_cpu_ftr_reg (SYS_ID_AA64MMFR2_EL1 , info->reg_id_aa64mmfr2);init_cpu_ftr_reg (SYS_ID_AA64PFR0_EL1 , info->reg_id_aa64pfr0);init_cpu_ftr_reg (SYS_ID_AA64PFR1_EL1 , info->reg_id_aa64pfr1);init_cpu_ftr_reg (SYS_ID_DFR0_EL1 , info->reg_id_dfr0);
@@ -517,6 +524,8 @@ void update_cpu_features(int cpu, info->reg_id_aa64mmfr0, boot->reg_id_aa64mmfr0); taint |=check_update_ftr_reg (SYS_ID_AA64MMFR1_EL1 , cpu, info->reg_id_aa64mmfr1, boot->reg_id_aa64mmfr1); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR2_EL1 , cpu, + info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2);/* * EL3 is not our concern.
@@ -814,6 +823,7 @@ static u64 __raw_read_system_reg(u32 sys_id)case SYS_ID_AA64DFR1_EL1 :return read_cpuid (SYS_ID_AA64DFR0_EL1 );case SYS_ID_AA64MMFR0_EL1 :return read_cpuid (SYS_ID_AA64MMFR0_EL1 );case SYS_ID_AA64MMFR1_EL1 :return read_cpuid (SYS_ID_AA64MMFR1_EL1 ); +case SYS_ID_AA64MMFR2_EL1 :return read_cpuid (SYS_ID_AA64MMFR2_EL1 );case SYS_ID_AA64ISAR0_EL1 :return read_cpuid (SYS_ID_AA64ISAR0_EL1 );case SYS_ID_AA64ISAR1_EL1 :return read_cpuid (SYS_ID_AA64ISAR1_EL1 );
arch/arm64/kernel/cpuinfo.c +1 -0
@@ -210,6 +210,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64isar1 =read_cpuid (SYS_ID_AA64ISAR1_EL1 ); info->reg_id_aa64mmfr0 =read_cpuid (SYS_ID_AA64MMFR0_EL1 ); info->reg_id_aa64mmfr1 =read_cpuid (SYS_ID_AA64MMFR1_EL1 ); + info->reg_id_aa64mmfr2 =read_cpuid (SYS_ID_AA64MMFR2_EL1 ); info->reg_id_aa64pfr0 =read_cpuid (SYS_ID_AA64PFR0_EL1 ); info->reg_id_aa64pfr1 =read_cpuid (SYS_ID_AA64PFR1_EL1 );
Signature · 1 line
2.6.2
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Hi James,
This change breaks booting linux-next for me.
Quoted 100 lines · James Morse
ARMv8.2 adds a new feature register
id_aa64mmfr2. This patch adds the cpu feature boiler plate used by the actual features in later patches.Signed-off-by James Morse <[email protected]>Reviewed-by Suzuki K Poulose <[email protected]>--- arch/arm64/include/asm/cpu.h | 1 + arch/arm64/include/asm/sysreg.h | 4 ++++ arch/arm64/kernel/cpufeature.c | 10 ++++++++++ arch/arm64/kernel/cpuinfo.c | 1 + 4 files changed, 16 insertions(+)arch/arm64/include/asm/cpu.h +1
-0@@ -36,6 +36,7 @@ struct cpuinfo_arm64 {u64 reg_id_aa64isar1;u64 reg_id_aa64mmfr0;u64 reg_id_aa64mmfr1; +u64 reg_id_aa64mmfr2;u64 reg_id_aa64pfr0;u64 reg_id_aa64pfr1;arch/arm64/include/asm/sysreg.h +4
-0@@ -72,6 +72,7 @@#define SYS_ID_AA64MMFR0_EL1 sys_reg(3, 0, 0, 7, 0)#define SYS_ID_AA64MMFR1_EL1 sys_reg(3, 0, 0, 7, 1) +#define SYS_ID_AA64MMFR2_EL1 sys_reg(3, 0, 0, 7, 2)#define SYS_CNTFRQ_EL0 sys_reg(3, 3, 14, 0, 0)#define SYS_CTR_EL0 sys_reg(3, 3, 0, 0, 1)@@ -137,6 +138,9 @@#define ID_AA64MMFR1_VMIDBITS_SHIFT 4#define ID_AA64MMFR1_HADBS_SHIFT 0 +/* id_aa64mmfr2 */ +#define ID_AA64MMFR2_UAO_SHIFT 4 +/* id_aa64dfr0 */ #define ID_AA64DFR0_CTX_CMPS_SHIFT 28#define ID_AA64DFR0_WRPS_SHIFT 20arch/arm64/kernel/cpufeature.c +10
-0@@ -123,6 +123,11 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = { ARM64_FTR_END, }; +static struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { +ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,ID_AA64MMFR2_UAO_SHIFT ,4 ,0 ), +ARM64_FTR_END , +}; +static struct arm64_ftr_bits ftr_ctr[] = {U_ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,31 ,1 ,1 ),/* RAO */ ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,28 ,3 ,0 ),@@ -284,6 +289,7 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {/* Op1 = 0, CRn = 0, CRm = 7 */ ARM64_FTR_REG (SYS_ID_AA64MMFR0_EL1 , ftr_id_aa64mmfr0),ARM64_FTR_REG (SYS_ID_AA64MMFR1_EL1 , ftr_id_aa64mmfr1), +ARM64_FTR_REG (SYS_ID_AA64MMFR2_EL1 , ftr_id_aa64mmfr2),/* Op1 = 3, CRn = 0, CRm = 0 */ ARM64_FTR_REG(SYS_CTR_EL0, ftr_ctr),@@ -408,6 +414,7 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)init_cpu_ftr_reg (SYS_ID_AA64ISAR1_EL1 , info->reg_id_aa64isar1);init_cpu_ftr_reg (SYS_ID_AA64MMFR0_EL1 , info->reg_id_aa64mmfr0);init_cpu_ftr_reg (SYS_ID_AA64MMFR1_EL1 , info->reg_id_aa64mmfr1); +init_cpu_ftr_reg (SYS_ID_AA64MMFR2_EL1 , info->reg_id_aa64mmfr2);init_cpu_ftr_reg (SYS_ID_AA64PFR0_EL1 , info->reg_id_aa64pfr0);init_cpu_ftr_reg (SYS_ID_AA64PFR1_EL1 , info->reg_id_aa64pfr1);init_cpu_ftr_reg (SYS_ID_DFR0_EL1 , info->reg_id_dfr0);@@ -517,6 +524,8 @@ void update_cpu_features(int cpu, info->reg_id_aa64mmfr0, boot->reg_id_aa64mmfr0); taint |=check_update_ftr_reg (SYS_ID_AA64MMFR1_EL1 , cpu, info->reg_id_aa64mmfr1, boot->reg_id_aa64mmfr1); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR2_EL1 , cpu, + info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2);/* * EL3 is not our concern. @@ -814,6 +823,7 @@ static u64 __raw_read_system_reg(u32 sys_id)case SYS_ID_AA64DFR1_EL1 :return read_cpuid (SYS_ID_AA64DFR0_EL1 );case SYS_ID_AA64MMFR0_EL1 :return read_cpuid (SYS_ID_AA64MMFR0_EL1 );case SYS_ID_AA64MMFR1_EL1 :return read_cpuid (SYS_ID_AA64MMFR1_EL1 ); +case SYS_ID_AA64MMFR2_EL1 :return read_cpuid (SYS_ID_AA64MMFR2_EL1 );case SYS_ID_AA64ISAR0_EL1 :return read_cpuid (SYS_ID_AA64ISAR0_EL1 );case SYS_ID_AA64ISAR1_EL1 :return read_cpuid (SYS_ID_AA64ISAR1_EL1 );arch/arm64/kernel/cpuinfo.c +0
-0
arch/arm64/kernel/cpuinfo.c static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) · line 215
Quoted 6 lines
arch/arm64/kernel/cpuinfo.c +1
-0@@ -210,6 +210,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64isar1 =read_cpuid (SYS_ID_AA64ISAR1_EL1 ); info->reg_id_aa64mmfr0 =read_cpuid (SYS_ID_AA64MMFR0_EL1 ); info->reg_id_aa64mmfr1 =read_cpuid (SYS_ID_AA64MMFR1_EL1 ); + info->reg_id_aa64mmfr2 =read_cpuid (SYS_ID_AA64MMFR2_EL1 ); info->reg_id_aa64pfr0 =read_cpuid (SYS_ID_AA64PFR0_EL1 ); info->reg_id_aa64pfr1 =read_cpuid (SYS_ID_AA64PFR1_EL1 );
swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
ffffff800808d730: d5380742 mrs x2, s3_0_c0_c7_2
Thanks,
Christopher Covington
Signature · 3 lines
Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Quoted 122 lines · Christopher Covington
Hi James,
This change breaks booting linux-next for me.
Quoted 100 lines · James Morse
ARMv8.2 adds a new feature register
id_aa64mmfr2. This patch adds the cpu feature boiler plate used by the actual features in later patches.Signed-off-by James Morse <[email protected]>Reviewed-by Suzuki K Poulose <[email protected]>--- arch/arm64/include/asm/cpu.h | 1 + arch/arm64/include/asm/sysreg.h | 4 ++++ arch/arm64/kernel/cpufeature.c | 10 ++++++++++ arch/arm64/kernel/cpuinfo.c | 1 + 4 files changed, 16 insertions(+)arch/arm64/include/asm/cpu.h +1
-0@@ -36,6 +36,7 @@ struct cpuinfo_arm64 {u64 reg_id_aa64isar1;u64 reg_id_aa64mmfr0;u64 reg_id_aa64mmfr1; +u64 reg_id_aa64mmfr2;u64 reg_id_aa64pfr0;u64 reg_id_aa64pfr1;arch/arm64/include/asm/sysreg.h +4
-0@@ -72,6 +72,7 @@#define SYS_ID_AA64MMFR0_EL1 sys_reg(3, 0, 0, 7, 0)#define SYS_ID_AA64MMFR1_EL1 sys_reg(3, 0, 0, 7, 1) +#define SYS_ID_AA64MMFR2_EL1 sys_reg(3, 0, 0, 7, 2)#define SYS_CNTFRQ_EL0 sys_reg(3, 3, 14, 0, 0)#define SYS_CTR_EL0 sys_reg(3, 3, 0, 0, 1)@@ -137,6 +138,9 @@#define ID_AA64MMFR1_VMIDBITS_SHIFT 4#define ID_AA64MMFR1_HADBS_SHIFT 0 +/* id_aa64mmfr2 */ +#define ID_AA64MMFR2_UAO_SHIFT 4 +/* id_aa64dfr0 */ #define ID_AA64DFR0_CTX_CMPS_SHIFT 28#define ID_AA64DFR0_WRPS_SHIFT 20arch/arm64/kernel/cpufeature.c +10
-0@@ -123,6 +123,11 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = { ARM64_FTR_END, }; +static struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { +ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,ID_AA64MMFR2_UAO_SHIFT ,4 ,0 ), +ARM64_FTR_END , +}; +static struct arm64_ftr_bits ftr_ctr[] = {U_ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,31 ,1 ,1 ),/* RAO */ ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,28 ,3 ,0 ),@@ -284,6 +289,7 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {/* Op1 = 0, CRn = 0, CRm = 7 */ ARM64_FTR_REG (SYS_ID_AA64MMFR0_EL1 , ftr_id_aa64mmfr0),ARM64_FTR_REG (SYS_ID_AA64MMFR1_EL1 , ftr_id_aa64mmfr1), +ARM64_FTR_REG (SYS_ID_AA64MMFR2_EL1 , ftr_id_aa64mmfr2),/* Op1 = 3, CRn = 0, CRm = 0 */ ARM64_FTR_REG(SYS_CTR_EL0, ftr_ctr),@@ -408,6 +414,7 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)init_cpu_ftr_reg (SYS_ID_AA64ISAR1_EL1 , info->reg_id_aa64isar1);init_cpu_ftr_reg (SYS_ID_AA64MMFR0_EL1 , info->reg_id_aa64mmfr0);init_cpu_ftr_reg (SYS_ID_AA64MMFR1_EL1 , info->reg_id_aa64mmfr1); +init_cpu_ftr_reg (SYS_ID_AA64MMFR2_EL1 , info->reg_id_aa64mmfr2);init_cpu_ftr_reg (SYS_ID_AA64PFR0_EL1 , info->reg_id_aa64pfr0);init_cpu_ftr_reg (SYS_ID_AA64PFR1_EL1 , info->reg_id_aa64pfr1);init_cpu_ftr_reg (SYS_ID_DFR0_EL1 , info->reg_id_dfr0);@@ -517,6 +524,8 @@ void update_cpu_features(int cpu, info->reg_id_aa64mmfr0, boot->reg_id_aa64mmfr0); taint |=check_update_ftr_reg (SYS_ID_AA64MMFR1_EL1 , cpu, info->reg_id_aa64mmfr1, boot->reg_id_aa64mmfr1); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR2_EL1 , cpu, + info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2);/* * EL3 is not our concern. @@ -814,6 +823,7 @@ static u64 __raw_read_system_reg(u32 sys_id)case SYS_ID_AA64DFR1_EL1 :return read_cpuid (SYS_ID_AA64DFR0_EL1 );case SYS_ID_AA64MMFR0_EL1 :return read_cpuid (SYS_ID_AA64MMFR0_EL1 );case SYS_ID_AA64MMFR1_EL1 :return read_cpuid (SYS_ID_AA64MMFR1_EL1 ); +case SYS_ID_AA64MMFR2_EL1 :return read_cpuid (SYS_ID_AA64MMFR2_EL1 );case SYS_ID_AA64ISAR0_EL1 :return read_cpuid (SYS_ID_AA64ISAR0_EL1 );case SYS_ID_AA64ISAR1_EL1 :return read_cpuid (SYS_ID_AA64ISAR1_EL1 );arch/arm64/kernel/cpuinfo.c +0
-0arch/arm64/kernel/cpuinfo.c static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) · line 215
Quoted 6 lines
arch/arm64/kernel/cpuinfo.c +1
-0@@ -210,6 +210,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64isar1 =read_cpuid (SYS_ID_AA64ISAR1_EL1 ); info->reg_id_aa64mmfr0 =read_cpuid (SYS_ID_AA64MMFR0_EL1 ); info->reg_id_aa64mmfr1 =read_cpuid (SYS_ID_AA64MMFR1_EL1 ); + info->reg_id_aa64mmfr2 =read_cpuid (SYS_ID_AA64MMFR2_EL1 ); info->reg_id_aa64pfr0 =read_cpuid (SYS_ID_AA64PFR0_EL1 ); info->reg_id_aa64pfr1 =read_cpuid (SYS_ID_AA64PFR1_EL1 );swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
Quoted 1 line
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2
Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically a reserved encoding, rather than an unallocated one, so it should read as zero, not undef. What are you running on?
Robin.
Quoted 4 lines
Thanks,
Christopher Covington
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Quoted 23 lines · Robin Murphy
Quoted 21 lines · Christopher Covington
arch/arm64/kernel/cpuinfo.c static void __cpuinfo_store_cpu(struct · line 216
Quoted 10 lines
arch/arm64/kernel/cpuinfo.c +1
-0@@ -210,6 +210,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64isar1 = read_cpuid(SYS_ID_AA64ISAR1_EL1); info->reg_id_aa64mmfr0 =read_cpuid (SYS_ID_AA64MMFR0_EL1 ); info->reg_id_aa64mmfr1 =read_cpuid (SYS_ID_AA64MMFR1_EL1 ); + info->reg_id_aa64mmfr2 =read_cpuid (SYS_ID_AA64MMFR2_EL1 ); info->reg_id_aa64pfr0 =read_cpuid (SYS_ID_AA64PFR0_EL1 ); info->reg_id_aa64pfr1 =read_cpuid (SYS_ID_AA64PFR1_EL1 );swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
Quoted 5 lines
Quoted 1 line
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically a reserved encoding, rather than an unallocated one, so it should read as zero, not undef. What are you running on?
QDF2432. I'll investigate.
Cov
Signature · 3 lines
Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Quoted 24 lines · Christopher Covington
Quoted 23 lines · Robin Murphy
Quoted 21 lines · Christopher Covington
arch/arm64/kernel/cpuinfo.c static void __cpuinfo_store_cpu(struct · line 216
Quoted 10 lines
arch/arm64/kernel/cpuinfo.c +1
-0@@ -210,6 +210,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64isar1 = read_cpuid(SYS_ID_AA64ISAR1_EL1); info->reg_id_aa64mmfr0 =read_cpuid (SYS_ID_AA64MMFR0_EL1 ); info->reg_id_aa64mmfr1 =read_cpuid (SYS_ID_AA64MMFR1_EL1 ); + info->reg_id_aa64mmfr2 =read_cpuid (SYS_ID_AA64MMFR2_EL1 ); info->reg_id_aa64pfr0 =read_cpuid (SYS_ID_AA64PFR0_EL1 ); info->reg_id_aa64pfr1 =read_cpuid (SYS_ID_AA64PFR1_EL1 );swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically a reserved encoding, rather than an unallocated one, so it should read as zero, not undef. What are you running on?
QDF2432. I'll investigate.
FWIW, I think the same issue was reported on qemu, so we may want to handle this in the kernel anyway. We could either use alternatives on the register read or handle the undef.
Will
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Quoted 21 lines · Will Deacon
Quoted 20 lines · Christopher Covington
Quoted 18 lines · Robin Murphy
Quoted 11 lines · Christopher Covington
swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically a reserved encoding, rather than an unallocated one, so it should read as zero, not undef. What are you running on?
Quoted 5 lines
Quoted 1 line
QDF2432. I'll investigate.
FWIW, I think the same issue was reported on qemu, so we may want to handle this in the kernel anyway. We could either use alternatives on the register read or handle the undef.
We could use parts of the mrs emulation patch [1] to emulate only the above for kernel mode.
[1] https://lkml.org/lkml/2015/10/13/641
Cheers
Suzuki
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Hi Suzuki,
Quoted 20 lines · Suzuki K. Poulose
Quoted 19 lines · Will Deacon
Quoted 18 lines · Christopher Covington
Quoted 16 lines · Robin Murphy
Quoted 11 lines · Christopher Covington
swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically a reserved encoding, rather than an unallocated one, so it should read as zero, not undef. What are you running on?
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
QDF2432. I'll investigate.
FWIW, I think the same issue was reported on qemu, so we may want to handle this in the kernel anyway. We could either use alternatives on the register read or handle the undef.
Alternatives would let us handle the QDF2432 case, but detecting the version of qemu doesn't sound right. I will put together a series to handle the undef.
Quoted 2 lines
We could use parts of the mrs emulation patch [1] to emulate only the above for kernel mode.
Thanks, I will see what I can poach!
James
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Quoted 23 lines · James Morse
Hi Suzuki,
Quoted 20 lines · Suzuki K. Poulose
Quoted 19 lines · Will Deacon
Quoted 18 lines · Christopher Covington
Quoted 16 lines · Robin Murphy
Quoted 11 lines · Christopher Covington
swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically a reserved encoding, rather than an unallocated one, so it should read as zero, not undef. What are you running on?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
QDF2432. I'll investigate.
FWIW, I think the same issue was reported on qemu, so we may want to handle this in the kernel anyway. We could either use alternatives on the register read or handle the undef.
Alternatives would let us handle the QDF2432 case, but detecting the version of qemu doesn't sound right. I will put together a series to handle the undef.
More generally, it would leave future potential for having to work out whether to apply alternatives to read an ID register to work out whether we need to apply alternatives for some feature within. I also can't even begin to fathom how such a thing would interact with the heterogeneous feature sanity-checking.
I think trapping the undef is the only viable approach.
Robin.
Quoted 14 lines
Quoted 2 lines
We could use parts of the mrs emulation patch [1] to emulate only the above for kernel mode.
Thanks, I will see what I can poach!
James
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland
reviewerPATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
In reply to Christopher CovingtonQuoted 24 lines · Christopher Covington
Quoted 23 lines · Robin Murphy
Quoted 21 lines · Christopher Covington
arch/arm64/kernel/cpuinfo.c static void __cpuinfo_store_cpu(struct · line 216
Quoted 10 lines
arch/arm64/kernel/cpuinfo.c +1
-0@@ -210,6 +210,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64isar1 = read_cpuid(SYS_ID_AA64ISAR1_EL1); info->reg_id_aa64mmfr0 =read_cpuid (SYS_ID_AA64MMFR0_EL1 ); info->reg_id_aa64mmfr1 =read_cpuid (SYS_ID_AA64MMFR1_EL1 ); + info->reg_id_aa64mmfr2 =read_cpuid (SYS_ID_AA64MMFR2_EL1 ); info->reg_id_aa64pfr0 =read_cpuid (SYS_ID_AA64PFR0_EL1 ); info->reg_id_aa64pfr1 =read_cpuid (SYS_ID_AA64PFR1_EL1 );swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti: ffffff8008b20000 PC is at __cpuinfo_store_cpu+0x68/0x198 LR is at cpuinfo_store_boot_cpu+0x28/0x50
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically a reserved encoding, rather than an unallocated one, so it should read as zero, not undef. What are you running on?
QDF2432. I'll investigate.
Just to check, I take it you're running natively, and not under a hypervisor?
Mark.
PATCH v2 2/5 arm64: add ARMv8.2 id_aa64mmfr2 boiler plate
Quoted 30 lines · Mark Rutland
Quoted 13 lines · Christopher Covington
Quoted 12 lines · Robin Murphy
Quoted 10 lines · Christopher Covington
Quoted 10 lines
arch/arm64/kernel/cpuinfo.c +1
-0@@ -210,6 +210,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_aa64isar1 = read_cpuid(SYS_ID_AA64ISAR1_EL1); info->reg_id_aa64mmfr0 =read_cpuid (SYS_ID_AA64MMFR0_EL1 ); info->reg_id_aa64mmfr1 =read_cpuid (SYS_ID_AA64MMFR1_EL1 ); + info->reg_id_aa64mmfr2 =read_cpuid (SYS_ID_AA64MMFR2_EL1 ); info->reg_id_aa64pfr0 =read_cpuid (SYS_ID_AA64PFR0_EL1 ); info->reg_id_aa64pfr1 =read_cpuid (SYS_ID_AA64PFR1_EL1 );Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
swapper[0]: undefined instruction: pc=ffffff800808d730
Code: d5380702 d5380721 f9017c02 f9018001 (d5380742) Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc6-next-20160303 #1 Hardware name: (null) (DT) task: ffffff8008b2d980 ti: ffffff8008b20000 task.ti:
ffffff8008b20000Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
PC is at
__cpuinfo_store_cpu+0x68/0x198
LR is atcpuinfo_store_boot_cpu+0x28/0x50
ffffff800808d730:d5380742mrs x2,s3_0_c0_c7_2Hmm, per table C5-6 in the ARMv8 ARM (issue i), that's specifically
a
Quoted 8 lines
Quoted 1 line
Quoted 1 line
reserved encoding, rather than an unallocated one, so it should
read as
Quoted 3 lines
Quoted 1 line
zero, not undef. What are you running on?
QDF2432. I'll investigate.
Just to check, I take it you're running natively, and not under a hypervisor?
Yes this was running natively. I haven't yet been able to confirm things, but I'll let you know when I do.
Cov
Signature · 5 lines
Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project Sent from my Snapdragon powered Android device with K-9 Mail. Please excuse my brevity.
PATCH v2 3/5 arm64: kernel: Add support for User Access Override
In reply to James Morse'User Access Override' is a new ARMv8.2 feature which allows the unprivileged load and store instructions to be overridden to behave in the normal way.
This patch converts {get,put}_user() and friends to use ldtr*/sttr* instructions - so that they can only access EL0 memory, then enables UAO when fs==KERNEL_DS so that these functions can access kernel memory.
This allows user space's read/write permissions to be checked against the page tables, instead of testing addr<USER_DS, then using the kernel's read/write permissions.
--- arch/arm64/Kconfig | 21 +++++++++++ arch/arm64/include/asm/alternative.h | 72 ++++++++++++++++++++++++++++++++++++ arch/arm64/include/asm/cpufeature.h | 3 +- arch/arm64/include/asm/processor.h | 1 + arch/arm64/include/asm/sysreg.h | 3 ++ arch/arm64/include/asm/thread_info.h | 6 +++ arch/arm64/include/asm/uaccess.h | 44 ++++++++++++++++------ arch/arm64/include/uapi/asm/ptrace.h | 1 + arch/arm64/kernel/cpufeature.c | 11 ++++++ arch/arm64/kernel/process.c | 20 ++++++++++ arch/arm64/lib/clear_user.S | 8 ++-- arch/arm64/lib/copy_from_user.S | 8 ++-- arch/arm64/lib/copy_in_user.S | 16 ++++---- arch/arm64/lib/copy_to_user.S | 8 ++-- arch/arm64/mm/fault.c | 31 +++++++++++++--- 15 files changed, 214 insertions(+), 39 deletions(-)
arch/arm64/Kconfig +21 -0
@@ -752,6 +752,27 @@ config ARM64_LSE_ATOMICS endmenu +config ARM64_UAO + bool "Enable support for User Access Override (UAO)" + default y + help + User Access Override (UAO; part of the ARMv8.2 Extensions) + causes the 'unprivileged' variant of the load/store instructions to + be overriden to be privileged. + + This option changes get_user() and friends to use the 'unprivileged' + variant of the load/store instructions. This ensures that user-space + really did have access to the supplied memory. When addr_limit is + set to kernel memory the UAO bit will be set, allowing privileged + access to kernel memory. + + Choosing this option will cause copy_to_user() et al to use user-space + memory permissions. + + The feature is detected at runtime, the kernel will use the + regular load/store instructions if the cpu does not implement the + feature. + endmenu menu "Boot options"
arch/arm64/include/asm/alternative.h +72 -0
90 lines not on this page · raw message
arch/arm64/include/asm/cpufeature.h +2 -1
@@ -30,8 +30,9 @@#define ARM64_HAS_LSE_ATOMICS 5#define ARM64_WORKAROUND_CAVIUM_23154 6#define ARM64_WORKAROUND_834220 7 +#define ARM64_HAS_UAO 8 -#define ARM64_NCAPS 8 +#define ARM64_NCAPS 9#ifndef __ASSEMBLY__
arch/arm64/include/asm/processor.h +1 -0
@@ -187,5 +187,6 @@ static inline void spin_lock_prefetch(const void *x)#endif void cpu_enable_pan (void *__unused); +void cpu_enable_uao (void *__unused);#endif /* __ASM_PROCESSOR_H */
arch/arm64/include/asm/sysreg.h +3 -0
@@ -79,9 +79,12 @@#define SYS_DCZID_EL0 sys_reg(3, 3, 0, 0, 7)#define REG_PSTATE_PAN_IMM sys_reg(0, 0, 4, 0, 4) +#define REG_PSTATE_UAO_IMM sys_reg(0, 0, 4, 0, 3)#define SET_PSTATE_PAN (x) __inst_arm(0xd5000000 | REG_PSTATE_PAN_IMM |\ (!!x)<<8 | 0x1f) +#define SET_PSTATE_UAO (x) __inst_arm(0xd5000000 | REG_PSTATE_UAO_IMM |\ + (!!x)<<8 | 0x1f)/* SCTLR_EL1 */ #define SCTLR_EL1_CP15BEN (0x1 << 5)
arch/arm64/include/asm/thread_info.h +6 -0
@@ -85,6 +85,12 @@ static inline struct thread_info *current_thread_info(void)return (struct thread_info *)sp_el0; } +/* Access struct thread_info of another thread */ +static inline struct thread_info *get_thread_info (unsigned long thread_stack) +{ +return (struct thread_info *)(thread_stack & ~(THREAD_SIZE -1 )); +} +#define thread_saved_pc (tsk) \ ((unsigned long)(tsk->thread.cpu_context.pc))#define thread_saved_sp (tsk) \
arch/arm64/include/asm/uaccess.h +32 -12
88 lines not on this page · raw message
arch/arm64/include/uapi/asm/ptrace.h +1 -0
@@ -45,6 +45,7 @@#define PSR_A_BIT 0x00000100#define PSR_D_BIT 0x00000200#define PSR_PAN_BIT 0x00400000 +#define PSR_UAO_BIT 0x00800000#define PSR_Q_BIT 0x08000000#define PSR_V_BIT 0x10000000#define PSR_C_BIT 0x20000000
arch/arm64/kernel/cpufeature.c +11 -0
@@ -660,6 +660,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .min_field_value = 2, },#endif /* CONFIG_AS_LSE && CONFIG_ARM64_LSE_ATOMICS */ +#ifdef CONFIG_ARM64_UAO + { + .desc = "User Access Override", + .capability = ARM64_HAS_UAO, + .matches = has_cpuid_feature, + .sys_reg = SYS_ID_AA64MMFR2_EL1, + .field_pos = ID_AA64MMFR2_UAO_SHIFT, + .min_field_value = 1, + .enable = cpu_enable_uao, + }, +#endif /* CONFIG_ARM64_UAO */ {}, };
arch/arm64/kernel/process.c +20 -0
@@ -46,6 +46,7 @@#include <linux/notifier.h> #include <trace/events/power.h> +#include <asm/alternative.h> #include <asm/compat.h> #include <asm/cacheflush.h> #include <asm/fpsimd.h>
@@ -280,6 +281,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start, }else {memset (childregs,0 ,sizeof (struct pt_regs )); childregs->pstate = PSR_MODE_EL1h; +if (IS_ENABLED (CONFIG_ARM64_UAO ) && +cpus_have_cap (ARM64_HAS_UAO )) + childregs->pstate |=PSR_UAO_BIT ; p->thread.cpu_context.x19 = stack_start; p->thread.cpu_context.x20 = stk_sz; }
@@ -308,6 +312,20 @@ static void tls_thread_switch(struct task_struct *next) : : "r" (tpidr), "r" (tpidrro)); } +/* Restore the UAO state depending on next's addr_limit */ +static void uao_thread_switch (struct task_struct *next) +{ +unsigned long next_sp = next->thread.cpu_context.sp; + +if (IS_ENABLED (CONFIG_ARM64_UAO ) && +get_thread_info (next_sp)->addr_limit ==KERNEL_DS ) + asm(ALTERNATIVE("nop", SET_PSTATE_UAO(1), ARM64_HAS_UAO, + CONFIG_ARM64_UAO)); +else + asm(ALTERNATIVE("nop", SET_PSTATE_UAO(0), ARM64_HAS_UAO, + CONFIG_ARM64_UAO)); +} +/* * Thread switching. */
@@ -327,6 +345,8 @@ struct task_struct *__switch_to(struct task_struct *prev,*/ dsb (ish); +uao_thread_switch (next); +/* the actual thread switch */ last =cpu_switch_to (prev, next);
arch/arm64/lib/clear_user.S +4 -4
@@ -39,20 +39,20 @@ ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \ subs x1, x1, #8 b.mi 2f 1: -USER(9f, str xzr, [x0], #8 ) +uao_user_alternative 9f, str, sttr, xzr, x0, 8 subs x1, x1, #8 b.pl 1b 2: adds x1, x1, #4 b.mi 3f -USER(9f, str wzr, [x0], #4 ) +uao_user_alternative 9f, str, sttr, wzr, x0, 4 sub x1, x1, #4 3: adds x1, x1, #2 b.mi 4f -USER(9f, strh wzr, [x0], #2 ) +uao_user_alternative 9f, strh, sttrh, wzr, x0, 2 sub x1, x1, #2 4: adds x1, x1, #1 b.mi 5f -USER(9f, strb wzr, [x0] ) +uao_user_alternative 9f, strb, sttrb, wzr, x0, 0 5: mov x0, #0 ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_HAS_PAN, \ CONFIG_ARM64_PAN)
arch/arm64/lib/copy_from_user.S +4 -4
@@ -34,7 +34,7 @@ */ .macro ldrb1 ptr, regB, val - USER(9998f, ldrb \ptr, [\regB], \val) + uao_user_alternative 9998f, ldrb, ldtrb, \ptr, \regB, \val .endm .macro strb1 ptr, regB, val
@@ -42,7 +42,7 @@ .endm .macro ldrh1 ptr, regB, val - USER(9998f, ldrh \ptr, [\regB], \val) + uao_user_alternative 9998f, ldrh, ldtrh, \ptr, \regB, \val .endm .macro strh1 ptr, regB, val
@@ -50,7 +50,7 @@ .endm .macro ldr1 ptr, regB, val - USER(9998f, ldr \ptr, [\regB], \val) + uao_user_alternative 9998f, ldr, ldtr, \ptr, \regB, \val .endm .macro str1 ptr, regB, val
@@ -58,7 +58,7 @@ .endm .macro ldp1 ptr, regB, regC, val - USER(9998f, ldp \ptr, \regB, [\regC], \val) + uao_ldp 9998f, \ptr, \regB, \regC, \val .endm .macro stp1 ptr, regB, regC, val
arch/arm64/lib/copy_in_user.S +8 -8
@@ -35,35 +35,35 @@ * x0 - bytes not copied */ .macro ldrb1 ptr, regB, val - USER(9998f, ldrb \ptr, [\regB], \val) + uao_user_alternative 9998f, ldrb, ldtrb, \ptr, \regB, \val .endm .macro strb1 ptr, regB, val - USER(9998f, strb \ptr, [\regB], \val) + uao_user_alternative 9998f, strb, sttrb, \ptr, \regB, \val .endm .macro ldrh1 ptr, regB, val - USER(9998f, ldrh \ptr, [\regB], \val) + uao_user_alternative 9998f, ldrh, ldtrh, \ptr, \regB, \val .endm .macro strh1 ptr, regB, val - USER(9998f, strh \ptr, [\regB], \val) + uao_user_alternative 9998f, strh, sttrh, \ptr, \regB, \val .endm .macro ldr1 ptr, regB, val - USER(9998f, ldr \ptr, [\regB], \val) + uao_user_alternative 9998f, ldr, ldtr, \ptr, \regB, \val .endm .macro str1 ptr, regB, val - USER(9998f, str \ptr, [\regB], \val) + uao_user_alternative 9998f, str, sttr, \ptr, \regB, \val .endm .macro ldp1 ptr, regB, regC, val - USER(9998f, ldp \ptr, \regB, [\regC], \val) + uao_ldp 9998f, \ptr, \regB, \regC, \val .endm .macro stp1 ptr, regB, regC, val - USER(9998f, stp \ptr, \regB, [\regC], \val) + uao_stp 9998f, \ptr, \regB, \regC, \val .endm end .req x5
arch/arm64/lib/copy_to_user.S +4 -4
@@ -37,7 +37,7 @@ .endm .macro strb1 ptr, regB, val - USER(9998f, strb \ptr, [\regB], \val) + uao_user_alternative 9998f, strb, sttrb, \ptr, \regB, \val .endm .macro ldrh1 ptr, regB, val
@@ -45,7 +45,7 @@ .endm .macro strh1 ptr, regB, val - USER(9998f, strh \ptr, [\regB], \val) + uao_user_alternative 9998f, strh, sttrh, \ptr, \regB, \val .endm .macro ldr1 ptr, regB, val
@@ -53,7 +53,7 @@ .endm .macro str1 ptr, regB, val - USER(9998f, str \ptr, [\regB], \val) + uao_user_alternative 9998f, str, sttr, \ptr, \regB, \val .endm .macro ldp1 ptr, regB, regC, val
@@ -61,7 +61,7 @@ .endm .macro stp1 ptr, regB, regC, val - USER(9998f, stp \ptr, \regB, [\regC], \val) + uao_stp 9998f, \ptr, \regB, \regC, \val .endm end .req x5
arch/arm64/mm/fault.c +25 -6
@@ -192,6 +192,14 @@ out:return fault; } +static inline int permission_fault (unsigned int esr) +{ +unsigned int ec = (esr & ESR_ELx_EC_MASK) >> ESR_ELx_EC_SHIFT; +unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE; + +return (ec == ESR_ELx_EC_DABT_CUR && fsc_type == ESR_ELx_FSC_PERM); +} +static int __kprobes do_page_fault (unsigned long addr,unsigned int esr,struct pt_regs *regs) {
@@ -225,12 +233,10 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, mm_flags |=FAULT_FLAG_WRITE ; } -/* -* PAN bit set implies the fault happened in kernel space, but not -* in the arch's user access functions. -*/ -if (IS_ENABLED (CONFIG_ARM64_PAN ) && (regs->pstate &PSR_PAN_BIT )) - goto no_context; +if (permission_fault (esr) && (addr <USER_DS )) { +if (!search_exception_tables (regs->pc)) +panic ("Accessing user space memory outside uaccess.h routines" ); + }/* * As per x86, we may deadlock here. However, since the kernel only
@@ -561,3 +567,16 @@ void cpu_enable_pan(void *__unused)config_sctlr_el1 (SCTLR_EL1_SPAN ,0 ); }#endif /* CONFIG_ARM64_PAN */ + +#ifdef CONFIG_ARM64_UAO +/* +* Kernel threads have fs=KERNEL_DS by default, and don't need to call +* set_fs(), devtmpfs in particular relies on this behaviour. +* We need to enable the feature at runtime (instead of adding it to +* PSR_MODE_EL1h) as the feature may not be implemented by the cpu. +*/ +void cpu_enable_uao (void *__unused) +{ + asm(SET_PSTATE_UAO(1)); +} +#endif /* CONFIG_ARM64_UAO */
Signature · 1 line
2.6.2
Catalin Marinas
maintainerPATCH v2 3/5 arm64: kernel: Add support for User Access Override
Quoted 2 lines · James Morse
arch/arm64/kernel/process.c +0
-0
[...]
arch/arm64/kernel/process.c static void tls_thread_switch(struct task_struct *next) · line 321
Quoted 11 lines
arch/arm64/kernel/process.c +7
-0@@ -308,6 +312,20 @@ static void tls_thread_switch(struct task_struct *next) : : "r" (tpidr), "r" (tpidrro)); } +/* Restore the UAO state depending on next's addr_limit */ +static void uao_thread_switch (struct task_struct *next) +{ +unsigned long next_sp = next->thread.cpu_context.sp; + +if (IS_ENABLED (CONFIG_ARM64_UAO ) && +get_thread_info (next_sp)->addr_limit ==KERNEL_DS )
Can you not use task_thread_info(next) directly instead of a new get_thread_info()?
arch/arm64/kernel/process.c static void tls_thread_switch(struct task_struct *next) · line 327
Quoted 6 lines
arch/arm64/kernel/process.c +6
-0+ asm(ALTERNATIVE("nop", SET_PSTATE_UAO(1), ARM64_HAS_UAO, + CONFIG_ARM64_UAO)); + else + asm(ALTERNATIVE("nop", SET_PSTATE_UAO(0), ARM64_HAS_UAO, + CONFIG_ARM64_UAO)); +}
I think we still end up with some empty asm in this function even though CONFIG_ARM64_UAO is disabled, I'm not sure the compiler will eliminate it completely. If that's the case, maybe use two nested "if" blocks.
arch/arm64/kernel/process.c struct task_struct *__switch_to(struct task_struct *prev, · line 348
Quoted 9 lines
arch/arm64/kernel/process.c +2
-0+/* * Thread switching. */ @@ -327,6 +345,8 @@ struct task_struct *__switch_to(struct task_struct *prev,*/ dsb (ish); +uao_thread_switch (next);
We should place this before the dsb(), together with the other switches. The pstate change should be self synchronising.
Signature · 1 line
Catalin
PATCH v2 4/5 arm64: cpufeature: Test 'matches' pointer to find the end of the list
In reply to James MorseCPU feature code uses the desc field as a test to find the end of the list, this means every entry must have a description. This generates noise for entries in the list that aren't really features, but combinations of them. e.g.
Quoted 2 lines
CPU features: detected feature: Privileged Access Never CPU features: detected feature: PAN and not UAO
These combination features are needed for corner cases with alternatives, where cpu features interact.
Change all walkers of the arm64_features[] and arm64_hwcaps[] lists to test 'matches' not 'desc', and only print 'desc' if it is non-NULL.
--- arch/arm64/kernel/cpufeature.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
arch/arm64/kernel/cpufeature.c +6 -6
@@ -754,7 +754,7 @@ static void __init setup_cpu_hwcaps(void)int i;const struct arm64_cpu_capabilities *hwcaps = arm64_hwcaps; -for (i =0 ; hwcaps[i].desc; i++) +for (i =0 ; hwcaps[i].matches; i++)if (hwcaps[i].matches (&hwcaps[i]))cap_set_hwcap (&hwcaps[i]); }
@@ -764,11 +764,11 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps, {int i; -for (i =0 ; caps[i].desc; i++) { +for (i =0 ; caps[i].matches; i++) {if (!caps[i].matches (&caps[i]))continue ; -if (!cpus_have_cap (caps[i].capability)) +if (!cpus_have_cap (caps[i].capability) && caps[i].desc)pr_info ("%s %s\n" , info, caps[i].desc);cpus_set_cap (caps[i].capability); }
@@ -783,7 +783,7 @@ enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps) {int i; -for (i =0 ; caps[i].desc; i++) +for (i =0 ; caps[i].matches; i++)if (caps[i].enable &&cpus_have_cap (caps[i].capability))on_each_cpu (caps[i].enable,NULL , true); }
@@ -890,7 +890,7 @@ void verify_local_cpu_capabilities(void)return ; caps = arm64_features; -for (i =0 ; caps[i].desc; i++) { +for (i =0 ; caps[i].matches; i++) {if (!cpus_have_cap (caps[i].capability) || !caps[i].sys_reg)continue ;/*
@@ -903,7 +903,7 @@ void verify_local_cpu_capabilities(void) caps[i].enable (NULL ); } -for (i =0 , caps = arm64_hwcaps; caps[i].desc; i++) { +for (i =0 , caps = arm64_hwcaps; caps[i].matches; i++) {if (!cpus_have_hwcap (&caps[i]))continue ;if (!feature_matches (__raw_read_system_reg (caps[i].sys_reg), &caps[i]))
Signature · 1 line
2.6.2
PATCH v2 5/5 arm64: kernel: Don't toggle PAN on systems with UAO
In reply to James MorseIf a CPU supports both Privileged Access Never (PAN) and User Access Override (UAO), we don't need to disable/re-enable PAN round all copy_to_user() like calls.
UAO alternatives cause these calls to use the 'unprivileged' load/store instructions, which are overridden to be the privileged kind when fs==KERNEL_DS.
This patch changes the copy_to_user() calls to have their PAN toggling depend on a new composite 'feature' ARM64_ALT_PAN_NOT_UAO.
If both features are detected, PAN will be enabled, but the copy_to_user() alternatives will not be applied. This means PAN will be enabled all the time for these functions. If only PAN is detected, the toggling will be enabled as normal.
This will save the time taken to disable/re-enable PAN, and allow us to catch copy_to_user() accesses that occur with fs==KERNEL_DS.
Futex and swp-emulation code continue to hang their PAN toggling code on ARM64_HAS_PAN.
--- arch/arm64/include/asm/cpufeature.h | 3 ++- arch/arm64/include/asm/uaccess.h | 8 ++++---- arch/arm64/kernel/cpufeature.c | 16 ++++++++++++++++ arch/arm64/lib/clear_user.S | 4 ++-- arch/arm64/lib/copy_from_user.S | 4 ++-- arch/arm64/lib/copy_in_user.S | 4 ++-- arch/arm64/lib/copy_to_user.S | 4 ++-- arch/arm64/mm/fault.c | 3 +++ 8 files changed, 33 insertions(+), 13 deletions(-)
arch/arm64/include/asm/cpufeature.h +2 -1
@@ -31,8 +31,9 @@#define ARM64_WORKAROUND_CAVIUM_23154 6#define ARM64_WORKAROUND_834220 7#define ARM64_HAS_UAO 8 +#define ARM64_ALT_PAN_NOT_UAO 9 -#define ARM64_NCAPS 9 +#define ARM64_NCAPS 10#ifndef __ASSEMBLY__
arch/arm64/include/asm/uaccess.h +4 -4
@@ -145,7 +145,7 @@ static inline void set_fs(mm_segment_t fs) do { \ unsigned long __gu_val; \ __chk_user_ptr(ptr); \ - asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN, \ + asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_ALT_PAN_NOT_UAO,\ CONFIG_ARM64_PAN)); \ switch (sizeof(*(ptr))) { \ case 1: \
@@ -168,7 +168,7 @@ do { \BUILD_BUG (); \ } \ (x) = (__force __typeof__(*(ptr)))__gu_val; \ - asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN, \ + asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_ALT_PAN_NOT_UAO,\ CONFIG_ARM64_PAN)); \ }while (0 )
@@ -217,7 +217,7 @@ do { \ do { \ __typeof__(*(ptr)) __pu_val = (x); \ __chk_user_ptr(ptr); \ - asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN, \ + asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_ALT_PAN_NOT_UAO,\ CONFIG_ARM64_PAN)); \ switch (sizeof(*(ptr))) { \ case 1: \
@@ -239,7 +239,7 @@ do { \default : \BUILD_BUG (); \ } \ - asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN, \ + asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_ALT_PAN_NOT_UAO,\ CONFIG_ARM64_PAN)); \ }while (0 )
arch/arm64/kernel/cpufeature.c +16 -0
@@ -67,6 +67,10 @@ DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS); .width = 0, \ } +/* meta feature for alternatives */ +static bool __maybe_unused +cpufeature_pan_not_uao (const struct arm64_cpu_capabilities *entry); +static struct arm64_ftr_bits ftr_id_aa64isar0[] = {ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,32 ,32 ,0 ),ARM64_FTR_BITS (FTR_STRICT ,FTR_EXACT ,ID_AA64ISAR0_RDM_SHIFT ,4 ,0 ),
@@ -671,6 +675,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .enable = cpu_enable_uao, },#endif /* CONFIG_ARM64_UAO */ +#ifdef CONFIG_ARM64_PAN + { + .capability = ARM64_ALT_PAN_NOT_UAO, + .matches = cpufeature_pan_not_uao, + }, +#endif /* CONFIG_ARM64_PAN */ {}, };
@@ -949,3 +959,9 @@ void __init setup_cpu_features(void)pr_warn ("L1_CACHE_BYTES smaller than the Cache Writeback Granule (%d < %d)\n" ,L1_CACHE_BYTES , cls); } + +static bool __maybe_unused +cpufeature_pan_not_uao (const struct arm64_cpu_capabilities *entry) +{ +return (cpus_have_cap (ARM64_HAS_PAN ) && !cpus_have_cap (ARM64_HAS_UAO )); +}
arch/arm64/lib/clear_user.S +2 -2
@@ -33,7 +33,7 @@ * Alignment fixed up by hardware. */ ENTRY(__clear_user) -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) mov x2, x1 // save the size for fixup return subs x1, x1, #8
@@ -54,7 +54,7 @@ uao_user_alternative 9f, strh, sttrh, wzr, x0, 2 b.mi 5f uao_user_alternative 9f, strb, sttrb, wzr, x0, 0 5: mov x0, #0 -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) ret ENDPROC(__clear_user)
arch/arm64/lib/copy_from_user.S +2 -2
@@ -67,11 +67,11 @@ end .req x5 ENTRY(__copy_from_user) -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) add end, x0, x2 #include "copy_template.S" -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) mov x0, #0 // Nothing to copy ret
arch/arm64/lib/copy_in_user.S +2 -2
@@ -68,11 +68,11 @@ end .req x5 ENTRY(__copy_in_user) -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) add end, x0, x2 #include "copy_template.S" -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) mov x0, #0 ret
arch/arm64/lib/copy_to_user.S +2 -2
@@ -66,11 +66,11 @@ end .req x5 ENTRY(__copy_to_user) -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) add end, x0, x2 #include "copy_template.S" -ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_HAS_PAN, \ +ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \ CONFIG_ARM64_PAN) mov x0, #0 ret
arch/arm64/mm/fault.c +3 -0
@@ -234,6 +234,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, }if (permission_fault (esr) && (addr <USER_DS )) { +if (get_thread_info (regs->sp)->addr_limit ==KERNEL_DS ) +panic ("Accessing user space memory with fs=KERNEL_DS" ); +if (!search_exception_tables (regs->pc))panic ("Accessing user space memory outside uaccess.h routines" ); }
Signature · 1 line
2.6.2
Catalin Marinas
maintainerPATCH v2 5/5 arm64: kernel: Don't toggle PAN on systems with UAO
arch/arm64/mm/fault.c static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, · line 238
Quoted 8 lines · James Morse
arch/arm64/mm/fault.c +2
-0@@ -234,6 +234,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, }if (permission_fault (esr) && (addr <USER_DS )) { +if (get_thread_info (regs->sp)->addr_limit ==KERNEL_DS ) +panic ("Accessing user space memory with fs=KERNEL_DS" );
We could simply use "get_fs() == KERNEL_DS" as we should call do_page_fault() outside the current context.
Signature · 1 line
Catalin
PATCH v2 5/5 arm64: kernel: Don't toggle PAN on systems with UAO
Hi Catalin,
Quoted 6 lines · Catalin Marinas
Quoted 5 lines · James Morse
arch/arm64/mm/fault.c +0
-0@@ -234,6 +234,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, }
Quoted 6 lines
arch/arm64/mm/fault.c static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, · line 238
Quoted 3 lines
arch/arm64/mm/fault.c +2
-0if (permission_fault (esr) && (addr <USER_DS )) { +if (get_thread_info (regs->sp)->addr_limit ==KERNEL_DS ) +panic ("Accessing user space memory with fs=KERNEL_DS" );We could simply use "
get_fs()==KERNEL_DS" as we should calldo_page_fault()outside the current context.
shouldn't?
You're right, it will always be on the same stack, so get_fs() will give us the right result. This fits better with getting rid of the new get_thread_info() in patch 3.
Thanks,
James
v2 cover arm64: kernel: Add support for User Access Override
In reply to James MorseQuoted 5 lines · On Friday 05 February 2016 14:58:45 James Morse wrote:
This series adds support for User Access Override (UAO; part of the ARMv8.2 Extensions[0]). When enabled, this causes the
get_user()accessors to use the unprivileged load/store instructions. Whenaddr_limitis set toKERNEL_DS, we set the override bit allowing privileged access.
Quoted 7 lines
Because the unprivileged instructions don't trip PAN, the last patch changes which 'alternative' values are swapped in, allowing PAN to be left enabled during
get_user()and friends.This series can be retrieved from:
git://linux-arm.org/linux-jm.git-b uao/v2
Looks very nice. I have no complaints about the implementations, but two questions:
* There was recently some work in reducing the number of set_fs() calls in the compat_ioctl, which further reduces the attack surface. Should we try to continue that effort in other syscalls?
* Do we expect to handle this using live patching indefinitely? I can imagine that at some point in the future, ARMv8.2+ systems will be the vast majority, so it might be nice to support enabling it unconditionally (same for any of the alternative bits really). Is there a long-term strategy?
Arnd
v2 cover arm64: kernel: Add support for User Access Override
Quoted 4 lines · Arnd Bergmann
* Do we expect to handle this using live patching indefinitely? I can imagine that at some point in the future, ARMv8.2+ systems will be the vast majority, so it might be nice to support enabling it unconditionally (same for any of the alternative bits really). Is there a long-term strategy?
Another good reason for an option turning these things on by default is that it makes debugging *so* much easier when the vmlinux matches what's actually running!
So yes, that's definitely something we should look at in the long-term, but we still need the live patching for distro kernels that will assumedly want a single Image covering ARMv8+ for a while.
Will
Catalin Marinas
maintainerv2 cover arm64: kernel: Add support for User Access Override
In reply to James MorseQuoted 6 lines · James Morse
James Morse (5):
arm64: cpufeature: Changeread_cpuid()to use sysreg'smrs_smacro
arm64: add ARMv8.2id_aa64mmfr2boiler plate
arm64: kernel: Add support for User Access Override
arm64: cpufeature: Test 'matches' pointer to find the end of the list
arm64: kernel: Don't toggle PAN on systems with UAO
Patches applied with an additional fixup on top for removing get_thread_info().
Thanks.
Signature · 1 line
Catalin
v2 cover arm64: kernel: Add support for User Access Override
Quoted 10 lines · Catalin Marinas
Quoted 6 lines · James Morse
James Morse (5):
arm64: cpufeature: Changeread_cpuid()to use sysreg'smrs_smacro
arm64: add ARMv8.2id_aa64mmfr2boiler plate
arm64: kernel: Add support for User Access Override
arm64: cpufeature: Test 'matches' pointer to find the end of the list
arm64: kernel: Don't toggle PAN on systems with UAOPatches applied with an additional fixup on top for removing
get_thread_info().
Just to let you know, unfortunately this series breaks booting the kernel on QEMU. We didn't implement the parts of the ID register space that the ARM ARM documents as "reserved, RAZ", and so when the kernel touches ID_AA64MMFR2 (new in v8.2) QEMU hands it an UNDEF. This is obviously a bug in QEMU, and I just sent out a patch for it: http://patchwork.ozlabs.org/patch/585237/ -- but of course that doesn't do anything for all the buggy QEMUs already in the field.
This is more of a heads-up than a demand that you Do Something, but perhaps somebody has a clever idea...
thanks
-- PMM
Catalin Marinas
maintainerv2 cover arm64: kernel: Add support for User Access Override
Quoted 20 lines · Peter Maydell
Quoted 10 lines · Catalin Marinas
Quoted 6 lines · James Morse
James Morse (5):
arm64: cpufeature: Changeread_cpuid()to use sysreg'smrs_smacro
arm64: add ARMv8.2id_aa64mmfr2boiler plate
arm64: kernel: Add support for User Access Override
arm64: cpufeature: Test 'matches' pointer to find the end of the list
arm64: kernel: Don't toggle PAN on systems with UAOPatches applied with an additional fixup on top for removing
get_thread_info().Just to let you know, unfortunately this series breaks booting the kernel on QEMU. We didn't implement the parts of the ID register space that the ARM ARM documents as "reserved, RAZ", and so when the kernel touches
ID_AA64MMFR2(new in v8.2) QEMU hands it an UNDEF. This is obviously a bug in QEMU, and I just sent out a patch for it: http://patchwork.ozlabs.org/patch/585237/ -- but of course that doesn't do anything for all the buggy QEMUs already in the field.
Quoted 2 lines
This is more of a heads-up than a demand that you Do Something, but perhaps somebody has a clever idea...
Only if Qemu had its own MIDR/REVIDR ;) (and patch the instruction like other errata workarounds).
Signature · 1 line
Catalin
v2 cover arm64: kernel: Add support for User Access Override
Quoted 6 lines · Catalin Marinas
Quoted 2 lines · Peter Maydell
This is more of a heads-up than a demand that you Do Something, but perhaps somebody has a clever idea...
Only if Qemu had its own MIDR/REVIDR ;) (and patch the instruction like other errata workarounds).
Smiley noted, but FWIW the relevant CPUs advertise themselves as MIDR 0x411fd070 REVIDR 0 (A57) and MIDR 0x410fd034 REVIDR 0 (A53), and it should be harmless to patch the ID_AA64MMFR2 read to always return 0 on any A53 or A57 including h/w, because they'll read as zero anyway...
So it comes down to how much we want to work around QEMU "errata".
thanks
-- PMM
v2 cover arm64: kernel: Add support for User Access Override
Quoted 8 lines · Peter Maydell
Quoted 6 lines · Catalin Marinas
Quoted 2 lines · Peter Maydell
This is more of a heads-up than a demand that you Do Something, but perhaps somebody has a clever idea...
Only if Qemu had its own MIDR/REVIDR ;) (and patch the instruction like other errata workarounds).
Quoted 8 lines
Smiley noted, but FWIW the relevant CPUs advertise themselves as MIDR
0x411fd070REVIDR 0 (A57) and MIDR0x410fd034REVIDR 0 (A53), and it should be harmless to patch theID_AA64MMFR2read to always return 0 on any A53 or A57 including h/w, because they'll read as zero anyway...So it comes down to how much we want to work around QEMU "errata".
Doesn't this only affect TCG emulation, and not KVM?
v2 cover arm64: kernel: Add support for User Access Override
Quoted 1 line · Ard Biesheuvel
Doesn't this only affect TCG emulation, and not KVM?
Correct, but that's still a fair userbase.
thanks
-- PMM
v2 cover arm64: kernel: Add support for User Access Override
In reply to James MorseHi Catalin,
I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field of copy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75
Quoted 1 line
/* Invalid usage: none of these should succeed. */
[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass bad_usermem to memset():
arch/arm64/include/asm/uaccess.h:279
Quoted 4 lines
if (access_ok(VERIFY_READ, from, n)) n = __copy_from_user(to, from, n); else /* security hole - plug it */ memset(to, 0, n);
This (correctly) trips UAO's "Accessing user space memory outside uaccess.h routines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.
As far as I can see, this would only affect arm64. I can't find an equivalent memset() for x86_64.
The below ugly hack [0], handles this more gracefully. I can send this as a fix sooner/later if you think its the right thing to do.
Thanks,
James
[0]
-----------------%<-----------------
arch/arm64/include/asm/uaccess.h +0 -0
@@ -278,8 +278,8 @@ static inline unsigned long __must_check copy_from_user(void
*to, const void __u
{
if (access_ok(VERIFY_READ, from, n))
n = __copy_from_user(to, from, n);
- else /* security hole - plug it */
- memset(to, 0, n);
+ else if ((unsigned long)to > USER_DS) /* swapped from/to args? */
+ memset(to, 0, n); /* security hole - plug it */
return n;
}
-----------------%<-----------------
v2 cover arm64: kernel: Add support for User Access Override
Quoted 23 lines · James Morse
Hi Catalin,
I've just spotted UAO causes the
test_user_copymodule (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field ofcopy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75Quoted 1 line
/* Invalid usage: none of these should succeed. */[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok()catches the "(char__user*)kmem", causingcopy_from_user()to passbad_usermemtomemset():
arch/arm64/include/asm/uaccess.h:279Quoted 4 lines
if (access_ok(VERIFY_READ, from, n)) n = __copy_from_user(to, from, n); else /* security hole - plug it */ memset(to, 0, n);
Quoted 6 lines
This (correctly) trips UAO's "Accessing user space memory outside
uaccess.hroutines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.As far as I can see, this would only affect arm64. I can't find an equivalent
memset()forx86_64.
I don't think you've looked hard enough. :)
arch/x86/lib/usercopy_32.c:
unsigned long _copy_from_user(void *to, const void __user *from, unsigned n)
{
if (access_ok(VERIFY_READ, from, n))
n = __copy_from_user(to, from, n);
else
memset(to, 0, n);
return n;
}EXPORT_SYMBOL(_copy_from_user);
Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
v2 cover arm64: kernel: Add support for User Access Override
Quoted 5 lines · Russell King - ARM Linux
Quoted 2 lines · James Morse
As far as I can see, this would only affect arm64. I can't find an equivalent
memset()forx86_64.I don't think you've looked hard enough. :)
Heh, Thanks! I ignored the 32bit code and instead got lost in the maze of underscores and alternative-strings for the 64 bit path.
Having seen that path, I've now found:
Quoted 3 lines
/* If the destination is a kernel buffer, we always clear the end */ if (!__addr_ok(to)) memset(to, 0, len);
in arch/x86/lib/usercopy_64.c:copy_user_handle_tail(), which may be the x86_64 equivalent, or I may be lost in the maze again.
Thanks!
James
v2 cover arm64: kernel: Add support for User Access Override
Quoted 10 lines · James Morse
Quoted 5 lines · Russell King - ARM Linux
Quoted 2 lines · James Morse
As far as I can see, this would only affect arm64. I can't find an equivalent
memset()forx86_64.I don't think you've looked hard enough. :)
Heh, Thanks! I ignored the 32bit code and instead got lost in the maze of underscores and alternative-strings for the 64 bit path.
Quoted 7 lines
Having seen that path, I've now found:
Quoted 3 lines
/* If the destination is a kernel buffer, we always clear the end */ if (!__addr_ok(to)) memset(to, 0, len);in arch/x86/lib/usercopy_64.c:copy_user_handle_tail(), which may be the
x86_64equivalent, or I may be lost in the maze again.
x86_64 has the function in assembler:
/* Standard copy_from_user with segment limit checking */ ENTRY(_copy_from_user)
GET_THREAD_INFO(%rax)
movq %rsi,%rcx
addq %rdx,%rcx
jc bad_from_user
cmpq TI_addr_limit(%rax),%rcx
ja bad_from_user
ALTERNATIVE_2 "jmp copy_user_generic_unrolled", \
"jmp copy_user_generic_string", \
X86_FEATURE_REP_GOOD, \
"jmp copy_user_enhanced_fast_string", \
X86_FEATURE_ERMSENDPROC(_copy_from_user)
.section .fixup,"ax"
/* must zero dest */ENTRY(bad_from_user)bad_from_user:
movl %edx,%ecx
xorl %eax,%eax
rep
stosbbad_to_user:
movl %edx,%eax
retENDPROC(bad_from_user)
The memset() are the 4 instructions after bad_from_user:. This will be triggered if 'from' is an invalid address.
However, you're right that we end up in copy_user_handle_tail() if we fault, which will try to copy as much data and then memset() the remainder. Since this is used for both copy_from_user() and copy_to_user() I think the __addr_ok() is just a side-effect of avoiding memsetting for the copy_to_user() case.
Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Catalin Marinas
maintainerv2 cover arm64: kernel: Add support for User Access Override
In reply to James Morse(cc'ing Kees)
Quoted 16 lines · James Morse
I've just spotted UAO causes the
test_user_copymodule (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field ofcopy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75Quoted 1 line
/* Invalid usage: none of these should succeed. */[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok()catches the "(char__user*)kmem", causingcopy_from_user()to passbad_usermemtomemset():
arch/arm64/include/asm/uaccess.h:279
Quoted 8 lines
Quoted 4 lines
if (access_ok(VERIFY_READ, from, n)) n = __copy_from_user(to, from, n); else /* security hole - plug it */ memset(to, 0, n);This (correctly) trips UAO's "Accessing user space memory outside
uaccess.hroutines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.
I suggest we don't do anything on arch/arm64 (or arch/arm), I just consider the test to be broken. The semantics of copy_from_user() don't say anything about the safety checks on the destination pointer, this is supposed to be a valid kernel address. The test assumes that if the source pointer is invalid, the copy_from_user() routine should not touch the destination.
A better test would be to use a destination buffer filled with a poison value and checked after the operation.
Signature · 1 line
Catalin
v2 cover arm64: kernel: Add support for User Access Override
Quoted 28 lines · Catalin Marinas
(cc'ing Kees)
Quoted 16 lines · James Morse
I've just spotted UAO causes the
test_user_copymodule (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field ofcopy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75Quoted 1 line
/* Invalid usage: none of these should succeed. */[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok()catches the "(char__user*)kmem", causingcopy_from_user()to passbad_usermemtomemset():
arch/arm64/include/asm/uaccess.h:279Quoted 8 lines
Quoted 4 lines
if (
access_ok(VERIFY_READ, from, n))
n =__copy_from_user(to, from, n);
else /* security hole - plug it */
memset(to, 0, n);This (correctly) trips UAO's "Accessing user space memory outside
uaccess.hroutines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.
Quoted 6 lines
I suggest we don't do anything on
arch/arm64(orarch/arm), I just consider the test to be broken. The semantics ofcopy_from_user()don't say anything about the safety checks on the destination pointer, this is supposed to be a valid kernel address. The test assumes that if the source pointer is invalid, thecopy_from_user()routine should not touch the destination.
Hmmm, I'd prefer that copy_*_user was checking both src and destination. That's what these tests are checking for.
Quoted 2 lines
A better test would be to use a destination buffer filled with a poison value and checked after the operation.
Well, I could certainly add that check, but I'd prefer that a bogus destination be rejected.
-Kees
Signature · 2 lines
Kees Cook Chrome OS & Brillo Security
Catalin Marinas
maintainerv2 cover arm64: kernel: Add support for User Access Override
Quoted 36 lines · Kees Cook
Quoted 28 lines · Catalin Marinas
(cc'ing Kees)
Quoted 16 lines · James Morse
I've just spotted UAO causes the
test_user_copymodule (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field ofcopy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75Quoted 1 line
/* Invalid usage: none of these should succeed. */[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok()catches the "(char__user*)kmem", causingcopy_from_user()to passbad_usermemtomemset():
arch/arm64/include/asm/uaccess.h:279Quoted 8 lines
Quoted 4 lines
if (
access_ok(VERIFY_READ, from, n))
n =__copy_from_user(to, from, n);
else /* security hole - plug it */
memset(to, 0, n);This (correctly) trips UAO's "Accessing user space memory outside
uaccess.hroutines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.Quoted 6 lines
I suggest we don't do anything on
arch/arm64(orarch/arm), I just consider the test to be broken. The semantics ofcopy_from_user()don't say anything about the safety checks on the destination pointer, this is supposed to be a valid kernel address. The test assumes that if the source pointer is invalid, thecopy_from_user()routine should not touch the destination.
Quoted 2 lines
Hmmm, I'd prefer that copy_*_user was checking both src and destination. That's what these tests are checking for.
If the kernel pointer (source or destination, depending on the function) is not valid, I would rather panic the kernel (maybe as a result of a data abort) than silently returning a non-zero value from copy_from_user() which doesn't even tell whether the source or destination pointer is wrong.
The copy_*_user functions are indeed meant to check the validity of user pointer arguments and safely handle aborts (extable annotations) since these values are usually provided by user space. But the kernel pointer arguments are under the control of the kernel, so IMO passing a corrupt value is a serious kernel/driver bug.
Signature · 1 line
Catalin
v2 cover arm64: kernel: Add support for User Access Override
Quoted 46 lines · Catalin Marinas
Quoted 36 lines · Kees Cook
Quoted 28 lines · Catalin Marinas
(cc'ing Kees)
Quoted 16 lines · James Morse
I've just spotted UAO causes the
test_user_copymodule (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field ofcopy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75Quoted 1 line
/* Invalid usage: none of these should succeed. */[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok()catches the "(char__user*)kmem", causingcopy_from_user()to passbad_usermemtomemset():
arch/arm64/include/asm/uaccess.h:279Quoted 8 lines
Quoted 4 lines
if (
access_ok(VERIFY_READ, from, n))
n =__copy_from_user(to, from, n);
else /* security hole - plug it */
memset(to, 0, n);This (correctly) trips UAO's "Accessing user space memory outside
uaccess.hroutines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.Quoted 6 lines
I suggest we don't do anything on
arch/arm64(orarch/arm), I just consider the test to be broken. The semantics ofcopy_from_user()don't say anything about the safety checks on the destination pointer, this is supposed to be a valid kernel address. The test assumes that if the source pointer is invalid, thecopy_from_user()routine should not touch the destination.Quoted 2 lines
Hmmm, I'd prefer that copy_*_user was checking both src and destination. That's what these tests are checking for.
If the kernel pointer (source or destination, depending on the function) is not valid, I would rather panic the kernel (maybe as a result of a data abort) than silently returning a non-zero value from
copy_from_user()which doesn't even tell whether the source or destination pointer is wrong.
Quoted 5 lines
The copy_*_user functions are indeed meant to check the validity of user pointer arguments and safely handle aborts (extable annotations) since these values are usually provided by user space. But the kernel pointer arguments are under the control of the kernel, so IMO passing a corrupt value is a serious kernel/driver bug.
I'm totally fine with that, though I suspect Linus would not like this (perhaps just Oops instead of panic). Regardless, if that was changed, we could move this entire test into lkdtm (where system panics are considered a "success").
-Kees
Signature · 2 lines
Kees Cook Chrome OS & Brillo Security
Catalin Marinas
maintainerv2 cover arm64: kernel: Add support for User Access Override
Quoted 47 lines · Kees Cook
Quoted 46 lines · Catalin Marinas
Quoted 36 lines · Kees Cook
Quoted 28 lines · Catalin Marinas
(cc'ing Kees)
Quoted 16 lines · James Morse
I've just spotted UAO causes the
test_user_copymodule (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field ofcopy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75Quoted 1 line
/* Invalid usage: none of these should succeed. */[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok()catches the "(char__user*)kmem", causingcopy_from_user()to passbad_usermemtomemset():
arch/arm64/include/asm/uaccess.h:279Quoted 8 lines
Quoted 4 lines
if (
access_ok(VERIFY_READ, from, n))
n =__copy_from_user(to, from, n);
else /* security hole - plug it */
memset(to, 0, n);This (correctly) trips UAO's "Accessing user space memory outside
uaccess.hroutines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.Quoted 6 lines
I suggest we don't do anything on
arch/arm64(orarch/arm), I just consider the test to be broken. The semantics ofcopy_from_user()don't say anything about the safety checks on the destination pointer, this is supposed to be a valid kernel address. The test assumes that if the source pointer is invalid, thecopy_from_user()routine should not touch the destination.Quoted 2 lines
Hmmm, I'd prefer that copy_*_user was checking both src and destination. That's what these tests are checking for.
If the kernel pointer (source or destination, depending on the function) is not valid, I would rather panic the kernel (maybe as a result of a data abort) than silently returning a non-zero value from
copy_from_user()which doesn't even tell whether the source or destination pointer is wrong.
Quoted 8 lines
Quoted 5 lines
The copy_*_user functions are indeed meant to check the validity of user pointer arguments and safely handle aborts (extable annotations) since these values are usually provided by user space. But the kernel pointer arguments are under the control of the kernel, so IMO passing a corrupt value is a serious kernel/driver bug.
I'm totally fine with that, though I suspect Linus would not like this (perhaps just Oops instead of panic).
That's what we get on arm64 with PAN enabled (oops).
Quoted 2 lines
Regardless, if that was changed, we could move this entire test into lkdtm (where system panics are considered a "success").
The only downside is that not all architectures behave in the same way w.r.t. the copy_*_user() routines, so the test may have some surprises with the oops not being triggered.
Anyway, I think the test should also check the invalid source/destination independently (possibly in combination with set_fs(KERNEL_DS)).
Signature · 1 line
Catalin
v2 cover arm64: kernel: Add support for User Access Override
Quoted 57 lines · Catalin Marinas
Quoted 47 lines · Kees Cook
Quoted 46 lines · Catalin Marinas
Quoted 36 lines · Kees Cook
Quoted 28 lines · Catalin Marinas
(cc'ing Kees)
Quoted 16 lines · James Morse
I've just spotted UAO causes the
test_user_copymodule (CONFIG_TEST_USER_COPY) to fail. Who to blame is up for discussion. The test is passing a user pointer as the 'to' field ofcopy_from_user(), which it expects to fail gracefully:
lib/test_user_copy.c:75Quoted 1 line
/* Invalid usage: none of these should succeed. */[ ... ]
Quoted 4 lines
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed");
access_ok()catches the "(char__user*)kmem", causingcopy_from_user()to passbad_usermemtomemset():
arch/arm64/include/asm/uaccess.h:279Quoted 8 lines
Quoted 4 lines
if (
access_ok(VERIFY_READ, from, n))
n =__copy_from_user(to, from, n);
else /* security hole - plug it */
memset(to, 0, n);This (correctly) trips UAO's "Accessing user space memory outside
uaccess.hroutines" message, which is a little confusing to debug, and stops the rest of the module's tests from being run.Quoted 6 lines
I suggest we don't do anything on
arch/arm64(orarch/arm), I just consider the test to be broken. The semantics ofcopy_from_user()don't say anything about the safety checks on the destination pointer, this is supposed to be a valid kernel address. The test assumes that if the source pointer is invalid, thecopy_from_user()routine should not touch the destination.Quoted 2 lines
Hmmm, I'd prefer that copy_*_user was checking both src and destination. That's what these tests are checking for.
If the kernel pointer (source or destination, depending on the function) is not valid, I would rather panic the kernel (maybe as a result of a data abort) than silently returning a non-zero value from
copy_from_user()which doesn't even tell whether the source or destination pointer is wrong.Quoted 8 lines
Quoted 5 lines
The copy_*_user functions are indeed meant to check the validity of user pointer arguments and safely handle aborts (extable annotations) since these values are usually provided by user space. But the kernel pointer arguments are under the control of the kernel, so IMO passing a corrupt value is a serious kernel/driver bug.
I'm totally fine with that, though I suspect Linus would not like this (perhaps just Oops instead of panic).
Quoted 8 lines
That's what we get on arm64 with PAN enabled (oops).
Quoted 2 lines
Regardless, if that was changed, we could move this entire test into lkdtm (where system panics are considered a "success").
The only downside is that not all architectures behave in the same way w.r.t. the copy_*_user() routines, so the test may have some surprises with the oops not being triggered.
Right. I think if the tests were moved to lkdtm, we could do the checks in a few stages, where lkdtm would generate the Oops if copy_*_user correctly errored/ignored the bad combo without Oopsing on its own.
Quoted 3 lines
Anyway, I think the test should also check the invalid source/destination independently (possibly in combination with
set_fs(KERNEL_DS)).
I'm open to whatever you like. :)
-Kees
Signature · 2 lines
Kees Cook Chrome OS & Brillo Security