[RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Bharata B Rao]

Hi,

This patchset makes use of Upper Address Ignore (UAI) feature available
on upcoming AMD processors to provide user address tagging support for x86/AMD.

UAI allows software to store a tag in the upper 7 bits of a logical
address [63:57]. When enabled, the processor will suppress the
traditional canonical address checks on the addresses. More information
about UAI can be found in section 5.10 of 'AMD64 Architecture
Programmer's Manual, Vol 2: System Programming' which is available from

https://bugzilla.kernel.org/attachment.cgi?id=300549

Currently ARM64 provides a way for processes to opt-in for
relaxed tagged ABI via prctl() options PR_SET/GET_TAGGED_ADDR_CTRL.
The prctl() API was found to be a bit restrictive for x86 use and
Kirill had posted an extension to it as part of Intel LAM patchset.
(https://lore.kernel.org/linux-mm/20210205151631.43511-1-kirill.shutemov@linux.intel.com/)

This patchset builds on that prctl() extension and adds support
for AMD UAI. AMD implementation is kept separate as equivalent
Intel LAM implementation is likely to be different due to different
bit positions and tag width.

This is an early implementation which has been only lightly tested.
I have used the tags_test.c from selftests/vm/tags/ to test this.
For ARM64 changes, I have only ensured that the changes compile.

Regards,
Bharata.

Bharata B Rao (5):
  x86/cpufeatures: Add Upper Address Ignore(UAI) as CPU feature
  x86: Enable Upper Address Ignore(UAI) feature
  x86: Provide an implementation of untagged_addr()
  x86: Untag user pointers in access_ok()
  x86: Add prctl() options to control tagged user addresses ABI

Kirill A. Shutemov (1):
  mm, arm64: Update PR_SET/GET_TAGGED_ADDR_CTRL interface

 arch/arm64/include/asm/processor.h            |  12 +-
 arch/arm64/kernel/process.c                   |  45 +++++-
 arch/arm64/kernel/ptrace.c                    |   4 +-
 arch/x86/Kconfig                              |   9 ++
 arch/x86/include/asm/cpufeatures.h            |   2 +-
 arch/x86/include/asm/msr-index.h              |   2 +
 arch/x86/include/asm/page_32.h                |   3 +
 arch/x86/include/asm/page_64.h                |  26 ++++
 arch/x86/include/asm/processor.h              |  12 ++
 arch/x86/include/asm/thread_info.h            |   2 +
 arch/x86/include/asm/uaccess.h                |  29 +++-
 arch/x86/kernel/cpu/scattered.c               |   1 +
 arch/x86/kernel/process.c                     | 134 ++++++++++++++++++
 arch/x86/kernel/setup.c                       |   8 ++
 kernel/sys.c                                  |  14 +-
 .../testing/selftests/arm64/tags/tags_test.c  |  31 ----
 .../selftests/{arm64 => vm}/tags/.gitignore   |   0
 .../selftests/{arm64 => vm}/tags/Makefile     |   0
 .../{arm64 => vm}/tags/run_tags_test.sh       |   0
 tools/testing/selftests/vm/tags/tags_test.c   |  59 ++++++++
 20 files changed, 335 insertions(+), 58 deletions(-)
 delete mode 100644 tools/testing/selftests/arm64/tags/tags_test.c
 rename tools/testing/selftests/{arm64 => vm}/tags/.gitignore (100%)
 rename tools/testing/selftests/{arm64 => vm}/tags/Makefile (100%)
 rename tools/testing/selftests/{arm64 => vm}/tags/run_tags_test.sh (100%)
 create mode 100644 tools/testing/selftests/vm/tags/tags_test.c

-- 
2.25.1

[RFC PATCH v0 1/6] mm, arm64: Update PR_SET/GET_TAGGED_ADDR_CTRL interface

[Bharata B Rao]

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

The interface for enabling tagged addresses is very inflexible. It
implies tag size and tag shift implemented by ARM TBI.

Rework the interface to accommodate different shifts and tag sizes.

PR_SET_TAGGED_ADDR_CTRL now accepts two new arguments:

 - nr_bits is pointer to int. The caller specifies the tag size it
   wants. Kernel updates the value of actual tag size that can be
   larger.

 - offset is pointer to int. Kernel returns there a shift of tag in the
   address.

The change doesn't break existing users of the interface: if any of
these pointers are NULL (as we had before the change), the user expects
ARM TBI implementation: nr_bits == 8 && offset == 56 as it was implied
before.

The initial implementation checked that these argument are NULL and the
change wouldn't not break any legacy users.

If tagging is enabled, GET_TAGGED_ADDR_CTRL would return size of tags
and offset in the additional arguments.

If tagging is disable, GET_TAGGED_ADDR_CTRL would return the maximum tag
size in nr_bits.

The selftest is updated accordingly and moved out of arm64-specific
directory as we going to enable the interface on x86.

As alternative to this approach we could introduce a totally new API and
leave the legacy one as is. But it would slow down adoption: new
prctl(2) flag wound need to propogate to the userspace headers.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
	[selftests/vm/tags/tags_test.c: Set ptr tag only when tagging
	 is enabled and a couple of checkpatch warning fixes]
Signed-off-by: Bharata B Rao <bharata@amd.com>
---
 arch/arm64/include/asm/processor.h            | 12 ++--
 arch/arm64/kernel/process.c                   | 45 +++++++++++---
 arch/arm64/kernel/ptrace.c                    |  4 +-
 kernel/sys.c                                  | 14 +++--
 .../testing/selftests/arm64/tags/tags_test.c  | 31 ----------
 .../selftests/{arm64 => vm}/tags/.gitignore   |  0
 .../selftests/{arm64 => vm}/tags/Makefile     |  0
 .../{arm64 => vm}/tags/run_tags_test.sh       |  0
 tools/testing/selftests/vm/tags/tags_test.c   | 59 +++++++++++++++++++
 9 files changed, 115 insertions(+), 50 deletions(-)
 delete mode 100644 tools/testing/selftests/arm64/tags/tags_test.c
 rename tools/testing/selftests/{arm64 => vm}/tags/.gitignore (100%)
 rename tools/testing/selftests/{arm64 => vm}/tags/Makefile (100%)
 rename tools/testing/selftests/{arm64 => vm}/tags/run_tags_test.sh (100%)
 create mode 100644 tools/testing/selftests/vm/tags/tags_test.c

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 6f41b65f9962..c3936bebf006 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -367,10 +367,14 @@ extern void __init minsigstksz_setup(void);
 
 #ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
 /* PR_{SET,GET}_TAGGED_ADDR_CTRL prctl */
-long set_tagged_addr_ctrl(struct task_struct *task, unsigned long arg);
-long get_tagged_addr_ctrl(struct task_struct *task);
-#define SET_TAGGED_ADDR_CTRL(arg)	set_tagged_addr_ctrl(current, arg)
-#define GET_TAGGED_ADDR_CTRL()		get_tagged_addr_ctrl(current)
+long set_tagged_addr_ctrl(struct task_struct *task, unsigned long flags,
+			  int __user *nr_bits, int __user *offset);
+long get_tagged_addr_ctrl(struct task_struct *task,
+			  int __user *nr_bits, int __user *offset);
+#define SET_TAGGED_ADDR_CTRL(flags, nr_bits, offset)	\
+	set_tagged_addr_ctrl(current, flags, nr_bits, offset)
+#define GET_TAGGED_ADDR_CTRL(nr_bits, offset)		\
+	get_tagged_addr_ctrl(current, nr_bits, offset)
 #endif
 
 /*
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 5369e649fa79..fc0240f5ead0 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -621,15 +621,21 @@ void arch_setup_new_exec(void)
 }
 
 #ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
+
+#define TBI_TAG_BITS	8
+#define TBI_TAG_SHIFT	56
+
 /*
  * Control the relaxed ABI allowing tagged user addresses into the kernel.
  */
 static unsigned int tagged_addr_disabled;
 
-long set_tagged_addr_ctrl(struct task_struct *task, unsigned long arg)
+long set_tagged_addr_ctrl(struct task_struct *task, unsigned long flags,
+			  int __user *nr_bits, int __user *offset)
 {
 	unsigned long valid_mask = PR_TAGGED_ADDR_ENABLE;
 	struct thread_info *ti = task_thread_info(task);
+	int val;
 
 	if (is_compat_thread(ti))
 		return -EINVAL;
@@ -637,25 +643,41 @@ long set_tagged_addr_ctrl(struct task_struct *task, unsigned long arg)
 	if (system_supports_mte())
 		valid_mask |= PR_MTE_TCF_MASK | PR_MTE_TAG_MASK;
 
-	if (arg & ~valid_mask)
+	if (flags & ~valid_mask)
 		return -EINVAL;
 
+	if (nr_bits) {
+		if (get_user(val, nr_bits))
+			return -EFAULT;
+		if (val > TBI_TAG_BITS || val < 1)
+			return -EINVAL;
+	}
+
 	/*
 	 * Do not allow the enabling of the tagged address ABI if globally
 	 * disabled via sysctl abi.tagged_addr_disabled.
 	 */
-	if (arg & PR_TAGGED_ADDR_ENABLE && tagged_addr_disabled)
+	if (flags & PR_TAGGED_ADDR_ENABLE && tagged_addr_disabled)
 		return -EINVAL;
 
-	if (set_mte_ctrl(task, arg) != 0)
+	if (set_mte_ctrl(task, flags) != 0)
 		return -EINVAL;
 
-	update_ti_thread_flag(ti, TIF_TAGGED_ADDR, arg & PR_TAGGED_ADDR_ENABLE);
+	if (flags & PR_TAGGED_ADDR_ENABLE) {
+		if (nr_bits && put_user(TBI_TAG_BITS, nr_bits))
+			return -EFAULT;
+		if (offset && put_user(TBI_TAG_SHIFT, offset))
+			return -EFAULT;
+	}
+
+	update_ti_thread_flag(ti, TIF_TAGGED_ADDR,
+			      flags & PR_TAGGED_ADDR_ENABLE);
 
 	return 0;
 }
 
-long get_tagged_addr_ctrl(struct task_struct *task)
+long get_tagged_addr_ctrl(struct task_struct *task,
+			  int __user *nr_bits, int __user *offset)
 {
 	long ret = 0;
 	struct thread_info *ti = task_thread_info(task);
@@ -663,8 +685,17 @@ long get_tagged_addr_ctrl(struct task_struct *task)
 	if (is_compat_thread(ti))
 		return -EINVAL;
 
-	if (test_ti_thread_flag(ti, TIF_TAGGED_ADDR))
+	if (test_ti_thread_flag(ti, TIF_TAGGED_ADDR)) {
 		ret = PR_TAGGED_ADDR_ENABLE;
+		if (nr_bits && put_user(TBI_TAG_BITS, nr_bits))
+			return -EFAULT;
+		if (offset && put_user(TBI_TAG_SHIFT, offset))
+			return -EFAULT;
+	} else {
+		/* Report maximum tag size */
+		if (nr_bits && put_user(TBI_TAG_BITS, nr_bits))
+			return -EFAULT;
+	}
 
 	ret |= get_mte_ctrl(task);
 
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 39dbdfdc38d3..471fd40f7d4e 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1073,7 +1073,7 @@ static int tagged_addr_ctrl_get(struct task_struct *target,
 				const struct user_regset *regset,
 				struct membuf to)
 {
-	long ctrl = get_tagged_addr_ctrl(target);
+	long ctrl = get_tagged_addr_ctrl(target, NULL, NULL);
 
 	if (IS_ERR_VALUE(ctrl))
 		return ctrl;
@@ -1093,7 +1093,7 @@ static int tagged_addr_ctrl_set(struct task_struct *target, const struct
 	if (ret)
 		return ret;
 
-	return set_tagged_addr_ctrl(target, ctrl);
+	return set_tagged_addr_ctrl(target, ctrl, NULL, NULL);
 }
 #endif
 
diff --git a/kernel/sys.c b/kernel/sys.c
index 97dc9e5d6bf9..3af5c5098b3c 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -126,10 +126,10 @@
 # define PAC_GET_ENABLED_KEYS(a)	(-EINVAL)
 #endif
 #ifndef SET_TAGGED_ADDR_CTRL
-# define SET_TAGGED_ADDR_CTRL(a)	(-EINVAL)
+# define SET_TAGGED_ADDR_CTRL(a, b, c)	(-EINVAL)
 #endif
 #ifndef GET_TAGGED_ADDR_CTRL
-# define GET_TAGGED_ADDR_CTRL()		(-EINVAL)
+# define GET_TAGGED_ADDR_CTRL(a, b)	(-EINVAL)
 #endif
 
 /*
@@ -2557,14 +2557,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		error = PAC_GET_ENABLED_KEYS(me);
 		break;
 	case PR_SET_TAGGED_ADDR_CTRL:
-		if (arg3 || arg4 || arg5)
+		if (arg5)
 			return -EINVAL;
-		error = SET_TAGGED_ADDR_CTRL(arg2);
+		error = SET_TAGGED_ADDR_CTRL(arg2, (int __user *)arg3,
+					     (int __user *)arg4);
 		break;
 	case PR_GET_TAGGED_ADDR_CTRL:
-		if (arg2 || arg3 || arg4 || arg5)
+		if (arg4 || arg5)
 			return -EINVAL;
-		error = GET_TAGGED_ADDR_CTRL();
+		error = GET_TAGGED_ADDR_CTRL((int __user *)arg2,
+					     (int __user *)arg3);
 		break;
 	case PR_SET_IO_FLUSHER:
 		if (!capable(CAP_SYS_RESOURCE))
diff --git a/tools/testing/selftests/arm64/tags/tags_test.c b/tools/testing/selftests/arm64/tags/tags_test.c
deleted file mode 100644
index 5701163460ef..000000000000
--- a/tools/testing/selftests/arm64/tags/tags_test.c
+++ /dev/null
@@ -1,31 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <stdint.h>
-#include <sys/prctl.h>
-#include <sys/utsname.h>
-
-#define SHIFT_TAG(tag)		((uint64_t)(tag) << 56)
-#define SET_TAG(ptr, tag)	(((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
-					SHIFT_TAG(tag))
-
-int main(void)
-{
-	static int tbi_enabled = 0;
-	unsigned long tag = 0;
-	struct utsname *ptr;
-	int err;
-
-	if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
-		tbi_enabled = 1;
-	ptr = (struct utsname *)malloc(sizeof(*ptr));
-	if (tbi_enabled)
-		tag = 0x42;
-	ptr = (struct utsname *)SET_TAG(ptr, tag);
-	err = uname(ptr);
-	free(ptr);
-
-	return err;
-}
diff --git a/tools/testing/selftests/arm64/tags/.gitignore b/tools/testing/selftests/vm/tags/.gitignore
similarity index 100%
rename from tools/testing/selftests/arm64/tags/.gitignore
rename to tools/testing/selftests/vm/tags/.gitignore
diff --git a/tools/testing/selftests/arm64/tags/Makefile b/tools/testing/selftests/vm/tags/Makefile
similarity index 100%
rename from tools/testing/selftests/arm64/tags/Makefile
rename to tools/testing/selftests/vm/tags/Makefile
diff --git a/tools/testing/selftests/arm64/tags/run_tags_test.sh b/tools/testing/selftests/vm/tags/run_tags_test.sh
similarity index 100%
rename from tools/testing/selftests/arm64/tags/run_tags_test.sh
rename to tools/testing/selftests/vm/tags/run_tags_test.sh
diff --git a/tools/testing/selftests/vm/tags/tags_test.c b/tools/testing/selftests/vm/tags/tags_test.c
new file mode 100644
index 000000000000..c8486b6398b1
--- /dev/null
+++ b/tools/testing/selftests/vm/tags/tags_test.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <stdint.h>
+#include <sys/prctl.h>
+#include <sys/utsname.h>
+
+static int tag_bits;
+static int tag_offset;
+
+#define SHIFT_TAG(tag)		((uint64_t)(tag) << tag_offset)
+#define SET_TAG(ptr, tag)	(((uint64_t)(ptr) & ~SHIFT_TAG((1 << tag_bits) - 1)) \
+				| SHIFT_TAG(tag))
+
+static int max_tag_bits(void)
+{
+	int nr;
+
+	if (prctl(PR_GET_TAGGED_ADDR_CTRL, 0, 0, 0) < 0)
+		return 0;
+
+	if (prctl(PR_GET_TAGGED_ADDR_CTRL, &nr, 0, 0) < 0)
+		return 8; /* Assume ARM TBI */
+
+	return nr;
+}
+
+int main(void)
+{
+	static int tags_enabled;
+	unsigned long tag = 0;
+	struct utsname *ptr;
+	int err;
+
+	tag_bits = max_tag_bits();
+
+	if (tag_bits && !prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE,
+			       &tag_bits, &tag_offset, 0)) {
+		tags_enabled = 1;
+	} else if (tag_bits == 8 && !prctl(PR_SET_TAGGED_ADDR_CTRL,
+					   PR_TAGGED_ADDR_ENABLE, 0, 0)) {
+		/* ARM TBI with legacy interface*/
+		tags_enabled = 1;
+		tag_offset = 56;
+	}
+
+	ptr = (struct utsname *)malloc(sizeof(*ptr));
+	if (tags_enabled) {
+		tag = (1UL << tag_bits) - 1;
+		ptr = (struct utsname *)SET_TAG(ptr, tag);
+	}
+	err = uname(ptr);
+	printf("Sysname: %s\n", ptr->sysname);
+	free(ptr);
+
+	return err;
+}
-- 
2.25.1

[RFC PATCH v0 2/6] x86/cpufeatures: Add Upper Address Ignore(UAI) as CPU feature

[Bharata B Rao]

Currently the maximum logical address size for AMD processors in
64 bit mode is 57 bits. This means that the remaining 7 upper bits
[63:57] are available for software use. With UAI feature turned ON,
the processor ignores these upper bits when performing canonical
check on these addresses.

Add UAI as a CPU feature.

Signed-off-by: Bharata B Rao <bharata@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 2 +-
 arch/x86/kernel/cpu/scattered.c    | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 6db4e2932b3d..5f4e88e67feb 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -201,7 +201,7 @@
 #define X86_FEATURE_INVPCID_SINGLE	( 7*32+ 7) /* Effectively INVPCID && CR4.PCIDE=1 */
 #define X86_FEATURE_HW_PSTATE		( 7*32+ 8) /* AMD HW-PState */
 #define X86_FEATURE_PROC_FEEDBACK	( 7*32+ 9) /* AMD ProcFeedbackInterface */
-/* FREE!                                ( 7*32+10) */
+#define X86_FEATURE_UAI			( 7*32+10) /* AMD Upper Address Ignore */
 #define X86_FEATURE_PTI			( 7*32+11) /* Kernel Page Table Isolation enabled */
 #define X86_FEATURE_RETPOLINE		( 7*32+12) /* "" Generic Retpoline mitigation for Spectre variant 2 */
 #define X86_FEATURE_RETPOLINE_AMD	( 7*32+13) /* "" AMD Retpoline mitigation for Spectre variant 2 */
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 21d1f062895a..5c19f6f525cf 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -42,6 +42,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_CPB,		CPUID_EDX,  9, 0x80000007, 0 },
 	{ X86_FEATURE_PROC_FEEDBACK,    CPUID_EDX, 11, 0x80000007, 0 },
 	{ X86_FEATURE_MBA,		CPUID_EBX,  6, 0x80000008, 0 },
