| 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: <1818f98d257c2431c600240ccda628e4@misterjones.org>
Date: Wed, 20 Jan 2021 13:16:28 +0000
From: Marc Zyngier <maz@...nel.org>
To: Mohamed Mediouni <mohamed.mediouni@...amail.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Stan Skowronek <stan@...ellium.com>
Subject: Re: [PATCH 1/3] arm64/kernel: FIQ support
Hi Mohamed,
On 2021-01-20 11:36, Mohamed Mediouni wrote:
> From: Stan Skowronek <stan@...ellium.com>
>
> On Apple processors, the timer is wired through FIQ.
Which timer? There are at least 3, potentially 4 timers per CPU
that can fire.
> As such, add FIQ support to the kernel.
>
> Signed-off-by: Stan Skowronek <stan@...ellium.com>
Missing SoB from the sender.
> ---
> arch/arm64/include/asm/arch_gicv3.h | 2 +-
> arch/arm64/include/asm/assembler.h | 8 ++--
> arch/arm64/include/asm/daifflags.h | 4 +-
> arch/arm64/include/asm/irq.h | 4 ++
> arch/arm64/include/asm/irqflags.h | 6 +--
> arch/arm64/kernel/entry.S | 74 ++++++++++++++++++++++++++---
> arch/arm64/kernel/irq.c | 14 ++++++
> arch/arm64/kernel/process.c | 2 +-
> 8 files changed, 97 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm64/include/asm/arch_gicv3.h
> b/arch/arm64/include/asm/arch_gicv3.h
> index 880b9054d75c..934b9be582d2 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -173,7 +173,7 @@ static inline void gic_pmr_mask_irqs(void)
>
> static inline void gic_arch_enable_irqs(void)
> {
> - asm volatile ("msr daifclr, #2" : : : "memory");
> + asm volatile ("msr daifclr, #3" : : : "memory");
If I trust the persistent rumour, this system doesn't have a GIC.
Why this change?
> }
>
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/arm64/include/asm/assembler.h
> b/arch/arm64/include/asm/assembler.h
> index bf125c591116..6fe55713dfe0 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -40,9 +40,9 @@
> msr daif, \flags
> .endm
>
> - /* IRQ is the lowest priority flag, unconditionally unmask the rest.
> */
> - .macro enable_da_f
> - msr daifclr, #(8 | 4 | 1)
> + /* IRQ/FIQ is the lowest priority flag, unconditionally unmask the
> rest. */
> + .macro enable_da
> + msr daifclr, #(8 | 4)
This cannot be unconditional. This potentially changes existing
behaviours,
and I'd feel a lot safer if FIQ was only messed with on that specific
HW.
I have the feeling that this should be detected on the boot CPU and
patched
before any interrupt can fire.
> .endm
>
> /*
> @@ -50,7 +50,7 @@
> */
> .macro save_and_disable_irq, flags
> mrs \flags, daif
> - msr daifset, #2
> + msr daifset, #3
> .endm
>
> .macro restore_irq, flags
> diff --git a/arch/arm64/include/asm/daifflags.h
> b/arch/arm64/include/asm/daifflags.h
> index 1c26d7baa67f..44de96c7fb1a 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -13,8 +13,8 @@
> #include <asm/ptrace.h>
>
> #define DAIF_PROCCTX 0
> -#define DAIF_PROCCTX_NOIRQ PSR_I_BIT
> -#define DAIF_ERRCTX (PSR_I_BIT | PSR_A_BIT)
> +#define DAIF_PROCCTX_NOIRQ (PSR_I_BIT | PSR_F_BIT)
> +#define DAIF_ERRCTX (PSR_I_BIT | PSR_F_BIT | PSR_A_BIT)
> #define DAIF_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
>
>
> diff --git a/arch/arm64/include/asm/irq.h
> b/arch/arm64/include/asm/irq.h
> index b2b0c6405eb0..2d1537d3a245 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -13,5 +13,9 @@ static inline int nr_legacy_irqs(void)
> return 0;
> }
>
> +int set_handle_fiq(void (*handle_fiq)(struct pt_regs *));
> +
> +extern void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init;
I guess this is set from the root interrupt controller, which also
will set handle_arch_irq? Why do we need two entry points? We have
ISR_EL1 to find out what is pending. Isn't that enough?
> +
> #endif /* !__ASSEMBLER__ */
> #endif
> diff --git a/arch/arm64/include/asm/irqflags.h
> b/arch/arm64/include/asm/irqflags.h
> index ff328e5bbb75..26d7f378113e 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -35,7 +35,7 @@ static inline void arch_local_irq_enable(void)
> }
>
> asm volatile(ALTERNATIVE(
> - "msr daifclr, #2 // arch_local_irq_enable",
> + "msr daifclr, #3 // arch_local_irq_enable",
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ARM64_HAS_IRQ_PRIO_MASKING)
> :
> @@ -54,7 +54,7 @@ static inline void arch_local_irq_disable(void)
> }
>
> asm volatile(ALTERNATIVE(
> - "msr daifset, #2 // arch_local_irq_disable",
> + "msr daifset, #3 // arch_local_irq_disable",
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ARM64_HAS_IRQ_PRIO_MASKING)
> :
> @@ -85,7 +85,7 @@ static inline int arch_irqs_disabled_flags(unsigned
> long flags)
> int res;
>
> asm volatile(ALTERNATIVE(
> - "and %w0, %w1, #" __stringify(PSR_I_BIT),
> + "and %w0, %w1, #" __stringify(PSR_I_BIT | PSR_F_BIT),
> "eor %w0, %w1, #" __stringify(GIC_PRIO_IRQON),
> ARM64_HAS_IRQ_PRIO_MASKING)
> : "=&r" (res)
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index c9bae73f2621..abcca0db0736 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -499,6 +499,14 @@ tsk .req x28 // current thread_info
> irq_stack_exit
> .endm
>
> + .macro fiq_handler
> + ldr_l x1, handle_arch_fiq
> + mov x0, sp
> + irq_stack_entry
> + blr x1
> + irq_stack_exit
> + .endm
> +
> #ifdef CONFIG_ARM64_PSEUDO_NMI
> /*
> * Set res to 0 if irqs were unmasked in interrupted context.
> @@ -547,18 +555,18 @@ SYM_CODE_START(vectors)
>
> kernel_ventry 1, sync // Synchronous EL1h
> kernel_ventry 1, irq // IRQ EL1h
> - kernel_ventry 1, fiq_invalid // FIQ EL1h
> + kernel_ventry 1, fiq // FIQ EL1h
> kernel_ventry 1, error // Error EL1h
>
> kernel_ventry 0, sync // Synchronous 64-bit EL0
> kernel_ventry 0, irq // IRQ 64-bit EL0
> - kernel_ventry 0, fiq_invalid // FIQ 64-bit EL0
> + kernel_ventry 0, fiq // FIQ 64-bit EL0
> kernel_ventry 0, error // Error 64-bit EL0
>
> #ifdef CONFIG_COMPAT
> kernel_ventry 0, sync_compat, 32 // Synchronous 32-bit EL0
> kernel_ventry 0, irq_compat, 32 // IRQ 32-bit EL0
> - kernel_ventry 0, fiq_invalid_compat, 32 // FIQ 32-bit EL0
> + kernel_ventry 0, fiq_compat, 32 // FIQ 32-bit EL0
> kernel_ventry 0, error_compat, 32 // Error 32-bit EL0
> #else
> kernel_ventry 0, sync_invalid, 32 // Synchronous 32-bit EL0
> @@ -661,7 +669,7 @@ SYM_CODE_END(el1_sync)
> SYM_CODE_START_LOCAL_NOALIGN(el1_irq)
> kernel_entry 1
> gic_prio_irq_setup pmr=x20, tmp=x1
> - enable_da_f
> + enable_da
>
> mov x0, sp
> bl enter_el1_irq_or_nmi
> @@ -689,6 +697,38 @@ alternative_else_nop_endif
> kernel_exit 1
> SYM_CODE_END(el1_irq)
>
> + .align 6
> +SYM_CODE_START_LOCAL_NOALIGN(el1_fiq)
> + kernel_entry 1
> + gic_prio_irq_setup pmr=x20, tmp=x1
This doesn't make much sense. The HW doesn't have a GIC, and Linux
doesn't use Group-0 interrupts, even in a guest.
> + enable_da
> +
> + mov x0, sp
> + bl enter_el1_irq_or_nmi
> +
> + fiq_handler
> +
> +#ifdef CONFIG_PREEMPTION
> + ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count
> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> + /*
> + * DA_F were cleared at start of handling. If anything is set in
> DAIF,
> + * we come back from an NMI, so skip preemption
> + */
> + mrs x0, daif
> + orr x24, x24, x0
> +alternative_else_nop_endif
> + cbnz x24, 1f // preempt count != 0 || NMI return path
> + bl arm64_preempt_schedule_irq // irq en/disable is done inside
> +1:
> +#endif
> +
> + mov x0, sp
> + bl exit_el1_irq_or_nmi
> +
> + kernel_exit 1
> +SYM_CODE_END(el1_fiq)
Given that this is essentially a copy paste of el1_irq, and that
a separate FIQ entry point seems superfluous, I don't think we
need any of this, and it could be implemented just as the IRQ
vector.
> +
> /*
> * EL0 mode handlers.
> */
> @@ -715,6 +755,12 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_irq_compat)
> b el0_irq_naked
> SYM_CODE_END(el0_irq_compat)
>
> + .align 6
> +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq_compat)
> + kernel_entry 0, 32
> + b el0_fiq_naked
> +SYM_CODE_END(el0_fiq_compat)
> +
> SYM_CODE_START_LOCAL_NOALIGN(el0_error_compat)
> kernel_entry 0, 32
> b el0_error_naked
> @@ -727,7 +773,7 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_irq)
> el0_irq_naked:
> gic_prio_irq_setup pmr=x20, tmp=x0
> user_exit_irqoff
> - enable_da_f
> + enable_da
>
> tbz x22, #55, 1f
> bl do_el0_irq_bp_hardening
> @@ -737,6 +783,22 @@ el0_irq_naked:
> b ret_to_user
> SYM_CODE_END(el0_irq)
>
> + .align 6
> +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq)
> + kernel_entry 0
> +el0_fiq_naked:
> + gic_prio_irq_setup pmr=x20, tmp=x0
> + user_exit_irqoff
> + enable_da
> +
> + tbz x22, #55, 1f
> + bl do_el0_irq_bp_hardening
> +1:
> + fiq_handler
> +
> + b ret_to_user
> +SYM_CODE_END(el0_fiq)
Same thing.
> +
> SYM_CODE_START_LOCAL(el1_error)
> kernel_entry 1
> mrs x1, esr_el1
> @@ -757,7 +819,7 @@ el0_error_naked:
> mov x0, sp
> mov x1, x25
> bl do_serror
> - enable_da_f
> + enable_da
> b ret_to_user
> SYM_CODE_END(el0_error)
>
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index dfb1feab867d..4d7a9fb41d93 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -88,3 +88,17 @@ void __init init_IRQ(void)
> local_daif_restore(DAIF_PROCCTX_NOIRQ);
> }
> }
> +
> +/*
> + * Analogous to the generic handle_arch_irq / set_handle_irq
> + */
> +void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init;
> +
> +int __init set_handle_fiq(void (*handle_fiq)(struct pt_regs *))
> +{
> + if (handle_arch_fiq)
> + return -EBUSY;
> +
> + handle_arch_fiq = handle_fiq;
> + return 0;
> +}
Also, this has no caller, so it is pretty hard to judge how useful
this is.
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 6616486a58fe..34ec400288d0 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -84,7 +84,7 @@ static void noinstr __cpu_do_idle_irqprio(void)
> unsigned long daif_bits;
>
> daif_bits = read_sysreg(daif);
> - write_sysreg(daif_bits | PSR_I_BIT, daif);
> + write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif);
>
> /*
> * Unmask PMR before going idle to make sure interrupts can
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists