diff mbox series

[v14,5/8] arm64: mte: Enable TCO in functions that can read beyond buffer limits

Message ID 20210308161434.33424-6-vincenzo.frascino@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: ARMv8.5-A: MTE: Add async mode support | expand

Commit Message

Vincenzo Frascino March 8, 2021, 4:14 p.m. UTC 
load_unaligned_zeropad() and __get/put_kernel_nofault() functions can
read passed some buffer limits which may include some MTE granule with a
different tag.

When MTE async mode is enable, the load operation crosses the boundaries
and the next granule has a different tag the PE sets the TFSR_EL1.TF1 bit
as if an asynchronous tag fault is happened.

Enable Tag Check Override (TCO) in these functions  before the load and
disable it afterwards to prevent this to happen.

Note: The same condition can be hit in MTE sync mode but we deal with it
through the exception handling.
In the current implementation, mte_async_mode flag is set only at boot
time but in future kasan might acquire some runtime features that
that change the mode dynamically, hence we disable it when sync mode is
selected for future proof.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reported-by: Branislav Rankov <Branislav.Rankov@arm.com>
Tested-by: Branislav Rankov <Branislav.Rankov@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/uaccess.h        | 24 ++++++++++++++++++++++++
 arch/arm64/include/asm/word-at-a-time.h |  4 ++++
 arch/arm64/kernel/mte.c                 | 22 ++++++++++++++++++++++
 3 files changed, 50 insertions(+)

Comments

Mark Rutland March 8, 2021, 6:09 p.m. UTC | #1 
On Mon, Mar 08, 2021 at 04:14:31PM +0000, Vincenzo Frascino wrote:
> load_unaligned_zeropad() and __get/put_kernel_nofault() functions can
> read passed some buffer limits which may include some MTE granule with a
> different tag.

s/passed/past/

> When MTE async mode is enable, the load operation crosses the boundaries

s/enabel/enabled/

> and the next granule has a different tag the PE sets the TFSR_EL1.TF1 bit
> as if an asynchronous tag fault is happened.
> 
> Enable Tag Check Override (TCO) in these functions  before the load and
> disable it afterwards to prevent this to happen.
> 
> Note: The same condition can be hit in MTE sync mode but we deal with it
> through the exception handling.
> In the current implementation, mte_async_mode flag is set only at boot
> time but in future kasan might acquire some runtime features that
> that change the mode dynamically, hence we disable it when sync mode is
> selected for future proof.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Reported-by: Branislav Rankov <Branislav.Rankov@arm.com>
> Tested-by: Branislav Rankov <Branislav.Rankov@arm.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/include/asm/uaccess.h        | 24 ++++++++++++++++++++++++
>  arch/arm64/include/asm/word-at-a-time.h |  4 ++++
>  arch/arm64/kernel/mte.c                 | 22 ++++++++++++++++++++++
>  3 files changed, 50 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 0deb88467111..a857f8f82aeb 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -188,6 +188,26 @@ static inline void __uaccess_enable_tco(void)
>  				 ARM64_MTE, CONFIG_KASAN_HW_TAGS));
>  }
>  
> +/* Whether the MTE asynchronous mode is enabled. */
> +DECLARE_STATIC_KEY_FALSE(mte_async_mode);

Can we please hide this behind something like:

static inline bool system_uses_mte_async_mode(void)
{
	return IS_ENABLED(CONFIG_KASAN_HW_TAGS) &&
		static_branch_unlikely(&mte_async_mode);
}

... like we do for system_uses_ttbr0_pan()?

That way the callers are easier to read, and kernels built without
CONFIG_KASAN_HW_TAGS don't have the static branch at all. I reckon you
can put that in one of hte mte headers and include it where needed.

Thanks,
Mark.