+	{ X86_FEATURE_UAI,		CPUID_EAX,  7, 0x80000021, 0 },
 	{ 0, 0, 0, 0, 0 }
 };
 
-- 
2.25.1

[RFC PATCH v0 3/6] x86: Enable Upper Address Ignore(UAI) feature

[Bharata B Rao]

UAI feature is enabled by setting bit 20 in EFER MSR.

Signed-off-by: Bharata B Rao <bharata@amd.com>
---
 arch/x86/include/asm/msr-index.h | 2 ++
 arch/x86/kernel/setup.c          | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index a4a39c3e0f19..ce763952278f 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -30,6 +30,7 @@
 #define _EFER_SVME		12 /* Enable virtualization */
 #define _EFER_LMSLE		13 /* Long Mode Segment Limit Enable */
 #define _EFER_FFXSR		14 /* Enable Fast FXSAVE/FXRSTOR */
+#define _EFER_UAI		20 /* Enable Upper Address Ignore */
 
 #define EFER_SCE		(1<<_EFER_SCE)
 #define EFER_LME		(1<<_EFER_LME)
@@ -38,6 +39,7 @@
 #define EFER_SVME		(1<<_EFER_SVME)
 #define EFER_LMSLE		(1<<_EFER_LMSLE)
 #define EFER_FFXSR		(1<<_EFER_FFXSR)
+#define EFER_UAI		(1<<_EFER_UAI)
 
 /* Intel MSRs. Some also available on other CPUs */
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f7a132eb794d..12615b1b4af5 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -740,6 +740,12 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
 	return 0;
 }
 
