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

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

[Pavel Tatashin]

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.

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 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.

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);

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

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(+)

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

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

[Mark Rutland]

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
> 

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

[Pavel Tatashin]

On Wed, Nov 20, 2019 at 11:43 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> 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.

Indeed, it was biting us randomly, and it took me awhile to understand
the root cause.

>
> > 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 will send the full test shortly. Note, I was never able to reproduce
it in QEMU, only on real hardware. Also, for some unknown reason after
kexec I could not reproduce it only during first boot, so it is
somewhat fragile, but I am sure it can be reproduced in other cases as
well, it is just my reproducer is not tunes for that.

>
> > 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.

Correct.

>
> 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 will shortly.

>
> 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?

Less than a second on 8 CPU SoC it takes for a process to get access
to address space of another process.

>
> >
> > 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).

I agree, and I actually have a patch for that, but I wanted my fix to
be included into 5.4 if possible. This is why I sent it out. I will
send out a C wrapper patch soon, but that one won't need to be
backported to stable.

Pasha

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

[Will Deacon]

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.
> 
> 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 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.
> 
> 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);
> 
> From time to time I was getting map[0] to contain pid for a different
> process.
> 
> 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(+)

Thanks. I've pushed this and your other patch out [1], with some changes
to the commit message. I'm annoyed that I didn't spot this during review
of the initial PAN patches.

Will

[1] https://fixes.arm64.dev

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

[Pavel Tatashin]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

> > 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 will send the full test shortly. Note, I was never able to reproduce
> it in QEMU, only on real hardware. Also, for some unknown reason after
> kexec I could not reproduce it only during first boot, so it is
> somewhat fragile, but I am sure it can be reproduced in other cases as
> well, it is just my reproducer is not tunes for that.
>

Attached is the test program that I used to reproduce memory corruption.
Test on board with Broadcom's Stingray SoC.

Without fix:
# time /tmp/repro
Corruption: pid 1474 map[0] 1488 cpu 3
Terminated

real    0m0.088s
user    0m0.004s
sys     0m0.071s

With the fix:

# time /tmp/repro
Test passed, all good
Terminated

real    1m1.286s
user    0m0.004s
sys     0m0.970s



Pasha

[-- Attachment #2: repro.c --]
[-- Type: text/x-csrc, Size: 2440 bytes --]

#define _GNU_SOURCE
#include <linux/perf_event.h>
#include <sys/mman.h>
#include <sys/sysinfo.h>
#include <sys/ioctl.h>
#include <sys/syscall.h>
#include <sys/wait.h>
#include <string.h>
#include <stdlib.h>
#include <signal.h>
#include <stdio.h>
#include <sched.h>
#include <time.h>
#include <unistd.h>

#define RUN_TIME	60
#define SIZE		4096
#define NPROCS		4096
#define NCPU		get_nprocs_conf()
#define PAGEMAP_LENGTH 8
unsigned long get_pa(void *addr) {
   FILE *pagemap = fopen("/proc/self/pagemap", "rb");
   unsigned long offset = (unsigned long)addr / getpagesize() * PAGEMAP_LENGTH;
   unsigned long pfn = 0;

   if(fseek(pagemap, (unsigned long)offset, SEEK_SET) != 0) {
      fprintf(stderr, "Failed to seek pagemap to proper location\n");
      exit(1);
   }

   fread(&pfn, 1, PAGEMAP_LENGTH-1, pagemap);

   pfn &= 0x7FFFFFFFFFFFFF;

   fclose(pagemap);

   return pfn * getpagesize();
}

int
do_work()
{
	int *map, pid;
	unsigned long pa;

	if (fork())
		exit(0);

	pid = getpid();
	map = mmap(NULL, SIZE, PROT_READ|PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
		   -1, 0);
	map[0] = pid;
	sched_yield();
	if (map[0] != pid) {
		fprintf(stderr, "Corruption: pid %d map[0] %d cpu %d\n",
			pid, map[0], sched_getcpu());
		kill(0, SIGTERM);
		return 1;
	}
	munmap(map, SIZE);
	return 0;
}

static void event_open(struct perf_event_attr *ctx_event_attr, int config)
{
	int i, fd;
	ctx_event_attr->config = config;
	for (i = 0; i < NCPU; i++) {
		fd = syscall(__NR_perf_event_open, ctx_event_attr,
			-1, i, -1, 0);
		ioctl(fd, PERF_EVENT_IOC_ENABLE, 0);
	}
}

static void
perf_events()
{
	struct perf_event_attr ctx_event_attr;

	memset(&ctx_event_attr, 0, sizeof(struct perf_event_attr));
	ctx_event_attr.type = PERF_TYPE_SOFTWARE;
	ctx_event_attr.size = sizeof (struct perf_event_attr);
	ctx_event_attr.sample_period = 1;
	ctx_event_attr.sample_type = PERF_SAMPLE_CALLCHAIN;

	event_open(&ctx_event_attr, PERF_COUNT_SW_CPU_CLOCK);
}

int
main(int argc, char **argv)
{
	pid_t p[NPROCS];
	int i, fd;
	cpu_set_t  mask;

	CPU_ZERO(&mask);
	for (i = 0; i < NCPU; i++)
		CPU_SET(i, &mask);
	sched_setaffinity(0, sizeof(mask), &mask);

	perf_events();
	for (i = 0; i < NPROCS; i++) {
		p[i] = fork();
		if (p[i] == 0) {
			for (;;) {
				if (do_work()) {
					fprintf(stderr, "Bug is detected\n");
					kill(0, SIGTERM);
					exit(1);
				}
			}
		}
	}

	sleep(RUN_TIME);
	printf("Test passed, all good\n");
	kill(0, SIGTERM);
	return 0;
}

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

[Pavel Tatashin]

>
> Thanks. I've pushed this and your other patch out [1], with some changes
> to the commit message. I'm annoyed that I didn't spot this during review
> of the initial PAN patches.
>
> Will

Great.

Thank you,
Pasha

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

[Pavel Tatashin]

On Wed, Nov 20, 2019 at 2:46 PM Pavel Tatashin
<pasha.tatashin@soleen.com> wrote:
>
> > > 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 will send the full test shortly. Note, I was never able to reproduce
> > it in QEMU, only on real hardware. Also, for some unknown reason after
> > kexec I could not reproduce it only during first boot, so it is
> > somewhat fragile, but I am sure it can be reproduced in other cases as
> > well, it is just my reproducer is not tunes for that.
> >
>
> Attached is the test program that I used to reproduce memory corruption.
> Test on board with Broadcom's Stingray SoC.

I forgot to remove from repro.c some of the stuff that I used for debugging:
get_pa() and sched_setaffinity() stuff are not needed.

>
> Without fix:
> # time /tmp/repro
> Corruption: pid 1474 map[0] 1488 cpu 3
> Terminated
>
> real    0m0.088s
> user    0m0.004s
> sys     0m0.071s
>
> With the fix:
>
> # time /tmp/repro
> Test passed, all good
> Terminated
>
> real    1m1.286s
> user    0m0.004s
> sys     0m0.970s
>
>
>
> Pasha