[PATCH v2 0/4] arm64: Allow the compat vdso to be disabled at runtime

[PATCH v2 0/4] arm64: Allow the compat vdso to be disabled at runtime

[Marc Zyngier]

The relatively recent introduction of the compat vdso on arm64 has
overlooked its interactions with some of the interesting errata
workarounds, such as ARM64_ERRATUM_1418040 (and its older 1188873
incarnation).

This erratum requires the 64bit kernel to trap 32bit accesses to the
virtual counter and emulate it. When the workaround was introduced,
the compat vdso simply wasn't a thing. Now that the patches have
landed in mainline, we trap the CVTVCT accesses from the vdso.

This can end-up in a nasty loop in the vdso, where the sequence number
changes on each trap, never stabilising, and leaving userspace in a
bit of a funny state (which is why we disable the vdso in most similar
cases). This erratum mentionned above is a bit special in the sense
that in only requires to trap AArch32 accesses, and 64bit tasks can be
left alone. Consequently, the vdso is never disabled and AArch32 tasks
are affected.

Obviously, we really want to retain the 64bit vdso in this case. To
that effect, this series offers a way to disable the 32bit view of the
vdso without impacting its 64bit counterpart, by providing a
"no-compat" vdso clock_mode, and plugging this feature into the
1418040 detection code.

Lastly, I've tagged a rework of the 1414080 workaround (which had been
posted separately) at the end of the series so that it limits its
effect to 32bit tasks exclusively (so far, it forces the userspace
access bit on 64bit tasks, and we may need to leave it disabled in the
future...).

* From v1:
  - Reworked following Mark's feedback (patches #2 and #3)
  - Reworked patch #4 after Will's comments
  - patches #1 to #3 are now cc stable
  - Applied Mark's AB to patch #1

Marc Zyngier (4):
  arm64: Introduce a way to disable the 32bit vdso
  arm64: arch_timer: Allow an workaround descriptor to disable compat
    vdso
  arm64: arch_timer: Disable the compat vdso for cores affected by
    ARM64_WORKAROUND_1418040
  arm64: Rework ARM_ERRATUM_1414080 handling

 arch/arm64/include/asm/arch_timer.h           |  1 +
 arch/arm64/include/asm/vdso/clocksource.h     |  7 +++-
 .../include/asm/vdso/compat_gettimeofday.h    |  8 +++-
 arch/arm64/kernel/entry.S                     | 40 +++++++++++--------
 drivers/clocksource/arm_arch_timer.c          | 11 +++++
 5 files changed, 48 insertions(+), 19 deletions(-)

-- 
2.27.0

[PATCH v2 1/4] arm64: Introduce a way to disable the 32bit vdso

[Marc Zyngier]

We have a class of errata (grouped under the ARM64_WORKAROUND_1418040
banner) that force the trapping of counter access from 32bit EL0.

We would normally disable the whole vdso for such defect, except that
it would disable it for 64bit userspace as well, which is a shame.

Instead, add a new vdso_clock_mode, which signals that the vdso
isn't usable for compat tasks.  This gets checked in the new
vdso_clocksource_ok() helper, now provided for the 32bit vdso.

Cc: stable@vger.kernel.org
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/vdso/clocksource.h         | 7 +++++--
 arch/arm64/include/asm/vdso/compat_gettimeofday.h | 8 +++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/vdso/clocksource.h b/arch/arm64/include/asm/vdso/clocksource.h
index df6ea65c1dec..b054d9febfb5 100644
--- a/arch/arm64/include/asm/vdso/clocksource.h
+++ b/arch/arm64/include/asm/vdso/clocksource.h
@@ -2,7 +2,10 @@
 #ifndef __ASM_VDSOCLOCKSOURCE_H
 #define __ASM_VDSOCLOCKSOURCE_H
 
-#define VDSO_ARCH_CLOCKMODES	\
-	VDSO_CLOCKMODE_ARCHTIMER
+#define VDSO_ARCH_CLOCKMODES					\
+	/* vdso clocksource for both 32 and 64bit tasks */	\
+	VDSO_CLOCKMODE_ARCHTIMER,				\
+	/* vdso clocksource for 64bit tasks only */		\
+	VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT
 
 #endif
diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
index b6907ae78e53..9a625e8947ff 100644
--- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
+++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
@@ -111,7 +111,7 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
 	 * update. Return something. Core will do another round and then
 	 * see the mode change and fallback to the syscall.
 	 */