+static inline void __init uai_enable(void)
+{
+	if (boot_cpu_has(X86_FEATURE_UAI))
+		msr_set_bit(MSR_EFER, _EFER_UAI);
+}
+
 /*
  * Determine if we were loaded by an EFI loader.  If so, then we have also been
  * passed the efi memmap, systab, etc., so we should use these data structures
@@ -1146,6 +1152,8 @@ void __init setup_arch(char **cmdline_p)
 
 	x86_init.paging.pagetable_init();
 
+	uai_enable();
+
 	kasan_init();
 
 	/*
-- 
2.25.1

Re: [RFC PATCH v0 3/6] x86: Enable Upper Address Ignore(UAI) feature

[Andrew Cooper]

On 10/03/2022 11:15, Bharata B Rao wrote:
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index f7a132eb794d..12615b1b4af5 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -740,6 +740,12 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
>  	return 0;
>  }
>  
> +static inline void __init uai_enable(void)
> +{
> +	if (boot_cpu_has(X86_FEATURE_UAI))
> +		msr_set_bit(MSR_EFER, _EFER_UAI);
> +}
> +
>  /*
>   * Determine if we were loaded by an EFI loader.  If so, then we have also been
>   * passed the efi memmap, systab, etc., so we should use these data structures
> @@ -1146,6 +1152,8 @@ void __init setup_arch(char **cmdline_p)
>  
>  	x86_init.paging.pagetable_init();
>  
> +	uai_enable();

I would think incredibly carefully before enabling UAI by default.

Suffice it to say that Intel were talked down from 7 bits to 6, and
apparently AMD didn't get the same memo from the original requesters.

The problem is that UAI + LA57 means that all the poison pointers cease
functioning as a defence-in-depth mechanism, and become legal pointers
pointing at random positions in user or kernel space.

~Andrew

RE: [RFC PATCH v0 3/6] x86: Enable Upper Address Ignore(UAI) feature

[David Laight]

From: Andrew Cooper
> Sent: 10 March 2022 19:47
> 
> On 10/03/2022 11:15, Bharata B Rao wrote:
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index f7a132eb794d..12615b1b4af5 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -740,6 +740,12 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
> >  	return 0;
> >  }
> >
> > +static inline void __init uai_enable(void)
> > +{
> > +	if (boot_cpu_has(X86_FEATURE_UAI))
> > +		msr_set_bit(MSR_EFER, _EFER_UAI);
> > +}
> > +
> >  /*
> >   * Determine if we were loaded by an EFI loader.  If so, then we have also been
> >   * passed the efi memmap, systab, etc., so we should use these data structures
> > @@ -1146,6 +1152,8 @@ void __init setup_arch(char **cmdline_p)
> >
> >  	x86_init.paging.pagetable_init();
> >
> > +	uai_enable();
> 
> I would think incredibly carefully before enabling UAI by default.
> 
> Suffice it to say that Intel were talked down from 7 bits to 6, and
> apparently AMD didn't get the same memo from the original requesters.
> 
> The problem is that UAI + LA57 means that all the poison pointers cease
> functioning as a defence-in-depth mechanism, and become legal pointers
> pointing at random positions in user or kernel space.

Isn't that true regardless of how many bits are 'ignored'.
AFAICT the only sane thing would be to have something in the cpu
that verifies the 'ignored' bits match values set in the PTE.
That could be used to ensure (well make it more likely) that stack
access stay in the stack and pointers to mmap()ed data stay
pointing to the correct pages.

Just letting user address space be aliased a lot of times doesn't
seem like a security feature to me.
It must have some strange use case.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v0 3/6] x86: Enable Upper Address Ignore(UAI) feature

[Dave Hansen]

On 3/10/22 14:37, David Laight wrote:
> Just letting user address space be aliased a lot of times doesn't
> seem like a security feature to me.
> It must have some strange use case.

This should have been in the changelogs... sheesh...

Right now, address sanitizers keep pointer metadata in various spots.
But, it requires recompiling apps and libraries.  These compiler-based
things are also so slow that production use is rare.

These masking things (ARM TBI, AMD UAI, Intel LAM) _theoretically_ let
you plumb enough metadata around with pointers to do address sanitizer
implementations in production.

I think LAM is the most sane of the three, but I'm biased.

Re: [RFC PATCH v0 3/6] x86: Enable Upper Address Ignore(UAI) feature

[Boris Petkov]

>diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>index f7a132eb794d..12615b1b4af5 100644
>--- a/arch/x86/kernel/setup.c
>+++ b/arch/x86/kernel/setup.c
>@@ -740,6 +740,12 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
> 	return 0;
> }
> 
>+static inline void __init uai_enable(void)
>+{
>+	if (boot_cpu_has(X86_FEATURE_UAI))

cpu_feature_enabled

>+		msr_set_bit(MSR_EFER, _EFER_UAI);
>+}


-- 
Sent from a small device: formatting sux and brevity is inevitable.

[RFC PATCH v0 4/6] x86: Provide an implementation of untagged_addr()

[Bharata B Rao]

untagged_addr() will be used by core mm routines to remove
the tag bits and convert the address to canonical form.

Limit the implementation to AMD CPUs as Intel's version
of the same is likely to be different.

Signed-off-by: Bharata B Rao <bharata@amd.com>
---
 arch/x86/include/asm/page_32.h |  3 +++
 arch/x86/include/asm/page_64.h | 26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/arch/x86/include/asm/page_32.h b/arch/x86/include/asm/page_32.h
index df42f8aa99e4..8309aa987354 100644
--- a/arch/x86/include/asm/page_32.h
+++ b/arch/x86/include/asm/page_32.h
@@ -30,6 +30,9 @@ static inline void copy_page(void *to, void *from)
 {
 	memcpy(to, from, PAGE_SIZE);
 }
+
+#define untagged_addr(addr)	(addr)
+#define untagged_ptr(ptr)	(ptr)
 #endif	/* !__ASSEMBLY__ */
 
 #endif /* _ASM_X86_PAGE_32_H */
diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
index e9c86299b835..41d4a729e783 100644
--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -90,6 +90,32 @@ static __always_inline unsigned long task_size_max(void)
 }
 #endif	/* CONFIG_X86_5LEVEL */
 
+#ifdef CONFIG_CPU_SUP_AMD
+/*
+ * Tag bits are in same position [63:57] for both 4 and 5 level page
+ * tables. For both the cases we sign-extend from 56th bit since
+ * bits [56:48] are anyway expected to be canonical for 4 level page tables.
+ */
+#define __untagged_addr(addr)	\
+	((__force __typeof__(addr))sign_extend64((__force u64)(addr), 56))
+
+#define untagged_addr(addr)	({					\
+	u64 __addr = (__force u64)(addr);				\
+	__addr &= __untagged_addr(__addr);				\
+	(__force __typeof__(addr))__addr;				\
+})
+
+#define untagged_ptr(ptr)	({					\
+	u64 __ptrval = (__force u64)(ptr);				\
+	__ptrval = untagged_addr(__ptrval);				\
+	(__force __typeof__(*(ptr)) *)__ptrval;				\
+})
+
+#else
+#define untagged_addr(addr)	(addr)
+#define untagged_ptr(ptr)	(ptr)
+#endif
+
 #endif	/* !__ASSEMBLY__ */
 
 #ifdef CONFIG_X86_VSYSCALL_EMULATION
-- 
2.25.1

[RFC PATCH v0 5/6] x86: Untag user pointers in access_ok()

[Bharata B Rao]

This is a preparatory work to allow passing of tagged user
pointers (with a top few bits other than zeroes) as syscall
arguments.

Since user can provide tagged pointer to syscalls they need
to be untagged before validating them in access_ok().
Untagging is done only if flag TIF_TAGGED_ADDR is set for
the thread. This is set via prctl() option which will be
introduced in a subsequent patch.

get_user() and put_user() don't use access_ok(), but check
access against TASK_SIZE direcly in assembly. Strip tags,
before calling into the assembly helper.

[kirill.shutemov@linux.intel.com - get/put_user() changes]

