| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3665e735-1927-49b3-ad93-c50eb919b370@arm.com>
Date: Thu, 13 Jun 2024 11:03:09 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Anshuman Khandual <anshuman.khandual@....com>,
linux-arm-kernel@...ts.infradead.org
Cc: mark.rutland@....com, Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE
On 13/06/2024 10:45, Anshuman Khandual wrote:
> Fault status codes at page table level 0, 1, 2 and 3 for access, permission
> and translation faults are architecturally organized in a way, that masking
> out ESR_ELx_FSC_TYPE, fetches Level 0 status code for the respective fault.
>
> Helpers like esr_fsc_is_[translation|permission|access_flag]_fault() mask
> out ESR_ELx_FSC_TYPE before comparing against corresponding Level 0 status
> code as the kernel does not yet care about the page table level, the fault
> really occurred previously.
>
> This scheme is starting to crumble after FEAT_LPA2 when level -1 got added.
> Fault status code for translation fault at level -1 is 0x2B which does not
> follow ESR_ELx_FSC_TYPE, requiring esr_fsc_is_translation_fault() changes.
>
> This changes above helpers to compare against individual fault status code
> values for each page table level and drop ESR_ELx_FSC_TYPE which is losing
> its value as a common mask.
>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Marc Zyngier <maz@...nel.org>
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
This certainly looks like a nice clean up from a readability point of view, and
will also make it easier to support extra levels in future. It's probably
marginally slower, but given you are comparing the lowest level first, which I
guess is most likely to fault, you will short circuit most comparisons most of
the time. So:
Reviewed-by: Ryan Roberts <ryan.roberts@....com>
> ---
> This applies on 6.10-rc3
>
> arch/arm64/include/asm/esr.h | 42 +++++++++++++++++++++++++++---------
> arch/arm64/mm/fault.c | 4 ++--
> 2 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7abf09df7033..8cc0311d3fba 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -109,14 +109,23 @@
>
> /* Shared ISS fault status code(IFSC/DFSC) for Data/Instruction aborts */
> #define ESR_ELx_FSC (0x3F)
> -#define ESR_ELx_FSC_TYPE (0x3C)
> #define ESR_ELx_FSC_LEVEL (0x03)
> #define ESR_ELx_FSC_EXTABT (0x10)
> #define ESR_ELx_FSC_MTE (0x11)
> #define ESR_ELx_FSC_SERROR (0x11)
> -#define ESR_ELx_FSC_ACCESS (0x08)
> -#define ESR_ELx_FSC_FAULT (0x04)
> -#define ESR_ELx_FSC_PERM (0x0C)
> +#define ESR_ELx_FSC_ACCESS_L0 (0x08)
> +#define ESR_ELx_FSC_ACCESS_L1 (0x09)
> +#define ESR_ELx_FSC_ACCESS_L2 (0x0A)
> +#define ESR_ELx_FSC_ACCESS_L3 (0x0B)
> +#define ESR_ELx_FSC_FAULT_LN1 (0x2B)
> +#define ESR_ELx_FSC_FAULT_L0 (0x04)
> +#define ESR_ELx_FSC_FAULT_L1 (0x05)
> +#define ESR_ELx_FSC_FAULT_L2 (0x06)
> +#define ESR_ELx_FSC_FAULT_L3 (0x07)
> +#define ESR_ELx_FSC_PERM_L0 (0x0C)
> +#define ESR_ELx_FSC_PERM_L1 (0x0D)
> +#define ESR_ELx_FSC_PERM_L2 (0x0E)
> +#define ESR_ELx_FSC_PERM_L3 (0x0F)
> #define ESR_ELx_FSC_SEA_TTW(n) (0x14 + (n))
> #define ESR_ELx_FSC_SECC (0x18)
> #define ESR_ELx_FSC_SECC_TTW(n) (0x1c + (n))
> @@ -388,20 +397,33 @@ static inline bool esr_is_data_abort(unsigned long esr)
>
> static inline bool esr_fsc_is_translation_fault(unsigned long esr)
> {
> - /* Translation fault, level -1 */
> - if ((esr & ESR_ELx_FSC) == 0b101011)
> - return true;
> - return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
> + esr = esr & ESR_ELx_FSC;
> +
> + return (esr == ESR_ELx_FSC_FAULT_L3) ||
> + (esr == ESR_ELx_FSC_FAULT_L2) ||
> + (esr == ESR_ELx_FSC_FAULT_L1) ||
> + (esr == ESR_ELx_FSC_FAULT_L0) ||
> + (esr == ESR_ELx_FSC_FAULT_LN1);
> }
>
> static inline bool esr_fsc_is_permission_fault(unsigned long esr)
> {
> - return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_PERM;
> + esr = esr & ESR_ELx_FSC;
> +
> + return (esr == ESR_ELx_FSC_PERM_L3) ||
> + (esr == ESR_ELx_FSC_PERM_L2) ||
> + (esr == ESR_ELx_FSC_PERM_L1) ||
> + (esr == ESR_ELx_FSC_PERM_L0);
> }
>
> static inline bool esr_fsc_is_access_flag_fault(unsigned long esr)
> {
> - return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_ACCESS;
> + esr = esr & ESR_ELx_FSC;
> +
> + return (esr == ESR_ELx_FSC_ACCESS_L3) ||
> + (esr == ESR_ELx_FSC_ACCESS_L2) ||
> + (esr == ESR_ELx_FSC_ACCESS_L1) ||
> + (esr == ESR_ELx_FSC_ACCESS_L0);
> }
>
> /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 451ba7cbd5ad..7199aaff2a29 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -440,7 +440,7 @@ static void set_thread_esr(unsigned long address, unsigned long esr)
> */
> esr &= ESR_ELx_EC_MASK | ESR_ELx_IL |
> ESR_ELx_CM | ESR_ELx_WNR;
> - esr |= ESR_ELx_FSC_FAULT;
> + esr |= ESR_ELx_FSC_FAULT_L0;
> break;
> case ESR_ELx_EC_IABT_LOW:
> /*
> @@ -449,7 +449,7 @@ static void set_thread_esr(unsigned long address, unsigned long esr)
> * reported with that DFSC value, so we clear them.
> */
> esr &= ESR_ELx_EC_MASK | ESR_ELx_IL;
> - esr |= ESR_ELx_FSC_FAULT;
> + esr |= ESR_ELx_FSC_FAULT_L0;
> break;
> default:
> /*
Powered by blists - more mailing lists