-	if (clock_mode == VDSO_CLOCKMODE_NONE)
+	if (clock_mode != VDSO_CLOCKMODE_ARCHTIMER)
 		return 0;
 
 	/*
@@ -152,6 +152,12 @@ static __always_inline const struct vdso_data *__arch_get_vdso_data(void)
 	return ret;
 }
 
+static inline bool vdso_clocksource_ok(const struct vdso_data *vd)
+{
+	return vd->clock_mode == VDSO_CLOCKMODE_ARCHTIMER;
+}
+#define vdso_clocksource_ok	vdso_clocksource_ok
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_VDSO_GETTIMEOFDAY_H */
-- 
2.27.0

[PATCH v2 2/4] arm64: arch_timer: Allow an workaround descriptor to disable compat vdso

[Marc Zyngier]

As we are about to disable the vdso for compat tasks in some circumstances,
let's allow a workaround descriptor to express exactly that.

Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/arch_timer.h  | 1 +
 drivers/clocksource/arm_arch_timer.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 7ae54d7d333a..9f0ec21d6327 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -58,6 +58,7 @@ struct arch_timer_erratum_workaround {
 	u64 (*read_cntvct_el0)(void);
 	int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
 	int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
+	bool disable_compat_vdso;
 };
 
 DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index ecf7b7db2d05..a8e4fb429f52 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
 	if (wa->read_cntvct_el0) {
 		clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
 		vdso_default = VDSO_CLOCKMODE_NONE;
+	} else if (wa->disable_compat_vdso && vdso_default != VDSO_CLOCKMODE_NONE) {
+		vdso_default = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
+		clocksource_counter.vdso_clock_mode = vdso_default;
 	}
 }
 
-- 
2.27.0

Re: [PATCH v2 2/4] arm64: arch_timer: Allow an workaround descriptor to disable compat vdso

[Mark Rutland]

On Mon, Jul 06, 2020 at 05:38:00PM +0100, Marc Zyngier wrote:
> As we are about to disable the vdso for compat tasks in some circumstances,
> let's allow a workaround descriptor to express exactly that.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/arch_timer.h  | 1 +
>  drivers/clocksource/arm_arch_timer.c | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 7ae54d7d333a..9f0ec21d6327 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -58,6 +58,7 @@ struct arch_timer_erratum_workaround {
>  	u64 (*read_cntvct_el0)(void);
>  	int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
>  	int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
> +	bool disable_compat_vdso;
>  };
>  
>  DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index ecf7b7db2d05..a8e4fb429f52 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
>  	if (wa->read_cntvct_el0) {
>  		clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
>  		vdso_default = VDSO_CLOCKMODE_NONE;
> +	} else if (wa->disable_compat_vdso && vdso_default != VDSO_CLOCKMODE_NONE) {
> +		vdso_default = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
> +		clocksource_counter.vdso_clock_mode = vdso_default;
>  	}
>  }
>  
> -- 
> 2.27.0
> 

[PATCH v2 3/4] arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040

[Marc Zyngier]

ARM64_WORKAROUND_1418040 requires that AArch32 EL0 accesses to
the virtual counter register are trapped and emulated by the kernel.
This makes the vdso pretty pointless, and in some cases livelock
prone.

Provide a workaround entry that limits the vdso to 64bit tasks.

Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/clocksource/arm_arch_timer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index a8e4fb429f52..6c3e84180146 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -480,6 +480,14 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
 		.set_next_event_virt = erratum_set_next_event_tval_virt,
 	},
 #endif
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+	{
+		.match_type = ate_match_local_cap_id,
+		.id = (void *)ARM64_WORKAROUND_1418040,
+		.desc = "ARM erratum 1418040",
+		.disable_compat_vdso = true,
+	},
+#endif
 };
 
 typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *,
-- 
2.27.0

Re: [PATCH v2 3/4] arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040

[Mark Rutland]

On Mon, Jul 06, 2020 at 05:38:01PM +0100, Marc Zyngier wrote:
> ARM64_WORKAROUND_1418040 requires that AArch32 EL0 accesses to
> the virtual counter register are trapped and emulated by the kernel.
> This makes the vdso pretty pointless, and in some cases livelock
> prone.
> 
> Provide a workaround entry that limits the vdso to 64bit tasks.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/clocksource/arm_arch_timer.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index a8e4fb429f52..6c3e84180146 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -480,6 +480,14 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
>  		.set_next_event_virt = erratum_set_next_event_tval_virt,
>  	},
>  #endif
> +#ifdef CONFIG_ARM64_ERRATUM_1418040
> +	{
> +		.match_type = ate_match_local_cap_id,
> +		.id = (void *)ARM64_WORKAROUND_1418040,
> +		.desc = "ARM erratum 1418040",
> +		.disable_compat_vdso = true,
> +	},
> +#endif
>  };
>  
>  typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *,
> -- 
> 2.27.0
> 