Signed-off-by: Bharata B Rao <bharata@amd.com>
---
 arch/x86/include/asm/thread_info.h |  2 ++
 arch/x86/include/asm/uaccess.h     | 28 +++++++++++++++++++++-------
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index ebec69c35e95..c786103a5325 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -101,6 +101,7 @@ struct thread_info {
 #define TIF_BLOCKSTEP		25	/* set when we want DEBUGCTLMSR_BTF */
 #define TIF_LAZY_MMU_UPDATES	27	/* task is updating the mmu lazily */
 #define TIF_ADDR32		29	/* 32-bit address space on 64 bits */
+#define TIF_TAGGED_ADDR		30	/* Allow tagged user addresses */
 
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -124,6 +125,7 @@ struct thread_info {
 #define _TIF_BLOCKSTEP		(1 << TIF_BLOCKSTEP)
 #define _TIF_LAZY_MMU_UPDATES	(1 << TIF_LAZY_MMU_UPDATES)
 #define _TIF_ADDR32		(1 << TIF_ADDR32)
+#define _TIF_TAGGED_ADDR	(1 << TIF_TAGGED_ADDR)
 
 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW_BASE					\
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ac96f9b2d64b..feb2e21c7e09 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -35,11 +35,15 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un
 	return unlikely(addr > limit);
 }
 
-#define __range_not_ok(addr, size, limit)				\
-({									\
-	__chk_user_ptr(addr);						\
-	__chk_range_not_ok((unsigned long __force)(addr), size, limit); \
-})
+static inline bool __range_not_ok(const void __user *addr, unsigned long size,
+				  unsigned long limit)
+{
+	if (test_thread_flag(TIF_TAGGED_ADDR))
+		addr = untagged_addr(addr);
+
+	__chk_user_ptr(addr);
+	return __chk_range_not_ok((unsigned long __force)(addr), size, limit);
+}
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 static inline bool pagefault_disabled(void);
@@ -152,7 +156,12 @@ extern int __get_user_bad(void);
  * Return: zero on success, or -EFAULT on error.
  * On error, the variable @x is set to zero.
  */
-#define get_user(x,ptr) ({ might_fault(); do_get_user_call(get_user,x,ptr); })
+#define get_user(x,ptr)							\
+({									\
+	__typeof__(*(ptr)) *ptr_untagged = untagged_ptr(ptr);		\
+	might_fault();							\
+	do_get_user_call(get_user,x,ptr_untagged);			\
+})
 
 /**
  * __get_user - Get a simple variable from user space, with less checking.
@@ -249,7 +258,12 @@ extern void __put_user_nocheck_8(void);
  *
  * Return: zero on success, or -EFAULT on error.
  */
-#define put_user(x, ptr) ({ might_fault(); do_put_user_call(put_user,x,ptr); })
+#define put_user(x, ptr)						\
+({									\
+	__typeof__(*(ptr)) *ptr_untagged = untagged_ptr(ptr);		\
+	might_fault();							\
+	do_put_user_call(put_user,x,ptr_untagged);			\
+})
 
 /**
  * __put_user - Write a simple value into user space, with less checking.
-- 
2.25.1

[RFC PATCH v0 6/6] x86: Add prctl() options to control tagged user addresses ABI

[Bharata B Rao]

Provide an option for applications to opt-in for a relaxed
tagged ABI via prtcl(). This allows them to pass tagged addresses
to syscalls as pointer arguments.

The implementation is kept separate for AMD UAI as the equivalent
implementation for Intel LAM is likely to be different.

Signed-off-by: Bharata B Rao <bharata@amd.com>
---
 arch/x86/Kconfig                 |   9 +++
 arch/x86/include/asm/processor.h |  12 +++
 arch/x86/include/asm/uaccess.h   |   3 +-
 arch/x86/kernel/process.c        | 134 +++++++++++++++++++++++++++++++
 4 files changed, 157 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9f5bd41bf660..b73414eb1c01 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2436,6 +2436,15 @@ config STRICT_SIGALTSTACK_SIZE
 
 source "kernel/livepatch/Kconfig"
 
+config X86_TAGGED_ADDR_ABI
+	bool "Enable the tagged user addresses syscall ABI"
+	depends on X86_64 && CPU_SUP_AMD
+	default y
+	help
+	  When this option is enabled, user applications can opt-in to a
+	  relaxed ABI via prctl() allowing tagged addresses to be passed
+	  to system calls as pointer arguments.
+
 endmenu
 
 config ARCH_HAS_ADD_PAGES
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2c5f12ae7d04..414e2c039c34 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -863,4 +863,16 @@ bool arch_is_platform_page(u64 paddr);
 #define arch_is_platform_page arch_is_platform_page
 #endif
 
+#ifdef CONFIG_X86_TAGGED_ADDR_ABI
+/* PR_{SET,GET}_TAGGED_ADDR_CTRL prctl */
+long set_tagged_addr_ctrl(struct task_struct *task, unsigned long flags,
+			  int __user *nr_bits, int __user *offset);
+long get_tagged_addr_ctrl(struct task_struct *task,
+			  int __user *nr_bits, int __user *offset);
+#define SET_TAGGED_ADDR_CTRL(flags, nr_bits, offset)	\
+	set_tagged_addr_ctrl(current, flags, nr_bits, offset)
+#define GET_TAGGED_ADDR_CTRL(nr_bits, offset)		\
+	get_tagged_addr_ctrl(current, nr_bits, offset)
+#endif
+
 #endif /* _ASM_X86_PROCESSOR_H */
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index feb2e21c7e09..e415523dc425 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -38,7 +38,8 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un
 static inline bool __range_not_ok(const void __user *addr, unsigned long size,
 				  unsigned long limit)
 {
-	if (test_thread_flag(TIF_TAGGED_ADDR))
+	if (IS_ENABLED(CONFIG_X86_TAGGED_ADDR_ABI) &&
+	    (current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
 		addr = untagged_addr(addr);
 
 	__chk_user_ptr(addr);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 81d8ef036637..2bc44efdd994 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -26,6 +26,7 @@
 #include <linux/elf-randomize.h>
 #include <trace/events/power.h>
 #include <linux/hw_breakpoint.h>
+#include <linux/sysctl.h>
 #include <asm/cpu.h>
 #include <asm/apic.h>
 #include <linux/uaccess.h>
@@ -231,6 +232,12 @@ static void pkru_flush_thread(void)
 	pkru_write_default();
 }
 
+static void flush_tagged_addr_state(void)
+{
+	if (IS_ENABLED(CONFIG_X86_TAGGED_ADDR_ABI))
+		clear_thread_flag(TIF_TAGGED_ADDR);
+}
+
 void flush_thread(void)
 {
 	struct task_struct *tsk = current;
@@ -240,6 +247,7 @@ void flush_thread(void)
 
 	fpu_flush_thread();
 	pkru_flush_thread();
+	flush_tagged_addr_state();
 }
 
 void disable_TSC(void)
@@ -1000,3 +1008,129 @@ long do_arch_prctl_common(struct task_struct *task, int option,
 
 	return -EINVAL;
 }
+
+#ifdef CONFIG_X86_TAGGED_ADDR_ABI
+/*
+ * Control the relaxed ABI allowing tagged user addresses into the kernel.
+ */
+static unsigned int tagged_addr_disabled;
+
+#ifdef CONFIG_CPU_SUP_AMD
+#define UAI_TAG_BITS	7
+
+static long amd_set_tagged_addr_ctrl(unsigned long flags, int __user *nr_bits,
+				     int __user *offset)
+{
+	int val;
+
+	if (!boot_cpu_has(X86_FEATURE_UAI))
+		return -EINVAL;
+
+	/* Disable tagging */
+	if (!(flags & PR_TAGGED_ADDR_ENABLE)) {
+		clear_thread_flag(TIF_TAGGED_ADDR);
+		return 0;
+	}
+
+	if (!nr_bits || !offset)
+		return -EINVAL;
+
+	/*
+	 * Do not allow the enabling of the tagged address ABI if globally
+	 * disabled via sysctl abi.tagged_addr_disabled.
+	 */
+	if (tagged_addr_disabled)
+		return -EINVAL;
+
+	if (get_user(val, nr_bits))
+		return -EFAULT;
+
+	if (val != UAI_TAG_BITS)
+		return -EINVAL;
+
+	if (put_user(val, nr_bits) || put_user(64 - val, offset))
+		return -EFAULT;
+
+	set_thread_flag(TIF_TAGGED_ADDR);
+	return 0;
+}
+
+static long amd_get_tagged_addr_ctrl(int __user *nr_bits, int __user *offset)
+{
+	long ret = 0;
+
+	if (!boot_cpu_has(X86_FEATURE_UAI))
+		return -EINVAL;
+
+	if (test_thread_flag(TIF_TAGGED_ADDR)) {
+		if (nr_bits && put_user(UAI_TAG_BITS, nr_bits))
+			return -EFAULT;
+		if (offset && put_user(64 - UAI_TAG_BITS, offset))
+			return -EFAULT;
+		ret = PR_TAGGED_ADDR_ENABLE;
+	} else {
+		/* Report max tag size */
+		if (nr_bits && put_user(UAI_TAG_BITS, nr_bits))
+			return -EFAULT;
+	}
+	return ret;
+}
+#endif
+
+long set_tagged_addr_ctrl(struct task_struct *task, unsigned long flags,
+			  int __user *nr_bits, int __user *offset)
+{
+	unsigned long valid_mask = PR_TAGGED_ADDR_ENABLE;
+
+	if (in_32bit_syscall())
+		return -EINVAL;
+
+	if (flags & ~valid_mask)
+		return -EINVAL;
+
+	if (IS_ENABLED(CONFIG_CPU_SUP_AMD))
+		return amd_set_tagged_addr_ctrl(flags, nr_bits, offset);
+	else
+		return -EINVAL;
+}
+
+long get_tagged_addr_ctrl(struct task_struct *task,
+			  int __user *nr_bits, int __user *offset)
+{
+
+	if (in_32bit_syscall())
+		return -EINVAL;
+
+	if (IS_ENABLED(CONFIG_CPU_SUP_AMD))
+		return amd_get_tagged_addr_ctrl(nr_bits, offset);
+	else
+		return -EINVAL;
+}
+
+/*
+ * Global sysctl to disable the tagged user addresses support. This control
+ * only prevents the tagged address ABI enabling via prctl() and does not
+ * disable it for tasks that already opted in to the relaxed ABI.
+ */
+static struct ctl_table tagged_addr_sysctl_table[] = {
+	{
+		.procname	= "tagged_addr_disabled",
+		.mode		= 0644,
+		.data		= &tagged_addr_disabled,
+		.maxlen		= sizeof(int),
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{ }
+};
+
+static int __init tagged_addr_init(void)
+{
+	if (!register_sysctl("abi", tagged_addr_sysctl_table))
+		return -EINVAL;
+	return 0;
+}
+
+core_initcall(tagged_addr_init);
+#endif	/* CONFIG_X86_TAGGED_ADDR_ABI */
-- 
2.25.1

RE: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[David Laight]

From: Bharata B Rao <bharata@amd.com>
> Sent: 10 March 2022 11:16
> 
> This patchset makes use of Upper Address Ignore (UAI) feature available
> on upcoming AMD processors to provide user address tagging support for x86/AMD.
> 
> UAI allows software to store a tag in the upper 7 bits of a logical
> address [63:57]. When enabled, the processor will suppress the
> traditional canonical address checks on the addresses. More information
> about UAI can be found in section 5.10 of 'AMD64 Architecture
> Programmer's Manual, Vol 2: System Programming' which is available from
> 
> https://bugzilla.kernel.org/attachment.cgi?id=300549

Is that really allowing bit 63 to be used?
That is normally the user-kernel bit.
I can't help feeling that will just badly break things.

Otherwise the best thing is just to change access_ok()
to only reject addresses with the top bit set.
Then you shouldn't need any extra tests in the fast-path
of access_ok().

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Dave Hansen]

On 3/10/22 06:32, David Laight wrote:
>> UAI allows software to store a tag in the upper 7 bits of a logical
>> address [63:57]. When enabled, the processor will suppress the
>> traditional canonical address checks on the addresses. More information
>> about UAI can be found in section 5.10 of 'AMD64 Architecture
>> Programmer's Manual, Vol 2: System Programming' which is available from
>>
>> https://bugzilla.kernel.org/attachment.cgi?id=300549
> Is that really allowing bit 63 to be used?
> That is normally the user-kernel bit.
> I can't help feeling that will just badly break things.

Yeah, this does seem worrisome.  The LAM approach[1] retains
canonicality checking for bit 63.


1.
https://www.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html

RE: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[David Laight]

From: Dave Hansen <dave.hansen@intel.com>
> Sent: 10 March 2022 16:46
> 
> On 3/10/22 06:32, David Laight wrote:
> >> UAI allows software to store a tag in the upper 7 bits of a logical
> >> address [63:57]. When enabled, the processor will suppress the
> >> traditional canonical address checks on the addresses. More information
> >> about UAI can be found in section 5.10 of 'AMD64 Architecture
> >> Programmer's Manual, Vol 2: System Programming' which is available from
> >>
> >> https://bugzilla.kernel.org/attachment.cgi?id=300549
> > Is that really allowing bit 63 to be used?
> > That is normally the user-kernel bit.
> > I can't help feeling that will just badly break things.
> 
> Yeah, this does seem worrisome.  The LAM approach[1] retains
> canonicality checking for bit 63.

Actually it is rather worse than 'worrisome'.
Allowing the user all address upto the base of the valid
kernel addresses (probably tags to 3e, but not 3f)
means that you can't use a fast address check in access_ok().
You are forced to use the strict test that 32bit kernels use.

Otherwise for 64bit access_ok() need only test address < 0
and rely on kernel code reading something below the (big)
offset to valid kernel addresses.
No real need to include the length at all.

If the hardware is just ignoring the high address bits
then the should be no need to mask them in kernel.
The required kernel accesses to user memory should 'just work'.

Of course, the bit to enable this (wherever it is) needs
to be restored on every process switch.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Bharata B Rao]

On 3/10/2022 10:49 PM, David Laight wrote:
> From: Dave Hansen <dave.hansen@intel.com>
>> Sent: 10 March 2022 16:46
>>
>> On 3/10/22 06:32, David Laight wrote:
>>>> UAI allows software to store a tag in the upper 7 bits of a logical
>>>> address [63:57]. When enabled, the processor will suppress the
>>>> traditional canonical address checks on the addresses. More information
>>>> about UAI can be found in section 5.10 of 'AMD64 Architecture
>>>> Programmer's Manual, Vol 2: System Programming' which is available from
>>>>
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fattachment.cgi%3Fid%3D300549&amp;data=04%7C01%7Cbharata%40amd.com%7Ca1de24223931481b3fcb08da02ba2e6f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637825295938946622%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=HijEAUq172r8YwkcCuhvl99Vk5BwE6iSROXcSQXmJHk%3D&amp;reserved=0
>>> Is that really allowing bit 63 to be used?
>>> That is normally the user-kernel bit.
>>> I can't help feeling that will just badly break things.
>>
>> Yeah, this does seem worrisome.  The LAM approach[1] retains
>> canonicality checking for bit 63.
> 
> Actually it is rather worse than 'worrisome'.
> Allowing the user all address upto the base of the valid
> kernel addresses (probably tags to 3e, but not 3f)
> means that you can't use a fast address check in access_ok().
> You are forced to use the strict test that 32bit kernels use.

From what I see, there is a single implementation of access_ok()
in arch/x86/asm/include/uaccess.h that does check if the user
address+size exceeds the limit.

Guess I am missing something, but can you please point me to the fast
implementation(that benefits from bit 63 being user/kernel address
disambiguation bit) and the strict checking in 32bit kernels that
are you are referring to?

Also I wonder here why ARM64 TBI which also uses the full upper byte
(including bit 63) for storing tag/metadata doesn't suffer from
this same problem?

Regards,
Bharata.

RE: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[David Laight]

From: Bharata B Rao
> Sent: 11 March 2022 05:43
> On 3/10/2022 10:49 PM, David Laight wrote:
> > From: Dave Hansen <dave.hansen@intel.com>
> >> Sent: 10 March 2022 16:46
> >>
> >> On 3/10/22 06:32, David Laight wrote:
> >>>> UAI allows software to store a tag in the upper 7 bits of a logical
> >>>> address [63:57]. When enabled, the processor will suppress the
> >>>> traditional canonical address checks on the addresses. More information
> >>>> about UAI can be found in section 5.10 of 'AMD64 Architecture
> >>>> Programmer's Manual, Vol 2: System Programming' which is available from
> >>>>
,,,
> >>> Is that really allowing bit 63 to be used?
> >>> That is normally the user-kernel bit.
> >>> I can't help feeling that will just badly break things.
> >>
> >> Yeah, this does seem worrisome.  The LAM approach[1] retains
> >> canonicality checking for bit 63.
> >
> > Actually it is rather worse than 'worrisome'.
> > Allowing the user all address upto the base of the valid
> > kernel addresses (probably tags to 3e, but not 3f)
> > means that you can't use a fast address check in access_ok().
> > You are forced to use the strict test that 32bit kernels use.
> 
> From what I see, there is a single implementation of access_ok()
> in arch/x86/asm/include/uaccess.h that does check if the user
> address+size exceeds the limit.
> 
> Guess I am missing something, but can you please point me to the fast
> implementation(that benefits from bit 63 being user/kernel address
> disambiguation bit) and the strict checking in 32bit kernels that
> are you are referring to?

You can just check ((address | size) >> 62) on 64bit arch that
use bit 63 to select user/kernel and have a massive address
hole near the boundary.
The compiler optimises out constant size from that calculation.
On x86-64 non-canonical addresses give a different fault
to 'page not present' - but that can be handled.

In practice you can (probably) even completely ignore the 'size'
and just error if the top bit of the address is set.
While accesses might not be completely sequential they aren't
going to jump over the non-canonical addresses.

> Also I wonder here why ARM64 TBI which also uses the full upper byte
> (including bit 63) for storing tag/metadata doesn't suffer from
> this same problem?

The user-kernel bit is lower down (something like bit 53) so the
tags are using separate bits.
Although that choice of user/kernel bit makes life hard elsewhere.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Bharata B Rao]

On 3/11/2022 1:45 PM, David Laight wrote:
> From: Bharata B Rao
>> Sent: 11 March 2022 05:43
>> On 3/10/2022 10:49 PM, David Laight wrote:
>>> From: Dave Hansen <dave.hansen@intel.com>
>>>> Sent: 10 March 2022 16:46
>>>>
>>>> On 3/10/22 06:32, David Laight wrote:
>>>>>> UAI allows software to store a tag in the upper 7 bits of a logical
>>>>>> address [63:57]. When enabled, the processor will suppress the
>>>>>> traditional canonical address checks on the addresses. More information
>>>>>> about UAI can be found in section 5.10 of 'AMD64 Architecture
>>>>>> Programmer's Manual, Vol 2: System Programming' which is available from
>>>>>>
> ,,,
>>>>> Is that really allowing bit 63 to be used?
>>>>> That is normally the user-kernel bit.
>>>>> I can't help feeling that will just badly break things.
>>>>
>>>> Yeah, this does seem worrisome.  The LAM approach[1] retains
>>>> canonicality checking for bit 63.
>>>
>>> Actually it is rather worse than 'worrisome'.
>>> Allowing the user all address upto the base of the valid
>>> kernel addresses (probably tags to 3e, but not 3f)
>>> means that you can't use a fast address check in access_ok().
>>> You are forced to use the strict test that 32bit kernels use.
>>
>> From what I see, there is a single implementation of access_ok()
>> in arch/x86/asm/include/uaccess.h that does check if the user
>> address+size exceeds the limit.
>>
>> Guess I am missing something, but can you please point me to the fast
>> implementation(that benefits from bit 63 being user/kernel address
>> disambiguation bit) and the strict checking in 32bit kernels that
>> are you are referring to?
> 
> You can just check ((address | size) >> 62) on 64bit arch that
> use bit 63 to select user/kernel and have a massive address
> hole near the boundary.
> The compiler optimises out constant size from that calculation.
> On x86-64 non-canonical addresses give a different fault
> to 'page not present' - but that can be handled.

Ok, so are you mentioning about a future optimization to access_ok()
that could benefit by retaining bit 63 as kernel/user bit?

Since you said using bit 63 to store metadata will break things,
I was trying to understand how and where does it break.

Regards,
Bharata.

RE: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[David Laight]

From: Bharata B Rao
> Sent: 11 March 2022 09:11
> 
> On 3/11/2022 1:45 PM, David Laight wrote:
> > From: Bharata B Rao
> >> Sent: 11 March 2022 05:43
> >> On 3/10/2022 10:49 PM, David Laight wrote:
> >>> From: Dave Hansen <dave.hansen@intel.com>
> >>>> Sent: 10 March 2022 16:46
> >>>>
> >>>> On 3/10/22 06:32, David Laight wrote:
> >>>>>> UAI allows software to store a tag in the upper 7 bits of a logical
> >>>>>> address [63:57]. When enabled, the processor will suppress the
> >>>>>> traditional canonical address checks on the addresses. More information
> >>>>>> about UAI can be found in section 5.10 of 'AMD64 Architecture
> >>>>>> Programmer's Manual, Vol 2: System Programming' which is available from
> >>>>>>
> > ,,,
> >>>>> Is that really allowing bit 63 to be used?
> >>>>> That is normally the user-kernel bit.
> >>>>> I can't help feeling that will just badly break things.
> >>>>
> >>>> Yeah, this does seem worrisome.  The LAM approach[1] retains
> >>>> canonicality checking for bit 63.
> >>>
> >>> Actually it is rather worse than 'worrisome'.
> >>> Allowing the user all address upto the base of the valid
> >>> kernel addresses (probably tags to 3e, but not 3f)
> >>> means that you can't use a fast address check in access_ok().
> >>> You are forced to use the strict test that 32bit kernels use.
> >>
> >> From what I see, there is a single implementation of access_ok()
> >> in arch/x86/asm/include/uaccess.h that does check if the user
> >> address+size exceeds the limit.
> >>
> >> Guess I am missing something, but can you please point me to the fast
> >> implementation(that benefits from bit 63 being user/kernel address
> >> disambiguation bit) and the strict checking in 32bit kernels that
> >> are you are referring to?
> >
> > You can just check ((address | size) >> 62) on 64bit arch that
> > use bit 63 to select user/kernel and have a massive address
> > hole near the boundary.
> > The compiler optimises out constant size from that calculation.
> > On x86-64 non-canonical addresses give a different fault
> > to 'page not present' - but that can be handled.
> 
> Ok, so are you mentioning about a future optimization to access_ok()
> that could benefit by retaining bit 63 as kernel/user bit?
> 
> Since you said using bit 63 to store metadata will break things,
> I was trying to understand how and where does it break.

Kernel addresses start at 0xffxxx (for 57bit, 5 level page tables).
(Maybe the valid ones are still 0xffff8xxx.)
so you really don't want userspace using those to alias valid user
addresses.

I'm not entirely sure what enabling UAI does.
But the user page tables have to contain mappings for some kernel
addresses (even with page table separation).
Also you really don't want to have to mask off the high address
bits before a kernel access to the use buffer.
So it isn't really obvious how addresses that start 0xff can be used.
and that rather implies you can use bit 63 at all (without horrid
logic in some (probably) very critical hardware timing paths.

Wikipedia also notes:
    Intel has implemented a scheme with a 5-level page table, which allows
    Intel 64 processors to support a 57-bit virtual address space.
    Further extensions may allow full 64-bit virtual address space and
    physical memory by expanding the page table entry size to 128-bit,
    and reduce page walks in the 5-level hierarchy by using a larger 64 KiB
    page allocation size that still supports 4 KiB page operations for
    backward compatibility.
If they implement 64K pages then you lose the extra bits.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Dave Hansen]

On 3/11/22 01:36, David Laight wrote:
> Wikipedia also notes:
>     Intel has implemented a scheme with a 5-level page table, which allows
>     Intel 64 processors to support a 57-bit virtual address space.
>     Further extensions may allow full 64-bit virtual address space and
>     physical memory by expanding the page table entry size to 128-bit,
>     and reduce page walks in the 5-level hierarchy by using a larger 64 KiB
>     page allocation size that still supports 4 KiB page operations for
>     backward compatibility.
> If they implement 64K pages then you lose the extra bits.

I can't believe I need to say this:  Wikipedia is not an authoritative
source about what anyone is going to do with their CPUs in the future.
Please don't base any Linux decisions off this information.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Dave Hansen]

On 3/10/22 03:15, Bharata B Rao wrote:>
> This patchset builds on that prctl() extension and adds support
> for AMD UAI. AMD implementation is kept separate as equivalent
> Intel LAM implementation is likely to be different due to different
> bit positions and tag width.

Please don't keep the implementations separate.

We'll have one x86 implementation of address bit masking.  Both the
Intel and AMD implementations will feed into a shared implementation.
Something _like_ the cc_set_mask() interface where both implementations
do their detection and then call into common code to say how many bits
are being ignored.

A good litmus test for this is how many vendor-specific checks there are
in common code.  If there are a lot of them, it's not a good sign for
the design.

I'd also highly suggest going over Kirill's patch set in detail.  There
are things like this:

> https://lore.kernel.org/linux-mm/20210205151631.43511-10-kirill.shutemov@linux.intel.com/

which seem pretty sane to me but which are (I think) missing in this set.

I don't know if we can get there but, in an ideal world, this would be
series with, say 7 patches.  Patches 1-5 are generic enabling.  Patch 6
is tiny and does detection and enabling for UAI.  Patch 7 does the same
for LAM.  All the patches in the series are acked from LAM and UAI folks.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Dave Hansen]

