| 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: <ZnP9XNrz6L47Lhk1@J2N7QTR9R3>
Date: Thu, 20 Jun 2024 10:58:52 +0100
From: Mark Rutland <mark.rutland@....com>
To: Ryan Roberts <ryan.roberts@....com>
Cc: Anshuman Khandual <anshuman.khandual@....com>,
linux-arm-kernel@...ts.infradead.org, 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 Thu, Jun 20, 2024 at 09:45:38AM +0100, Ryan Roberts wrote:
> 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.
FWIW, I agree, I had preferred the spearate definitions because that
matched what was in the ARM ARM and didn't infer a pattern that could be
broken later.
> 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>
Likewise, either way:
Acked-by: Mark Rutland <mark.rutland@....com>
Mark.
>
> > +
> > /* 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 */
>
Powered by blists - more mailing lists