> +
> +/*
> + * These functions disable tag checking only if in MTE async mode
> + * since the sync mode generates exceptions synchronously and the
> + * nofault or load_unaligned_zeropad can handle them.
> + */
> +static inline void __uaccess_disable_tco_async(void)
> +{
> +	if (static_branch_unlikely(&mte_async_mode))
> +		 __uaccess_disable_tco();
> +}
> +
> +static inline void __uaccess_enable_tco_async(void)
> +{
> +	if (static_branch_unlikely(&mte_async_mode))
> +		__uaccess_enable_tco();
> +}
> +
>  static inline void uaccess_disable_privileged(void)
>  {
>  	__uaccess_disable_tco();
> @@ -307,8 +327,10 @@ do {									\
>  do {									\
>  	int __gkn_err = 0;						\
>  									\
> +	__uaccess_enable_tco_async();					\
>  	__raw_get_mem("ldr", *((type *)(dst)),				\
>  		      (__force type *)(src), __gkn_err);		\
> +	__uaccess_disable_tco_async();					\
>  	if (unlikely(__gkn_err))					\
>  		goto err_label;						\
>  } while (0)
> @@ -380,8 +402,10 @@ do {									\
>  do {									\
>  	int __pkn_err = 0;						\
>  									\
> +	__uaccess_enable_tco_async();					\
>  	__raw_put_mem("str", *((type *)(src)),				\
>  		      (__force type *)(dst), __pkn_err);		\
> +	__uaccess_disable_tco_async();					\
>  	if (unlikely(__pkn_err))					\
>  		goto err_label;						\
>  } while(0)
> diff --git a/arch/arm64/include/asm/word-at-a-time.h b/arch/arm64/include/asm/word-at-a-time.h
> index 3333950b5909..c62d9fa791aa 100644
> --- a/arch/arm64/include/asm/word-at-a-time.h
> +++ b/arch/arm64/include/asm/word-at-a-time.h
> @@ -55,6 +55,8 @@ static inline unsigned long load_unaligned_zeropad(const void *addr)
>  {
>  	unsigned long ret, offset;
>  
> +	__uaccess_enable_tco_async();
> +
>  	/* Load word from unaligned pointer addr */
>  	asm(
>  	"1:	ldr	%0, %3\n"
> @@ -76,6 +78,8 @@ static inline unsigned long load_unaligned_zeropad(const void *addr)
>  	: "=&r" (ret), "=&r" (offset)
>  	: "r" (addr), "Q" (*(unsigned long *)addr));
>  
> +	__uaccess_disable_tco_async();
> +
>  	return ret;
>  }
>  
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index fa755cf94e01..1ad9be4c8376 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -26,6 +26,10 @@ u64 gcr_kernel_excl __ro_after_init;
>  
>  static bool report_fault_once = true;
>  
> +/* Whether the MTE asynchronous mode is enabled. */
> +DEFINE_STATIC_KEY_FALSE(mte_async_mode);
> +EXPORT_SYMBOL_GPL(mte_async_mode);
> +
>  static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
>  {
>  	pte_t old_pte = READ_ONCE(*ptep);
> @@ -118,12 +122,30 @@ static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
>  
>  void mte_enable_kernel_sync(void)
>  {
> +	/*
> +	 * Make sure we enter this function when no PE has set
> +	 * async mode previously.
> +	 */
> +	WARN_ONCE(static_key_enabled(&mte_async_mode),
> +			"MTE async mode enabled system wide!");
> +
>  	__mte_enable_kernel("synchronous", SCTLR_ELx_TCF_SYNC);
>  }
>  
>  void mte_enable_kernel_async(void)
>  {
>  	__mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC);
> +
> +	/*
> +	 * MTE async mode is set system wide by the first PE that
> +	 * executes this function.
> +	 *
> +	 * Note: If in future KASAN acquires a runtime switching
> +	 * mode in between sync and async, this strategy needs
> +	 * to be reviewed.
> +	 */
> +	if (!static_branch_unlikely(&mte_async_mode))
> +		static_branch_enable(&mte_async_mode);
>  }
>  
>  void mte_set_report_once(bool state)
> -- 
> 2.30.0
>
kernel test robot March 8, 2021, 9:58 p.m. UTC | #2 
Hi Vincenzo,

I love your patch! Yet something to improve:

[auto build test ERROR on kvmarm/next]
[also build test ERROR on linus/master v5.12-rc2]
[cannot apply to arm64/for-next/core xlnx/master arm/for-next soc/for-next hnaz-linux-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vincenzo-Frascino/arm64-ARMv8-5-A-MTE-Add-async-mode-support/20210309-001716
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-randconfig-r006-20210308 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/660df126323fe5533a1be7834e1754a1adc69f13
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vincenzo-Frascino/arm64-ARMv8-5-A-MTE-Add-async-mode-support/20210309-001716
        git checkout 660df126323fe5533a1be7834e1754a1adc69f13
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> aarch64-linux-ld: mm/maccess.o:(__jump_table+0x8): undefined reference to `mte_async_mode'
   aarch64-linux-ld: mm/maccess.o:(__jump_table+0x18): undefined reference to `mte_async_mode'
   aarch64-linux-ld: mm/maccess.o:(__jump_table+0x28): undefined reference to `mte_async_mode'
   aarch64-linux-ld: mm/maccess.o:(__jump_table+0x38): undefined reference to `mte_async_mode'
   aarch64-linux-ld: mm/maccess.o:(__jump_table+0x48): undefined reference to `mte_async_mode'
   aarch64-linux-ld: mm/maccess.o:(__jump_table+0x58): more undefined references to `mte_async_mode' follow

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot March 9, 2021, 12:28 a.m. UTC | #3 
Hi Vincenzo,

I love your patch! Yet something to improve:

[auto build test ERROR on kvmarm/next]
[also build test ERROR on linus/master v5.12-rc2 next-20210305]
[cannot apply to arm64/for-next/core xlnx/master arm/for-next soc/for-next hnaz-linux-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vincenzo-Frascino/arm64-ARMv8-5-A-MTE-Add-async-mode-support/20210309-001716
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-randconfig-r021-20210308 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 3a11a41795bec548e91621caaa4cc00fc31b2212)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/660df126323fe5533a1be7834e1754a1adc69f13
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vincenzo-Frascino/arm64-ARMv8-5-A-MTE-Add-async-mode-support/20210309-001716
        git checkout 660df126323fe5533a1be7834e1754a1adc69f13
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: mte_async_mode
   >>> referenced by maccess.c
   >>>               maccess.o:(copy_from_kernel_nofault) in archive mm/built-in.a
   >>> referenced by maccess.c
   >>>               maccess.o:(copy_from_kernel_nofault) in archive mm/built-in.a
   >>> referenced by maccess.c
   >>>               maccess.o:(copy_from_kernel_nofault) in archive mm/built-in.a
   >>> referenced 62 more times

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Vincenzo Frascino March 9, 2021, 10:26 a.m. UTC | #4 
On 3/8/21 6:09 PM, Mark Rutland wrote:
>> +DECLARE_STATIC_KEY_FALSE(mte_async_mode);
> Can we please hide this behind something like:
> 
> static inline bool system_uses_mte_async_mode(void)
> {
> 	return IS_ENABLED(CONFIG_KASAN_HW_TAGS) &&
> 		static_branch_unlikely(&mte_async_mode);
> }
> 
> ... like we do for system_uses_ttbr0_pan()?
>

I agree, it is a cleaner solution. I will add it to v15.

> That way the callers are easier to read, and kernels built without
> CONFIG_KASAN_HW_TAGS don't have the static branch at all. I reckon you
> can put that in one of hte mte headers and include it where needed.
kernel test robot March 9, 2021, 10:40 p.m. UTC | #5 
Hi Vincenzo,

I love your patch! Yet something to improve:

[auto build test ERROR on kvmarm/next]
[also build test ERROR on linus/master v5.12-rc2 next-20210309]
[cannot apply to arm64/for-next/core xlnx/master arm/for-next soc/for-next hnaz-linux-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vincenzo-Frascino/arm64-ARMv8-5-A-MTE-Add-async-mode-support/20210309-001716
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-randconfig-s032-20210309 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-262-g5e674421-dirty
        # https://github.com/0day-ci/linux/commit/660df126323fe5533a1be7834e1754a1adc69f13
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vincenzo-Frascino/arm64-ARMv8-5-A-MTE-Add-async-mode-support/20210309-001716
        git checkout 660df126323fe5533a1be7834e1754a1adc69f13
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   aarch64-linux-ld: mm/maccess.o: in function `copy_from_kernel_nofault':
>> maccess.c:(.text+0x340): undefined reference to `mte_async_mode'
   maccess.c:(.text+0x340): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
>> aarch64-linux-ld: maccess.c:(.text+0x344): undefined reference to `mte_async_mode'
   aarch64-linux-ld: maccess.c:(.text+0x44c): undefined reference to `mte_async_mode'
   maccess.c:(.text+0x44c): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   aarch64-linux-ld: maccess.c:(.text+0x450): undefined reference to `mte_async_mode'
   aarch64-linux-ld: maccess.c:(.text+0x474): undefined reference to `mte_async_mode'
   aarch64-linux-ld: mm/maccess.o:maccess.c:(.text+0x4d0): more undefined references to `mte_async_mode' follow
   mm/maccess.o: in function `copy_from_kernel_nofault':
   maccess.c:(.text+0x4d0): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   maccess.c:(.text+0x550): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   mm/maccess.o: in function `copy_to_kernel_nofault':
   maccess.c:(.text+0x6cc): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   maccess.c:(.text+0x7d8): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   maccess.c:(.text+0x864): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   maccess.c:(.text+0x8ec): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   mm/maccess.o: in function `strncpy_from_kernel_nofault':
   maccess.c:(.text+0xaac): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   fs/namei.o: in function `full_name_hash':
   namei.c:(.text+0x28): relocation truncated to fit: R_AARCH64_ADR_PREL_PG_HI21 against undefined symbol `mte_async_mode'
   fs/namei.o: in function `hashlen_string':
   namei.c:(.text+0x2a28): additional relocation overflows omitted from the output

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 0deb88467111..a857f8f82aeb 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -188,6 +188,26 @@  static inline void __uaccess_enable_tco(void)
 				 ARM64_MTE, CONFIG_KASAN_HW_TAGS));
 }
 
+/* Whether the MTE asynchronous mode is enabled. */
+DECLARE_STATIC_KEY_FALSE(mte_async_mode);
+
+/*
+ * These functions disable tag checking only if in MTE async mode
+ * since the sync mode generates exceptions synchronously and the
+ * nofault or load_unaligned_zeropad can handle them.
+ */
+static inline void __uaccess_disable_tco_async(void)
+{
+	if (static_branch_unlikely(&mte_async_mode))
+		 __uaccess_disable_tco();
+}
+
+static inline void __uaccess_enable_tco_async(void)
+{
+	if (static_branch_unlikely(&mte_async_mode))
+		__uaccess_enable_tco();
+}
+
 static inline void uaccess_disable_privileged(void)
 {
 	__uaccess_disable_tco();
@@ -307,8 +327,10 @@  do {									\
 do {									\
 	int __gkn_err = 0;						\
 									\
+	__uaccess_enable_tco_async();					\
 	__raw_get_mem("ldr", *((type *)(dst)),				\
 		      (__force type *)(src), __gkn_err);		\
+	__uaccess_disable_tco_async();					\
 	if (unlikely(__gkn_err))					\
 		goto err_label;						\
 } while (0)
@@ -380,8 +402,10 @@  do {									\
 do {									\
 	int __pkn_err = 0;						\
 									\
+	__uaccess_enable_tco_async();					\
 	__raw_put_mem("str", *((type *)(src)),				\
 		      (__force type *)(dst), __pkn_err);		\
+	__uaccess_disable_tco_async();					\
 	if (unlikely(__pkn_err))					\
 		goto err_label;						\
 } while(0)
diff --git a/arch/arm64/include/asm/word-at-a-time.h b/arch/arm64/include/asm/word-at-a-time.h
index 3333950b5909..c62d9fa791aa 100644
--- a/arch/arm64/include/asm/word-at-a-time.h
+++ b/arch/arm64/include/asm/word-at-a-time.h
@@ -55,6 +55,8 @@  static inline unsigned long load_unaligned_zeropad(const void *addr)
 {
 	unsigned long ret, offset;
 
+	__uaccess_enable_tco_async();
+
 	/* Load word from unaligned pointer addr */
 	asm(
 	"1:	ldr	%0, %3\n"
@@ -76,6 +78,8 @@  static inline unsigned long load_unaligned_zeropad(const void *addr)
 	: "=&r" (ret), "=&r" (offset)
 	: "r" (addr), "Q" (*(unsigned long *)addr));
 
+	__uaccess_disable_tco_async();
+
 	return ret;
 }
 
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index fa755cf94e01..1ad9be4c8376 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -26,6 +26,10 @@  u64 gcr_kernel_excl __ro_after_init;
 
 static bool report_fault_once = true;
 
+/* Whether the MTE asynchronous mode is enabled. */
+DEFINE_STATIC_KEY_FALSE(mte_async_mode);
+EXPORT_SYMBOL_GPL(mte_async_mode);
+
 static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap)
 {
 	pte_t old_pte = READ_ONCE(*ptep);
@@ -118,12 +122,30 @@  static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
 
 void mte_enable_kernel_sync(void)
 {
+	/*
+	 * Make sure we enter this function when no PE has set
+	 * async mode previously.
+	 */
+	WARN_ONCE(static_key_enabled(&mte_async_mode),
+			"MTE async mode enabled system wide!");
+
 	__mte_enable_kernel("synchronous", SCTLR_ELx_TCF_SYNC);
 }
 
 void mte_enable_kernel_async(void)
 {
 	__mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC);
+
+	/*
+	 * MTE async mode is set system wide by the first PE that
+	 * executes this function.
+	 *
+	 * Note: If in future KASAN acquires a runtime switching
+	 * mode in between sync and async, this strategy needs
+	 * to be reviewed.
+	 */
+	if (!static_branch_unlikely(&mte_async_mode))
+		static_branch_enable(&mte_async_mode);
 }
 
 void mte_set_report_once(bool state)