On 3/10/22 07:16, Dave Hansen wrote:
> I'd also highly suggest going over Kirill's patch set in detail.  There
> are things like this:
> 
>> https://lore.kernel.org/linux-mm/20210205151631.43511-10-kirill.shutemov@linux.intel.com/
> which seem pretty sane to me but which are (I think) missing in this set.

Oh silly me, that's just for LAM_U48 which ignores more bits than
LAM_U57 or UAI.

Either way, I know you've looked at Kirill's set, but please do go over
it in detail to make sure there's nothing else you need to lift out of
there.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Bharata B Rao]

On 3/10/2022 8:46 PM, Dave Hansen wrote:
> On 3/10/22 03:15, Bharata B Rao wrote:>
>> This patchset builds on that prctl() extension and adds support
>> for AMD UAI. AMD implementation is kept separate as equivalent
>> Intel LAM implementation is likely to be different due to different
>> bit positions and tag width.
> 
> Please don't keep the implementations separate.
> 
> We'll have one x86 implementation of address bit masking.  Both the
> Intel and AMD implementations will feed into a shared implementation.
> Something _like_ the cc_set_mask() interface where both implementations
> do their detection and then call into common code to say how many bits
> are being ignored.

The difference isn't limited to the number of ignored bits, see below...

> 
> A good litmus test for this is how many vendor-specific checks there are
> in common code.  If there are a lot of them, it's not a good sign for
> the design.

That is generally true but considering the below differences, I felt it
was much cleaner to go for separate implementations for enablement and
untagging while still building on the common prctl() extension in the
RFC version.

1. While Intel LAM provides two LAM widths (15 and 6 bits wide), AMD UAI
has a fixed tag width of 7 bits. This results in following differences
in the implementation:
   - Two threadinfo flags (TIF_LAM_57 and TIF_LAM_48) in Intel LAM vs
     single flag TIF_TAGGED_ADDR(like in ARM64) in AMD UAI.
   - Untagging needs to be aware of 2 widths in Intel LAM as against
     a single width in AMD UAI.
   - Requirement of making LAM_U48 and mappings above 47bits mutually
     exclusive is required for Intel LAM only.

2. The enablement bit is part of CR3 in Intel LAM which brings in
additional complexity of updating the CR3 with right LAM mode on every
MM switch and associated tlbstate changes. In AMD UAI, enablement bit
is part of EFER MSR and it is a single time enablement with no MM switch
time changes.

However, let me take a detailed look once again and explore further
code sharing possibilities.

Regards,
Bharata.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Dave Hansen]

On 3/13/22 22:00, Bharata B Rao wrote:
> On 3/10/2022 8:46 PM, Dave Hansen wrote:> 1. While Intel LAM provides two LAM widths (15 and 6 bits wide), AMD UAI
> has a fixed tag width of 7 bits. This results in following differences
> in the implementation:
>    - Two threadinfo flags (TIF_LAM_57 and TIF_LAM_48) in Intel LAM vs
>      single flag TIF_TAGGED_ADDR(like in ARM64) in AMD UAI.
>    - Untagging needs to be aware of 2 widths in Intel LAM as against
>      a single width in AMD UAI.

I'd be perfectly happy with an initial version of this stuff that only
comprehends UAI and LAM_57.  As long as the ABI can be used for all
three cases, adding the two most similar ones initially would make a lot
of sense.

>    - Requirement of making LAM_U48 and mappings above 47bits mutually
>      exclusive is required for Intel LAM only.
> 
> 2. The enablement bit is part of CR3 in Intel LAM which brings in
> additional complexity of updating the CR3 with right LAM mode on every
> MM switch and associated tlbstate changes. In AMD UAI, enablement bit
> is part of EFER MSR and it is a single time enablement with no MM switch
> time changes.

Hold on a sec.  The context-switching is a *policy*.  A _kernel_ policy.
If we wanted the LAM settings to be static and system wide, we'd just
have a single:

	if (cpu_feature_enabled(LAM))
		cr3 |= LAM_MASK;

in build_cr3().  That's not exactly complicated.

You know what's a heck of a lot more complicated than that?  Adding
context-switching for EFER.

I can't imagine a world where we want this tagging to be system-wide.
There *ARE* going to be apps that break with this pointer tagging stuff.
 Normal, user apps.  Apps in containers.  With a system-wide setting,
they have zero recourse when things break.  All a user can do is reboot
the kernel.

As-is, it seems like it would be awful to even develop apps that use
tagging.  You always want to test them with tagging on and off.  With
this, you need to reboot to test either way.

Also, the prctl() in Kirill's version actually enables and disables the
hardware feature in addition to the in-kernel tag/untag operations.
This series takes the same ABI and doesn't change the hardware feature
state.  That will need to be rectified at some point.

Speaking of which, it's also really worrying that kernel behavior is
affected by _EFER_UAI.  When tagging is "disabled" in the prctl(),
_EFER_UAI is still set.  The kernel can go merrily dereferencing both
kernel and userspace pointers with no canonical checks.  That seems
scary.  LAM's supervisor separation makes things a lot less scary.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Andy Lutomirski]

On Thu, Mar 10, 2022, at 3:15 AM, Bharata B Rao wrote:
> Hi,
>
> This patchset makes use of Upper Address Ignore (UAI) feature available
> on upcoming AMD processors to provide user address tagging support for x86/AMD.
>
> UAI allows software to store a tag in the upper 7 bits of a logical
> address [63:57]. When enabled, the processor will suppress the
> traditional canonical address checks on the addresses. More information
> about UAI can be found in section 5.10 of 'AMD64 Architecture
> Programmer's Manual, Vol 2: System Programming' which is available from
>
> https://bugzilla.kernel.org/attachment.cgi?id=300549

I hate to be a pain, but I'm really not convinced that this feature is suitable for Linux.  There are a few reasons:

Right now, the concept that the high bit of an address determines whether it's a user or a kernel address is fairly fundamental to the x86_64 (and x86_32!) code.  It may not be strictly necessary to preserve this, but violating it would require substantial thought.  With UAI enabled, kernel and user addresses are, functionally, interleaved.  This makes things like access_ok checks, and more generally anything that operates on a range of addresses, behave potentially quite differently.  A lot of auditing of existing code would be needed to make it safe.

UAI looks like it wasn't intended to be context switched and, indeed, your series doesn't context switch it.  As far as I'm concerned, this is an error, and if we support UAI at all, we should context switch it.  Yes, this will be slow, perhaps painfully slow.  AMD knows how to fix it by, for example, reading the Intel SDM.  By *not* context switching UAI, we force it on for all user code, including unsuspecting user code, as well as for kernel code.  Do we actually want it on for kernel code?  With LAM, in contrast, the semantics for kernel pointers vs user pointers actually make sense and can be set per mm, which will make things like io_uring (in theory) do the right thing.

UAI and LAM are incompatible from a userspace perspective.  Since LAM is pretty clearly superior [0], it seems like a better long term outcome would be for programs that want tag bits to target LAM and for AMD to support LAM if there is demand.  For that matter, do we actually expect any userspace to want to support UAI?  (Are there existing too-clever sandboxes that would be broken by enabling UAI?)

Given that UAI is not efficiently context switched, the implementation of uaccess is rather bizarre.  With the implementation in this series in particular, if the access_ok checks ever get out of sync with actual user access, a user access could be emitted with the high bits not masked despite the range check succeeding due to masking, which would, unless great care is taken, result in a "user" access hitting the kernel range.  That's no good.

I believe it's possible for a high-quality kernel UAI implementation to exist, but, as above, I think it would be slow, and it might be quite complex and fragile.  Are we sure that it's worth supporting it?

[0] I hope I don't have to argue this point.



>
> Currently ARM64 provides a way for processes to opt-in for
> relaxed tagged ABI via prctl() options PR_SET/GET_TAGGED_ADDR_CTRL.
> The prctl() API was found to be a bit restrictive for x86 use and
> Kirill had posted an extension to it as part of Intel LAM patchset.
> (https://lore.kernel.org/linux-mm/20210205151631.43511-1-kirill.shutemov@linux.intel.com/)
>
> This patchset builds on that prctl() extension and adds support
> for AMD UAI. AMD implementation is kept separate as equivalent
> Intel LAM implementation is likely to be different due to different
> bit positions and tag width.
>
> This is an early implementation which has been only lightly tested.
> I have used the tags_test.c from selftests/vm/tags/ to test this.
> For ARM64 changes, I have only ensured that the changes compile.
>
> Regards,
> Bharata.
>
> Bharata B Rao (5):
>   x86/cpufeatures: Add Upper Address Ignore(UAI) as CPU feature
>   x86: Enable Upper Address Ignore(UAI) feature
>   x86: Provide an implementation of untagged_addr()
>   x86: Untag user pointers in access_ok()
>   x86: Add prctl() options to control tagged user addresses ABI
>
> Kirill A. Shutemov (1):
>   mm, arm64: Update PR_SET/GET_TAGGED_ADDR_CTRL interface
>
>  arch/arm64/include/asm/processor.h            |  12 +-
>  arch/arm64/kernel/process.c                   |  45 +++++-
>  arch/arm64/kernel/ptrace.c                    |   4 +-
>  arch/x86/Kconfig                              |   9 ++
>  arch/x86/include/asm/cpufeatures.h            |   2 +-
>  arch/x86/include/asm/msr-index.h              |   2 +
>  arch/x86/include/asm/page_32.h                |   3 +
>  arch/x86/include/asm/page_64.h                |  26 ++++
>  arch/x86/include/asm/processor.h              |  12 ++
>  arch/x86/include/asm/thread_info.h            |   2 +
>  arch/x86/include/asm/uaccess.h                |  29 +++-
>  arch/x86/kernel/cpu/scattered.c               |   1 +
>  arch/x86/kernel/process.c                     | 134 ++++++++++++++++++
>  arch/x86/kernel/setup.c                       |   8 ++
>  kernel/sys.c                                  |  14 +-
>  .../testing/selftests/arm64/tags/tags_test.c  |  31 ----
>  .../selftests/{arm64 => vm}/tags/.gitignore   |   0
>  .../selftests/{arm64 => vm}/tags/Makefile     |   0
>  .../{arm64 => vm}/tags/run_tags_test.sh       |   0
>  tools/testing/selftests/vm/tags/tags_test.c   |  59 ++++++++
>  20 files changed, 335 insertions(+), 58 deletions(-)
>  delete mode 100644 tools/testing/selftests/arm64/tags/tags_test.c
>  rename tools/testing/selftests/{arm64 => vm}/tags/.gitignore (100%)
>  rename tools/testing/selftests/{arm64 => vm}/tags/Makefile (100%)
>  rename tools/testing/selftests/{arm64 => vm}/tags/run_tags_test.sh (100%)
>  create mode 100644 tools/testing/selftests/vm/tags/tags_test.c
>
> -- 
> 2.25.1

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Thomas Gleixner]

On Mon, Mar 21 2022 at 15:29, Andy Lutomirski wrote:
> On Thu, Mar 10, 2022, at 3:15 AM, Bharata B Rao wrote:
>> This patchset makes use of Upper Address Ignore (UAI) feature available
>> on upcoming AMD processors to provide user address tagging support for x86/AMD.
>>
>> UAI allows software to store a tag in the upper 7 bits of a logical
>> address [63:57]. When enabled, the processor will suppress the
>> traditional canonical address checks on the addresses. More information
>> about UAI can be found in section 5.10 of 'AMD64 Architecture
>> Programmer's Manual, Vol 2: System Programming' which is available from
>>
>> https://bugzilla.kernel.org/attachment.cgi?id=300549
>
> I hate to be a pain, but I'm really not convinced that this feature is
> suitable for Linux.  There are a few reasons:

Abusing bit 63 is not suitable for any OS in my opinion.

> Right now, the concept that the high bit of an address determines
> whether it's a user or a kernel address is fairly fundamental to the
> x86_64 (and x86_32!) code.  It may not be strictly necessary to
> preserve this, but violating it would require substantial thought.
> With UAI enabled, kernel and user addresses are, functionally,
> interleaved.  This makes things like access_ok checks, and more
> generally anything that operates on a range of addresses, behave
> potentially quite differently.  A lot of auditing of existing code
> would be needed to make it safe.

Which might be finished ten years from now....

Seriously there is no justification for the bit 63 abuse. This has been
pointed out by various people to AMD before this saw the public. Other
vendors seem to have gotten the memo.

The proper solution here is to issue an erratum and fix this nonsense in
microcode for the already taped out silicon and get rid of it in the
design of future ones completely. Anything else is just wishful
thinking.

Thanks,

        tglx

RE: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[David Laight]

From: Andy Lutomirski
> Sent: 21 March 2022 22:30
> On Thu, Mar 10, 2022, at 3:15 AM, Bharata B Rao wrote:
> > Hi,
> >
> > This patchset makes use of Upper Address Ignore (UAI) feature available
> > on upcoming AMD processors to provide user address tagging support for x86/AMD.
> >
> > UAI allows software to store a tag in the upper 7 bits of a logical
> > address [63:57]. When enabled, the processor will suppress the
> > traditional canonical address checks on the addresses. More information
> > about UAI can be found in section 5.10 of 'AMD64 Architecture
> > Programmer's Manual, Vol 2: System Programming' which is available from
> >
> > https://bugzilla.kernel.org/attachment.cgi?id=300549
> 
> I hate to be a pain, but I'm really not convinced that this feature is suitable for Linux.  There are
> a few reasons:
> 
> Right now, the concept that the high bit of an address determines whether it's a user or a kernel
> address is fairly fundamental to the x86_64 (and x86_32!) code.  It may not be strictly necessary to
> preserve this, but violating it would require substantial thought.  With UAI enabled, kernel and user
> addresses are, functionally, interleaved.  This makes things like access_ok checks, and more generally
> anything that operates on a range of addresses, behave potentially quite differently.  A lot of
> auditing of existing code would be needed to make it safe.
> 
> UAI looks like it wasn't intended to be context switched and, indeed, your series doesn't context
> switch it.  As far as I'm concerned, this is an error, and if we support UAI at all, we should context
> switch it.  Yes, this will be slow, perhaps painfully slow.  AMD knows how to fix it by, for example,
> reading the Intel SDM.  By *not* context switching UAI, we force it on for all user code, including
> unsuspecting user code, as well as for kernel code.  Do we actually want it on for kernel code?  With
> LAM, in contrast, the semantics for kernel pointers vs user pointers actually make sense and can be
> set per mm, which will make things like io_uring (in theory) do the right thing.

You also need the kernel to be able to use the 'tagged'
addresses to access userspace - ie without having to
mask off any tag.

This is really for the same reason they want hardware
support for tagged addresses to avoid having to 'untag'
before calling library routines.

I'm not even sure which address the page fault handler
should see - it will need to the difference between a
normal process using a non-canonical address and a PF
on a tagged address.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Bharata B Rao]

On 3/22/2022 3:59 AM, Andy Lutomirski wrote:
> On Thu, Mar 10, 2022, at 3:15 AM, Bharata B Rao wrote:
>> Hi,
>>
>> This patchset makes use of Upper Address Ignore (UAI) feature available
>> on upcoming AMD processors to provide user address tagging support for x86/AMD.
>>
>> UAI allows software to store a tag in the upper 7 bits of a logical
>> address [63:57]. When enabled, the processor will suppress the
>> traditional canonical address checks on the addresses. More information
>> about UAI can be found in section 5.10 of 'AMD64 Architecture
>> Programmer's Manual, Vol 2: System Programming' which is available from
>>
>> https://bugzilla.kernel.org/attachment.cgi?id=300549
> 
> I hate to be a pain, but I'm really not convinced that this feature is suitable for Linux.  There are a few reasons:
> 
> Right now, the concept that the high bit of an address determines whether it's a user or a kernel address is fairly fundamental to the x86_64 (and x86_32!) code.  It may not be strictly necessary to preserve this, but violating it would require substantial thought.  With UAI enabled, kernel and user addresses are, functionally, interleaved.  This makes things like access_ok checks, and more generally anything that operates on a range of addresses, behave potentially quite differently.  A lot of auditing of existing code would be needed to make it safe.

Ok got that. However can you point to me a few instances in the current
kernel code where such assumption of high bit being user/kernel address
differentiator exists so that I get some idea of what it takes to
audit all such cases?

Also wouldn't the problem of high bit be solved by using only the
6 out of 7 available bits in UAI and leaving the 63rd bit alone?
The hardware will still ignore the top bit, but this should take
care of the requirement of high bit being 0/1 for user/kernel in the
x86_64 kernel. Wouldn't that work?

> 
> UAI looks like it wasn't intended to be context switched and, indeed, your series doesn't context switch it.  As far as I'm concerned, this is an error, and if we support UAI at all, we should context switch it.  Yes, this will be slow, perhaps painfully slow.  AMD knows how to fix it by, for example, reading the Intel SDM.  By *not* context switching UAI, we force it on for all user code, including unsuspecting user code, as well as for kernel code.  Do we actually want it on for kernel code?  With LAM, in contrast, the semantics for kernel pointers vs user pointers actually make sense and can be set per mm, which will make things like io_uring (in theory) do the right thing.

I plan to enable/disable UAI based on the next task's settings by
doing MSR write to EFER during context switch. I will have to measure
how much additional cost an MSR write in context switch path brings in.
However given that without a hardware feature like ARM64 MTE, this would
primarily be used in non-production environments. Hence I wonder if MSR
write cost could be tolerated?

Regarding enabling UAI for kernel, I will have to check how clean and
efficient it would be to disable/enable UAI on user/kernel entry/exit
points.

> 
> UAI and LAM are incompatible from a userspace perspective.  Since LAM is pretty clearly superior [0], it seems like a better long term outcome would be for programs that want tag bits to target LAM and for AMD to support LAM if there is demand.  For that matter, do we actually expect any userspace to want to support UAI?  (Are there existing too-clever sandboxes that would be broken by enabling UAI?)
> 
> Given that UAI is not efficiently context switched, the implementation of uaccess is rather bizarre.  With the implementation in this series in particular, if the access_ok checks ever get out of sync with actual user access, a user access could be emitted with the high bits not masked despite the range check succeeding due to masking, which would, unless great care is taken, result in a "user" access hitting the kernel range.  That's no good.

Okay, I guess if context switching and sticking to 6 bits as mentioned
earlier is feasible, this concern too goes away unless I am missing something.

Thanks for your feedback.

Regards,
Bharata.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Dave Hansen]

On 3/23/22 00:48, Bharata B Rao wrote:
> Ok got that. However can you point to me a few instances in the current
> kernel code where such assumption of high bit being user/kernel address
> differentiator exists so that I get some idea of what it takes to
> audit all such cases?

Look around for comparisons against TASK_SIZE_MAX.
arch/x86/lib/putuser.S or something like arch_check_bp_in_kernelspace()
come to mind.

> Also wouldn't the problem of high bit be solved by using only the
> 6 out of 7 available bits in UAI and leaving the 63rd bit alone?
> The hardware will still ignore the top bit, but this should take
> care of the requirement of high bit being 0/1 for user/kernel in the
> x86_64 kernel. Wouldn't that work?

I don't think so.

The kernel knows that a dereference of a pointer that looks like a
kernel address that get kernel data.  Userspace must be kept away from
things that look like kernel addresses.

Let's say some app does:

	void *ptr = (void *)0xffffffffc038d130;
	read(fd, ptr, 1);

and inside the kernel, that boils down to:

	put_user('x', 0xffffffffc038d130);

Today the kernels knows that 0xffffffffc038d130 is >=TASK_SIZE_MAX, so
this is obviously naughty userspace trying to write to the kernel.  But,
it's not obviously wrong if the high bits are ignored.

Like you said, we could, as a convention, check for the highest bit
being set and use *that* to indicate a kernel address.  But, the sneaky
old userspace would just do:

	put_user('x', 0x7fffffffc038d130);

It would pass the "high bit" check since that bit is clear, but it still
accesses kernel memory because UAI ignores the bit userspace just cleared.

I think the only way to get around this is to go find every single place
in the kernel that does a userspace address check and ensure that it
fully untags the pointer first.

...
> However given that without a hardware feature like ARM64 MTE, this would
> primarily be used in non-production environments. Hence I wonder if MSR
> write cost could be tolerated?

It would be great of the mysterious folks who asked both Intel and AMD
for this feature could weigh in on this thread.  But, I've been assuming
that these features will be used to accelerate address sanitizers which
used heavily today in non-production environments but are (generally)
too slow for production.

I'd be very surprised if folks wanted this snazzy new hardware feature
from every CPU vendor on the planet just to speed up their
non-production environments.

I'd be less surprised if they wanted to expand the use of pointer
tagging into more production environments.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Bharata B Rao]

On 4/2/2022 12:55 AM, Dave Hansen wrote:
> On 3/23/22 00:48, Bharata B Rao wrote:
>> Ok got that. However can you point to me a few instances in the current
>> kernel code where such assumption of high bit being user/kernel address
>> differentiator exists so that I get some idea of what it takes to
>> audit all such cases?
> 
> Look around for comparisons against TASK_SIZE_MAX.
> arch/x86/lib/putuser.S or something like arch_check_bp_in_kernelspace()
> come to mind.
> 
>> Also wouldn't the problem of high bit be solved by using only the
>> 6 out of 7 available bits in UAI and leaving the 63rd bit alone?
>> The hardware will still ignore the top bit, but this should take
>> care of the requirement of high bit being 0/1 for user/kernel in the
>> x86_64 kernel. Wouldn't that work?
> 
> I don't think so.
> 
> The kernel knows that a dereference of a pointer that looks like a
> kernel address that get kernel data.  Userspace must be kept away from
> things that look like kernel addresses.
> 
> Let's say some app does:
> 
> 	void *ptr = (void *)0xffffffffc038d130;
> 	read(fd, ptr, 1);
> 
> and inside the kernel, that boils down to:
> 
> 	put_user('x', 0xffffffffc038d130);
> 
> Today the kernels knows that 0xffffffffc038d130 is >=TASK_SIZE_MAX, so
> this is obviously naughty userspace trying to write to the kernel.  But,
> it's not obviously wrong if the high bits are ignored.
> 
> Like you said, we could, as a convention, check for the highest bit
> being set and use *that* to indicate a kernel address.  But, the sneaky
> old userspace would just do:
> 
> 	put_user('x', 0x7fffffffc038d130);
> 
> It would pass the "high bit" check since that bit is clear, but it still
> accesses kernel memory because UAI ignores the bit userspace just cleared.
> 
> I think the only way to get around this is to go find every single place
> in the kernel that does a userspace address check and ensure that it
> fully untags the pointer first.

Thanks Dave for the detailed explanation.

We are discussing these aspects with the hardware team to check the best
possible path forward.

Regards,
Bharata.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Andy Lutomirski]

On Wed, Mar 23, 2022, at 12:48 AM, Bharata B Rao wrote:
> On 3/22/2022 3:59 AM, Andy Lutomirski wrote:
>> On Thu, Mar 10, 2022, at 3:15 AM, Bharata B Rao wrote:
>>> Hi,
>>>
>>> This patchset makes use of Upper Address Ignore (UAI) feature available
>>> on upcoming AMD processors to provide user address tagging support for x86/AMD.
>>>
>>> UAI allows software to store a tag in the upper 7 bits of a logical
>>> address [63:57]. When enabled, the processor will suppress the
>>> traditional canonical address checks on the addresses. More information
>>> about UAI can be found in section 5.10 of 'AMD64 Architecture
>>> Programmer's Manual, Vol 2: System Programming' which is available from
>>>
>>> https://bugzilla.kernel.org/attachment.cgi?id=300549
>> 
>> I hate to be a pain, but I'm really not convinced that this feature is suitable for Linux.  There are a few reasons:
>> 
>> Right now, the concept that the high bit of an address determines whether it's a user or a kernel address is fairly fundamental to the x86_64 (and x86_32!) code.  It may not be strictly necessary to preserve this, but violating it would require substantial thought.  With UAI enabled, kernel and user addresses are, functionally, interleaved.  This makes things like access_ok checks, and more generally anything that operates on a range of addresses, behave potentially quite differently.  A lot of auditing of existing code would be needed to make it safe.
>
> Ok got that. However can you point to me a few instances in the current
> kernel code where such assumption of high bit being user/kernel address
> differentiator exists so that I get some idea of what it takes to
> audit all such cases?

Anything that thinks that an address >= some value means kernel.

