linux-kernel - Re: [PATCH V2] arm64/mm: Stop using ESR_ELx_FSC

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d29136e9-6c63-4e4f-81bb-488edcac587e@arm.com>
Date: Thu, 20 Jun 2024 09:45:38 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Anshuman Khandual <anshuman.khandual@....com>,
 linux-arm-kernel@...ts.infradead.org
Cc: mark.rutland@....com, maz@...nel.org,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] arm64/mm: Stop using ESR_ELx_FSC_TYPE during fault

On 18/06/2024 04:47, 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, where in
> 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 stop using 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 applies on v6.10-rc4 and still leaves behind ESR_ELx_FSC_TYPE for now.
> 
> Changes in V2:
> 
> - Defined ESR_ELx_FSC_[ACCESS|FAULT_PERM]_L() macros
> - Changed fault helpers using the above macros instead
> - Dropped each page table level fault status discrete values
> - Dropped set_thread_esr() changes in arch/arm64/mm/fault.c
> - Updated the commit message
> 
> Changes in V1:
> 
> https://lore.kernel.org/linux-arm-kernel/20240613094538.3263536-1-anshuman.khandual@arm.com/
> 
>  arch/arm64/include/asm/esr.h | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7abf09df7033..3f482500f71f 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -121,6 +121,14 @@
>  #define ESR_ELx_FSC_SECC	(0x18)
>  #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
>  
> +/* Status codes for individual page table levels */
> +#define ESR_ELx_FSC_ACCESS_L(n)	(ESR_ELx_FSC_ACCESS + n)
> +#define ESR_ELx_FSC_PERM_L(n)	(ESR_ELx_FSC_PERM + n)
> +
> +#define ESR_ELx_FSC_FAULT_nL	(0x2C)
> +#define ESR_ELx_FSC_FAULT_L(n)	(((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \
> +					    ESR_ELx_FSC_FAULT) + (n))

I think the only real argument for parameterizing it like this is so we can
write "-1" as a parameter rather than "N1" as part of the macro name? Other than
that (marginal) benefit, personally I don't think this approach is very
extensible because we are devining a pattern from the encoding that doesn't
really exist. If we ever needed a level 4 or -3 the encoding would have to be
discontiguous and we would need to rework this again to accomodate. Perhaps the
chances of that ever happening are small enough that the problem can be ignored.

TBH, I didn't really follow Marc's argument for keeping the "type" macros either
since ESR_ELx_FSC_FAULT does not help to identify the type of a level -1 or -2
translation fault - the encoding is completely different. But I'll take it on
faith that Marc is correct and I just don't understand ;-)

Regardless, the implementation looks correct, so:

Reviewed-by: Ryan Roberts <ryan.roberts@....com>

> +
>  /* ISS field definitions for Data Aborts */
>  #define ESR_ELx_ISV_SHIFT	(24)
>  #define ESR_ELx_ISV		(UL(1) << ESR_ELx_ISV_SHIFT)
> @@ -388,20 +396,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_L(3)) ||
> +	       (esr == ESR_ELx_FSC_FAULT_L(2)) ||
> +	       (esr == ESR_ELx_FSC_FAULT_L(1)) ||
> +	       (esr == ESR_ELx_FSC_FAULT_L(0)) ||
> +	       (esr == ESR_ELx_FSC_FAULT_L(-1));
>  }
>  
>  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_L(3)) ||
> +	       (esr == ESR_ELx_FSC_PERM_L(2)) ||
> +	       (esr == ESR_ELx_FSC_PERM_L(1)) ||
> +	       (esr == ESR_ELx_FSC_PERM_L(0));
>  }
>  
>  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_L(3)) ||
> +	       (esr == ESR_ELx_FSC_ACCESS_L(2)) ||
> +	       (esr == ESR_ELx_FSC_ACCESS_L(1)) ||
> +	       (esr == ESR_ELx_FSC_ACCESS_L(0));
>  }
>  
>  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */