Re: [PATCH] arm64: kernel: memory corruptions due non-disabled PAN

[Mark Rutland <mark.rutland@arm.com>]

Hi Pavel,

On Tue, Nov 19, 2019 at 05:10:06PM -0500, Pavel Tatashin wrote:
> Userland access functions (__arch_clear_user, __arch_copy_from_user,
> __arch_copy_in_user, __arch_copy_to_user), enable and disable PAN
> for the duration of copy. However, when copy fails for some reason,
> i.e. access violation the code is transferred to fixedup section,
> where we do not disable PAN.

Thanks for reporting this. This is a very nasty bug.

> The bug is a security violation as the access to userland is still
> open when it should be disabled, but it also causes memory corruptions
> when software emulated PAN is used: CONFIG_ARM64_SW_TTBR0_PAN=y.

I see that with CONFIG_ARM64_SW_TTBR0_PAN=y, this means that we may
leave the stale TTBR0 value installed across a context-switch (and have
reproduced that locally), but I'm having some difficulty reproducing the
corruption that you see.

> I was able to reproduce memory corruption problem on Broadcom's SoC
> ARMv8-A like this:
> 
> Enable software perf-events with PERF_SAMPLE_CALLCHAIN so userland's
> stack is accessed and copied.

IIUC this tickles the issue by performing a faulting uaccess in IRQ
context. On the path to returnign from the exception, it directly calls
into the scheduler as part of el1_preempt, erroneously passing the TTBR0
value to the next task. Note that a preemption would remove the stale
TTBR0 value as part of kernel entry.

It looks like if we're in this state, and return from an exception taken
from EL1 with SW PAN enabled, we'll also leave the stale TTBR0 value
installed. If PAN was disabled (e.g. in the middle of a uaccess region),
then we'll restore the correct TTBR0.

> The test program performed the following on every CPU and forking many
> processes:
> 
> 	unsigned long *map = mmap(NULL, PAGE_SIZE, PROT_READ|PROT_WRITE,
> 				  MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> 	map[0] = getpid();
> 	sched_yield();
> 	if (map[0] != getpid()) {
> 		fprintf(stderr, "Corruption detected!");
> 	}
> 	munmap(map, PAGE_SIZE);

Can you provide the whole test, please? And precisely how you're
launching it?

I've tried turning the above into a main() function, and spawning a
number of instances in parallel while perf is running, but I haven't
been able to reproduce the issue locally, and I'm concerned that I'm
missing something.

> From time to time I was getting map[0] to contain pid for a different
> process.

How often is "from time to time"? How many processes are you running,
across how many CPUs?

> 
> Fixes: 338d4f49d6f7114 ("arm64: kernel: Add support for Privileged...")
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>  arch/arm64/lib/clear_user.S     | 1 +
>  arch/arm64/lib/copy_from_user.S | 1 +
>  arch/arm64/lib/copy_in_user.S   | 1 +
>  arch/arm64/lib/copy_to_user.S   | 1 +
>  4 files changed, 4 insertions(+)

FWIW, the diff below looks correct to me, but we might want to fold this
into the C wrappers, so that this is consistent with the other uaccess
cases (and done in one place in the code).

Thanks,
Mark.

> 
> diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S
> index 10415572e82f..322b55664cca 100644
> --- a/arch/arm64/lib/clear_user.S
> +++ b/arch/arm64/lib/clear_user.S
> @@ -48,5 +48,6 @@ EXPORT_SYMBOL(__arch_clear_user)
>  	.section .fixup,"ax"
>  	.align	2
>  9:	mov	x0, x2			// return the original size
> +	uaccess_disable_not_uao x2, x3
>  	ret
>  	.previous
> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
> index 680e74409ff9..8472dc7798b3 100644
> --- a/arch/arm64/lib/copy_from_user.S
> +++ b/arch/arm64/lib/copy_from_user.S
> @@ -66,5 +66,6 @@ EXPORT_SYMBOL(__arch_copy_from_user)
>  	.section .fixup,"ax"
>  	.align	2
>  9998:	sub	x0, end, dst			// bytes not copied
> +	uaccess_disable_not_uao x3, x4
>  	ret
>  	.previous
> diff --git a/arch/arm64/lib/copy_in_user.S b/arch/arm64/lib/copy_in_user.S
> index 0bedae3f3792..8e0355c1e318 100644
> --- a/arch/arm64/lib/copy_in_user.S
> +++ b/arch/arm64/lib/copy_in_user.S
> @@ -68,5 +68,6 @@ EXPORT_SYMBOL(__arch_copy_in_user)
>  	.section .fixup,"ax"
>  	.align	2
>  9998:	sub	x0, end, dst			// bytes not copied
> +	uaccess_disable_not_uao x3, x4
>  	ret
>  	.previous
> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 2d88c736e8f2..6085214654dc 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -65,5 +65,6 @@ EXPORT_SYMBOL(__arch_copy_to_user)
>  	.section .fixup,"ax"
>  	.align	2
>  9998:	sub	x0, end, dst			// bytes not copied
> +	uaccess_disable_not_uao x3, x4
>  	ret
>  	.previous
> -- 
> 2.24.0
>