>
> Also wouldn't the problem of high bit be solved by using only the
> 6 out of 7 available bits in UAI and leaving the 63rd bit alone?
> The hardware will still ignore the top bit, but this should take
> care of the requirement of high bit being 0/1 for user/kernel in the
> x86_64 kernel. Wouldn't that work?

Maybe, but that seems quite ugly.  This will make the userspace and kernel semantics diverge.

>
>> 
>> UAI looks like it wasn't intended to be context switched and, indeed, your series doesn't context switch it.  As far as I'm concerned, this is an error, and if we support UAI at all, we should context switch it.  Yes, this will be slow, perhaps painfully slow.  AMD knows how to fix it by, for example, reading the Intel SDM.  By *not* context switching UAI, we force it on for all user code, including unsuspecting user code, as well as for kernel code.  Do we actually want it on for kernel code?  With LAM, in contrast, the semantics for kernel pointers vs user pointers actually make sense and can be set per mm, which will make things like io_uring (in theory) do the right thing.
>
> I plan to enable/disable UAI based on the next task's settings by
> doing MSR write to EFER during context switch. I will have to measure
> how much additional cost an MSR write in context switch path brings in.
> However given that without a hardware feature like ARM64 MTE, this would
> primarily be used in non-production environments. Hence I wonder if MSR
> write cost could be tolerated?

I'm not sure what you mean by a feature like ARM64 MTE.

>
> Regarding enabling UAI for kernel, I will have to check how clean and
> efficient it would be to disable/enable UAI on user/kernel entry/exit
> points.
>
>> 
>> UAI and LAM are incompatible from a userspace perspective.  Since LAM is pretty clearly superior [0], it seems like a better long term outcome would be for programs that want tag bits to target LAM and for AMD to support LAM if there is demand.  For that matter, do we actually expect any userspace to want to support UAI?  (Are there existing too-clever sandboxes that would be broken by enabling UAI?)
>> 
>> Given that UAI is not efficiently context switched, the implementation of uaccess is rather bizarre.  With the implementation in this series in particular, if the access_ok checks ever get out of sync with actual user access, a user access could be emitted with the high bits not masked despite the range check succeeding due to masking, which would, unless great care is taken, result in a "user" access hitting the kernel range.  That's no good.
>
> Okay, I guess if context switching and sticking to 6 bits as mentioned
> earlier is feasible, this concern too goes away unless I am missing something.

I think it does go away.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Peter Zijlstra]

On Wed, Mar 23, 2022 at 01:18:41PM +0530, Bharata B Rao wrote:
> On 3/22/2022 3:59 AM, Andy Lutomirski wrote:

> > I hate to be a pain, but I'm really not convinced that this feature
> > is suitable for Linux.  There are a few reasons:
> > 
> > Right now, the concept that the high bit of an address determines
> > whether it's a user or a kernel address is fairly fundamental to the
> > x86_64 (and x86_32!) code.  It may not be strictly necessary to
> > preserve this, but violating it would require substantial thought.
> > With UAI enabled, kernel and user addresses are, functionally,
> > interleaved.  This makes things like access_ok checks, and more
> > generally anything that operates on a range of addresses, behave
> > potentially quite differently.  A lot of auditing of existing code
> > would be needed to make it safe.
> 
> Ok got that. However can you point to me a few instances in the current
> kernel code where such assumption of high bit being user/kernel address
> differentiator exists so that I get some idea of what it takes to
> audit all such cases?

The fact that you have to ask and can't readily find them should be a
big honking clue on its own, no?

Anyway, see here:

arch/x86/events/perf_event.h:static inline bool kernel_ip(unsigned long ip)
arch/x86/events/perf_event.h:{
arch/x86/events/perf_event.h:#ifdef CONFIG_X86_32
arch/x86/events/perf_event.h:   return ip > PAGE_OFFSET;
arch/x86/events/perf_event.h:#else
arch/x86/events/perf_event.h:   return (long)ip < 0;
arch/x86/events/perf_event.h:#endif
arch/x86/events/perf_event.h:}

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Bharata B Rao]

On 4/5/2022 1:44 PM, Peter Zijlstra wrote:
> On Wed, Mar 23, 2022 at 01:18:41PM +0530, Bharata B Rao wrote:
>> On 3/22/2022 3:59 AM, Andy Lutomirski wrote:
> 
>>> I hate to be a pain, but I'm really not convinced that this feature
>>> is suitable for Linux.  There are a few reasons:
>>>
>>> Right now, the concept that the high bit of an address determines
>>> whether it's a user or a kernel address is fairly fundamental to the
>>> x86_64 (and x86_32!) code.  It may not be strictly necessary to
>>> preserve this, but violating it would require substantial thought.
>>> With UAI enabled, kernel and user addresses are, functionally,
>>> interleaved.  This makes things like access_ok checks, and more
>>> generally anything that operates on a range of addresses, behave
>>> potentially quite differently.  A lot of auditing of existing code
>>> would be needed to make it safe.
>>
>> Ok got that. However can you point to me a few instances in the current
>> kernel code where such assumption of high bit being user/kernel address
>> differentiator exists so that I get some idea of what it takes to
>> audit all such cases?
> 
> The fact that you have to ask and can't readily find them should be a
> big honking clue on its own, no?
> 
> Anyway, see here:
> 
> arch/x86/events/perf_event.h:static inline bool kernel_ip(unsigned long ip)
> arch/x86/events/perf_event.h:{
> arch/x86/events/perf_event.h:#ifdef CONFIG_X86_32
> arch/x86/events/perf_event.h:   return ip > PAGE_OFFSET;
> arch/x86/events/perf_event.h:#else
> arch/x86/events/perf_event.h:   return (long)ip < 0;
> arch/x86/events/perf_event.h:#endif
> arch/x86/events/perf_event.h:}

That's a pretty good and clear example.

Thanks Peter. I do now see that auditing all such instances would be
an uphill task.

Regards,
Bharata.

Re: [RFC PATCH v0 0/6] x86/AMD: Userspace address tagging

[Catalin Marinas]

On Mon, Mar 21, 2022 at 03:29:34PM -0700, Andy Lutomirski wrote:
> On Thu, Mar 10, 2022, at 3:15 AM, Bharata B Rao wrote:
> > This patchset makes use of Upper Address Ignore (UAI) feature available
> > on upcoming AMD processors to provide user address tagging support for x86/AMD.
> >
> > UAI allows software to store a tag in the upper 7 bits of a logical
> > address [63:57]. When enabled, the processor will suppress the
> > traditional canonical address checks on the addresses. More information
> > about UAI can be found in section 5.10 of 'AMD64 Architecture
> > Programmer's Manual, Vol 2: System Programming' which is available from
> >
> > https://bugzilla.kernel.org/attachment.cgi?id=300549
> 
> I hate to be a pain, but I'm really not convinced that this feature is
> suitable for Linux.  There are a few reasons:
> 
> Right now, the concept that the high bit of an address determines
> whether it's a user or a kernel address is fairly fundamental to the
> x86_64 (and x86_32!) code.  It may not be strictly necessary to
> preserve this, but violating it would require substantial thought.
> With UAI enabled, kernel and user addresses are, functionally,
> interleaved.  This makes things like access_ok checks, and more
> generally anything that operates on a range of addresses, behave
> potentially quite differently.  A lot of auditing of existing code
> would be needed to make it safe.

Just catching up with this thread. I'm not entirely familiar with the
x86 codebase but some points from the arm64 TBI (top-byte ignore)
feature that may be useful:

In the 52-bit VA configuration (maximum) the kernel addresses on arm64
start at 0xfff00000_00000000 and the user ones go up to
0x000fffff_ffffffff. Anything in between these addresses would trigger a
fault on access. So a non-zero top-byte, even with bit 63 set, would not
access any kernel address unless bits 52 to 63 are all 1 (and this would
fail the access_ok() check, see below).

On arm64 we had TBI from day 0 but the syscall ABI did not allow user
tagged pointers into the kernel. An access_ok() checking addr < TASK_SIZE
was sufficient. With the tagged address ABI, we wanted to allow user
addresses with a non-zero top byte into the kernel. The access_ok() was
changed to sign-extend from bit 55 before comparing with TASK_SIZE. The
hardware also uses bit 55 to select the user or the kernel page tables
(TTBR0/TTBR1_EL1 regs or current->mm->pgd vs swapper_pg_dir in Linux
terms).

I haven't looked at the AMD UAI feature but if it still selects the user
vs kernel page tables based on bit 63, there may be a potential problem.
However, if access_ok() ensures that bit 56 is 0 for valid user
addresses, such access would fault as it's below the kernel's
0xff000000_00000000 limit (if I got it correctly for x86).

Since the UAI goes from bit 57 and up, I have a suspicion that it keeps
bit 56 for user vs kernel address selection. An access_ok()
sign-extending from this bit should be sufficient. As I said above,
there's no risk if such addresses get past access_ok(). With bit 56
cleared they'd not be able to access any kernel data.

(that's unless I missed something in the x86 kernel address layout)

> UAI looks like it wasn't intended to be context switched and, indeed,
> your series doesn't context switch it.  As far as I'm concerned, this
> is an error, and if we support UAI at all, we should context switch
> it.  Yes, this will be slow, perhaps painfully slow.  AMD knows how to
> fix it by, for example, reading the Intel SDM.  By *not* context
> switching UAI, we force it on for all user code, including
> unsuspecting user code, as well as for kernel code.  Do we actually
> want it on for kernel code?  With LAM, in contrast, the semantics for
> kernel pointers vs user pointers actually make sense and can be set
> per mm, which will make things like io_uring (in theory) do the right
> thing.

Arm64 does not context switch the hardware TBI feature either (and it
was always on from the start). A reason is that it requires expensive
TLB maintenance. What we do context switch is the opt-in to the tagged
address ABI which allows tagged pointers into the kernel. That's purely
a software choice (TIF flag) and it only affects the access_ok() check.

With KASAN enabled, we enable the TBI feature for the kernel as well,
it is independently controlled from the user one.

> UAI and LAM are incompatible from a userspace perspective.  Since LAM
> is pretty clearly superior [0], it seems like a better long term
> outcome would be for programs that want tag bits to target LAM and for
> AMD to support LAM if there is demand.  For that matter, do we
> actually expect any userspace to want to support UAI?  (Are there
> existing too-clever sandboxes that would be broken by enabling UAI?)

From the arm64 experience, there were some thoughts in the early days
that JIT compilers may use the top byte in the generated code to store
pointer type etc. IIRC, this was never deployed. But subsequently we got
MTE (memory tagging extensions) that uses bits 56 to 59 as a tag checked
against the memory colour during access. This seems to be more useful.

There is ASan as well that can make use of the top bits but it requires
recompilation (in principle, for MTE you just need the libc to return
tagged pointers on malloc()).

Of course, there can be ABI surprises and we came across some, see
commit dcde237319e6 ("mm: Avoid creating virtual address aliases in
brk()/mmap()/mremap()").

> Given that UAI is not efficiently context switched, the implementation
> of uaccess is rather bizarre.  With the implementation in this series
> in particular, if the access_ok checks ever get out of sync with
> actual user access, a user access could be emitted with the high bits
> not masked despite the range check succeeding due to masking, which
> would, unless great care is taken, result in a "user" access hitting
> the kernel range.  That's no good.

Per my (mis)understanding of the x86 address space, I don't think it's
that bad if you are careful with bit 56 always being 0 for user
addresses. The user can't generate a valid kernel address that goes past
access_ok().

-- 
Catalin