[PATCH v2 4/4] arm64: Rework ARM_ERRATUM_1414080 handling

[Marc Zyngier]

The current handling of erratum 1414080 has the side effect that
cntkctl_el1 can get changed for both 32 and 64bit tasks.

This isn't a problem so far, but if we ever need to mitigate another
of these errata on the 64bit side, we'd better keep the messing with
cntkctl_el1 local to 32bit tasks.

For that, make sure that on entering the kernel from a 32bit tasks,
userspace access to cntvct gets enabled, and disabled returning to
userspace, while it never gets changed for 64bit tasks.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kernel/entry.S | 40 +++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 5304d193c79d..8f51f3273bc7 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -167,6 +167,19 @@ alternative_cb_end
 	stp	x28, x29, [sp, #16 * 14]
 
 	.if	\el == 0
+	.if	\regsize == 32
+	// If we come back from a 32bit task on a system affected by
+	// 1418040, let's reenable userspace access to the virtual counter.
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+alternative_if_not ARM64_WORKAROUND_1418040
+	b	.L__entry_skip_wa_1418040\@
+alternative_else_nop_endif
+	mrs	x0, cntkctl_el1
+	orr	x0, x0, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
+	msr	cntkctl_el1, x0
+.L__entry_skip_wa_1418040\@:
+#endif
+	.endif
 	clear_gp_regs
 	mrs	x21, sp_el0
 	ldr_this_cpu	tsk, __entry_task, x20
@@ -318,7 +331,17 @@ alternative_else_nop_endif
 	ldr	x23, [sp, #S_SP]		// load return stack pointer
 	msr	sp_el0, x23
 	tst	x22, #PSR_MODE32_BIT		// native task?
-	b.eq	3f
+	b.eq	4f
+
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+alternative_if_not ARM64_WORKAROUND_1418040
+	b	3f
+alternative_else_nop_endif
+	mrs	x1, cntkctl_el1
+	bic	x1, x1, #2			// ARCH_TIMER_USR_VCT_ACCESS_EN
+	msr	cntkctl_el1, x1
+3:
+#endif
 
 #ifdef CONFIG_ARM64_ERRATUM_845719
 alternative_if ARM64_WORKAROUND_845719
@@ -330,22 +353,7 @@ alternative_if ARM64_WORKAROUND_845719
 #endif
 alternative_else_nop_endif
 #endif
-3:
-#ifdef CONFIG_ARM64_ERRATUM_1418040
-alternative_if_not ARM64_WORKAROUND_1418040
-	b	4f
-alternative_else_nop_endif
-	/*
-	 * if (x22.mode32 == cntkctl_el1.el0vcten)
-	 *     cntkctl_el1.el0vcten = ~cntkctl_el1.el0vcten
-	 */
-	mrs	x1, cntkctl_el1
-	eon	x0, x1, x22, lsr #3
-	tbz	x0, #1, 4f
-	eor	x1, x1, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
-	msr	cntkctl_el1, x1
 4:
-#endif
 	scs_save tsk, x0
 
 	/* No kernel C function calls after this as user keys are set. */
-- 
2.27.0

Re: [PATCH v2 4/4] arm64: Rework ARM_ERRATUM_1414080 handling

[Mark Rutland]

On Mon, Jul 06, 2020 at 05:38:02PM +0100, Marc Zyngier wrote:
> The current handling of erratum 1414080 has the side effect that
> cntkctl_el1 can get changed for both 32 and 64bit tasks.
> 
> This isn't a problem so far, but if we ever need to mitigate another
> of these errata on the 64bit side, we'd better keep the messing with
> cntkctl_el1 local to 32bit tasks.
> 
> For that, make sure that on entering the kernel from a 32bit tasks,
> userspace access to cntvct gets enabled, and disabled returning to
> userspace, while it never gets changed for 64bit tasks.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>

The asm looks sound to me. Suggestion below, but either way:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

> ---
>  arch/arm64/kernel/entry.S | 40 +++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 5304d193c79d..8f51f3273bc7 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -167,6 +167,19 @@ alternative_cb_end
>  	stp	x28, x29, [sp, #16 * 14]
>  
>  	.if	\el == 0
> +	.if	\regsize == 32
> +	// If we come back from a 32bit task on a system affected by
> +	// 1418040, let's reenable userspace access to the virtual counter.
> +#ifdef CONFIG_ARM64_ERRATUM_1418040
> +alternative_if_not ARM64_WORKAROUND_1418040
> +	b	.L__entry_skip_wa_1418040\@
> +alternative_else_nop_endif
> +	mrs	x0, cntkctl_el1
> +	orr	x0, x0, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
> +	msr	cntkctl_el1, x0
> +.L__entry_skip_wa_1418040\@:
> +#endif

Given this is only 3 instructions, it might be clearer to remove the branch
and label and NOP the whole thing for legibility:

| #ifdef CONFIG_ARM64_ERRATUM_1418040
| alternative_if ARM64_WORKAROUND_1418040
| 	mrs	x0. cntkctl_el1
| 	orr	x0, x0, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
| 	msr	cntkctl_el1, x0
| alternative_else_nop_endif
| #endif

... and likewise for the clearing case.

Regardless, this looks sound to me, so take your pick. :)

Mark.

> +	.endif
>  	clear_gp_regs
>  	mrs	x21, sp_el0
>  	ldr_this_cpu	tsk, __entry_task, x20
> @@ -318,7 +331,17 @@ alternative_else_nop_endif
>  	ldr	x23, [sp, #S_SP]		// load return stack pointer
>  	msr	sp_el0, x23
>  	tst	x22, #PSR_MODE32_BIT		// native task?
> -	b.eq	3f
> +	b.eq	4f
> +
> +#ifdef CONFIG_ARM64_ERRATUM_1418040
> +alternative_if_not ARM64_WORKAROUND_1418040
> +	b	3f
> +alternative_else_nop_endif
> +	mrs	x1, cntkctl_el1
> +	bic	x1, x1, #2			// ARCH_TIMER_USR_VCT_ACCESS_EN
> +	msr	cntkctl_el1, x1
> +3:
> +#endif
>  
>  #ifdef CONFIG_ARM64_ERRATUM_845719
>  alternative_if ARM64_WORKAROUND_845719
> @@ -330,22 +353,7 @@ alternative_if ARM64_WORKAROUND_845719
>  #endif
>  alternative_else_nop_endif
>  #endif
> -3:
> -#ifdef CONFIG_ARM64_ERRATUM_1418040
> -alternative_if_not ARM64_WORKAROUND_1418040
> -	b	4f
> -alternative_else_nop_endif
> -	/*
> -	 * if (x22.mode32 == cntkctl_el1.el0vcten)
> -	 *     cntkctl_el1.el0vcten = ~cntkctl_el1.el0vcten
> -	 */
> -	mrs	x1, cntkctl_el1
> -	eon	x0, x1, x22, lsr #3
> -	tbz	x0, #1, 4f
> -	eor	x1, x1, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
> -	msr	cntkctl_el1, x1
>  4:
> -#endif
>  	scs_save tsk, x0
>  
>  	/* No kernel C function calls after this as user keys are set. */
> -- 
> 2.27.0
> 

Re: [PATCH v2 0/4] arm64: Allow the compat vdso to be disabled at runtime

[Will Deacon]

On Mon, 6 Jul 2020 17:37:58 +0100, Marc Zyngier wrote:
> The relatively recent introduction of the compat vdso on arm64 has
> overlooked its interactions with some of the interesting errata
> workarounds, such as ARM64_ERRATUM_1418040 (and its older 1188873
> incarnation).
> 
> This erratum requires the 64bit kernel to trap 32bit accesses to the
> virtual counter and emulate it. When the workaround was introduced,
> the compat vdso simply wasn't a thing. Now that the patches have
> landed in mainline, we trap the CVTVCT accesses from the vdso.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/4] arm64: Introduce a way to disable the 32bit vdso
      https://git.kernel.org/arm64/c/97884ca8c292
[2/4] arm64: arch_timer: Allow an workaround descriptor to disable compat vdso
      https://git.kernel.org/arm64/c/c1fbec4ac0d7
[3/4] arm64: arch_timer: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040
      https://git.kernel.org/arm64/c/4b661d6133c5
[4/4] arm64: Rework ARM_ERRATUM_1414080 handling
      https://git.kernel.org/arm64/c/dc802f2bc020

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev