Skip to contentSkip to conversation

arm64: Fix syzkaller splat in ioremap_prot()

PATCH Applied Will Deacon 2 participants 6 messages Feb 23 – Feb 26 arm64

lore
Share & tools
lore

Will Deacon

author maintainer
#0 lore raw

cover arm64: Fix syzkaller splat in ioremap_prot()

Hi folks,

These two patches aim to resolve the syzkaller splat reported by Jinjiang at [1].

Rather than introduce a new arch hook, as was proposed in [1], this series instead augments the existing ioremap_prot() hook on arm64.

Cheers,

Will

[1] https://lore.kernel.org/r/[email protected]

Cc Zeng Heng <[email protected]>
Cc Jinjiang Tu <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Andrew Morton <[email protected]>

--->8

Will Deacon (2):
arm64: io: Rename ioremap_prot() to __ioremap_prot()
arm64: io: Extract user memory type in ioremap_prot()

 arch/arm64/include/asm/io.h | 26 ++++++++++++++++++++------
 arch/arm64/kernel/acpi.c    |  2 +-
 arch/arm64/mm/ioremap.c     |  6 +++---
 3 files changed, 24 insertions(+), 10 deletions(-)
Signature · 1 line
2.53.0.371.g1d285c8824-goog
author maintainer
#1 lore raw

PATCH 1/2 arm64: io: Rename ioremap_prot() to __ioremap_prot()

Rename our ioremap_prot() implementation to __ioremap_prot() and convert all arch-internal callers over to the new function.

ioremap_prot() remains as a #define to __ioremap_prot() for generic_access_phys() and will be subsequently extended to handle user permissions in 'prot'.

Cc Zeng Heng <[email protected]>
Cc Jinjiang Tu <[email protected]>
Cc Catalin Marinas <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm64/include/asm/io.h | 11 ++++++-----
 arch/arm64/kernel/acpi.c    |  2 +-
 arch/arm64/mm/ioremap.c     |  6 +++---
 3 files changed, 10 insertions(+), 9 deletions(-)
arch/arm64/include/asm/io.h +6 -5
index 83e03abbb2ca..cd2fddfe814a 100644
@@ -264,19 +264,20 @@ __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 typedef int (*ioremap_prot_hook_t)(phys_addr_t phys_addr, size_t size,
 				   pgprot_t *prot);
 int arm64_ioremap_prot_hook_register(const ioremap_prot_hook_t hook);
+void __iomem *__ioremap_prot(phys_addr_t phys, size_t size, pgprot_t prot);
 
-#define ioremap_prot ioremap_prot
+#define ioremap_prot __ioremap_prot
 
 #define _PAGE_IOREMAP PROT_DEVICE_nGnRE
 
 #define ioremap_wc(addr, size)	\
-	ioremap_prot((addr), (size), __pgprot(PROT_NORMAL_NC))
+	__ioremap_prot((addr), (size), __pgprot(PROT_NORMAL_NC))
 #define ioremap_np(addr, size)	\
-	ioremap_prot((addr), (size), __pgprot(PROT_DEVICE_nGnRnE))
+	__ioremap_prot((addr), (size), __pgprot(PROT_DEVICE_nGnRnE))
 
 
 #define ioremap_encrypted(addr, size)	\
-	ioremap_prot((addr), (size), PAGE_KERNEL)
+	__ioremap_prot((addr), (size), PAGE_KERNEL)
 
 /*
  * io{read,write}{16,32,64}be() macros

@@ -297,7 +298,7 @@ static inline void __iomem *ioremap_cache(phys_addr_t addr, size_t size)
 	if (pfn_is_map_memory(__phys_to_pfn(addr)))
 		return (void __iomem *)__phys_to_virt(addr);
 
-	return ioremap_prot(addr, size, __pgprot(PROT_NORMAL));
+	return __ioremap_prot(addr, size, __pgprot(PROT_NORMAL));
 }
 
 /*
arch/arm64/kernel/acpi.c +1 -1
index af90128cfed5..a9d884fd1d00 100644
@@ -377,7 +377,7 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 				prot = __acpi_get_writethrough_mem_attribute();
 		}
 	}
-	return ioremap_prot(phys, size, prot);
+	return __ioremap_prot(phys, size, prot);
 }
 
 /*
arch/arm64/mm/ioremap.c +3 -3
index b12cbed9b5ad..6eef48ef6278 100644
@@ -14,8 +14,8 @@ int arm64_ioremap_prot_hook_register(ioremap_prot_hook_t hook)
 	return 0;
 }
 
-void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
-			   pgprot_t pgprot)
+void __iomem *__ioremap_prot(phys_addr_t phys_addr, size_t size,
+			     pgprot_t pgprot)
 {
 	unsigned long last_addr = phys_addr + size - 1;
 

@@ -39,7 +39,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 
 	return generic_ioremap_prot(phys_addr, size, pgprot);
 }
-EXPORT_SYMBOL(ioremap_prot);
+EXPORT_SYMBOL(__ioremap_prot);
 
 /*
  * Must be called after early_fixmap_init
Signature · 1 line
2.53.0.371.g1d285c8824-goog
maintainer Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] arm64: io: Rename ioremap_prot() to __ioremap_prot()

Quoted 11 lines · Will Deacon

Rename our ioremap_prot() implementation to __ioremap_prot() and convert all arch-internal callers over to the new function.

ioremap_prot() remains as a #define to __ioremap_prot() for generic_access_phys() and will be subsequently extended to handle user permissions in 'prot'.

Cc Zeng Heng <[email protected]>
Cc Jinjiang Tu <[email protected]>
Cc Catalin Marinas <[email protected]>
Signed-off-by Will Deacon <[email protected]>
Reviewed-by Catalin Marinas <[email protected]>
author maintainer
#3 lore raw

PATCH 2/2 arm64: io: Extract user memory type in ioremap_prot()

In reply to Will Deacon

The only caller of ioremap_prot() outside of the generic ioremap() implementation is generic_access_phys(), which passes a 'pgprot_t' value determined from the user mapping of the target 'pfn' being accessed by the kernel. On arm64, the 'pgprot_t' contains all of the non-address bits from the pte, including the permission controls, and so we end up returning a new user mapping from ioremap_prot() which faults when accessed from the kernel on systems with PAN:

  | Unable to handle kernel read from unreadable memory at virtual address ffff80008ea89000
  | ...
  | Call trace:
  |   __memcpy_fromio+0x80/0xf8
  |   generic_access_phys+0x20c/0x2b8
  |   __access_remote_vm+0x46c/0x5b8
  |   access_remote_vm+0x18/0x30
  |   environ_read+0x238/0x3e8
  |   vfs_read+0xe4/0x2b0
  |   ksys_read+0xcc/0x178
  |   __arm64_sys_read+0x4c/0x68

Extract only the memory type from the user 'pgprot_t' in ioremap_prot() and assert that we're being passed a user mapping, to protect us against any changes in future that may require additional handling. To avoid falsely flagging users of ioremap(), provide our own ioremap() macro which simply wraps __ioremap_prot().

Cc Zeng Heng <[email protected]>
Cc Jinjiang Tu <[email protected]>
Cc Catalin Marinas <[email protected]>
Fixes 893dea9ccd08 ("arm64: Add HAVE_IOREMAP_PROT support")
Reported-by Jinjiang Tu <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm64/include/asm/io.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
arch/arm64/include/asm/io.h +15 -2
index cd2fddfe814a..8cbd1e96fd50 100644
@@ -266,10 +266,23 @@ typedef int (*ioremap_prot_hook_t)(phys_addr_t phys_addr, size_t size,
 int arm64_ioremap_prot_hook_register(const ioremap_prot_hook_t hook);
 void __iomem *__ioremap_prot(phys_addr_t phys, size_t size, pgprot_t prot);
 
-#define ioremap_prot __ioremap_prot
+static inline void __iomem *ioremap_prot(phys_addr_t phys, size_t size,
+					 pgprot_t user_prot)
+{
+	pgprot_t prot;
+	ptdesc_t user_prot_val = pgprot_val(user_prot);
 
-#define _PAGE_IOREMAP PROT_DEVICE_nGnRE
+	if (WARN_ON_ONCE(!(user_prot_val & PTE_USER)))
+		return NULL;
 
+	prot = __pgprot_modify(PAGE_KERNEL, PTE_ATTRINDX_MASK,
+			       user_prot_val & PTE_ATTRINDX_MASK);
+	return __ioremap_prot(phys, size, prot);
+}
+#define ioremap_prot ioremap_prot
+
+#define ioremap(addr, size)	\
+	__ioremap_prot((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
 #define ioremap_wc(addr, size)	\
 	__ioremap_prot((addr), (size), __pgprot(PROT_NORMAL_NC))
 #define ioremap_np(addr, size)	\
Signature · 1 line
2.53.0.371.g1d285c8824-goog
maintainer Reviewed-by on v1 2/2
#4 lore raw

Re: [PATCH 2/2] arm64: io: Extract user memory type in ioremap_prot()

Quoted 26 lines · Will Deacon

The only caller of ioremap_prot() outside of the generic ioremap() implementation is generic_access_phys(), which passes a 'pgprot_t' value determined from the user mapping of the target 'pfn' being accessed by the kernel. On arm64, the 'pgprot_t' contains all of the non-address bits from the pte, including the permission controls, and so we end up returning a new user mapping from ioremap_prot() which faults when accessed from the kernel on systems with PAN:

  | Unable to handle kernel read from unreadable memory at virtual address ffff80008ea89000
  | ...
  | Call trace:
  |   __memcpy_fromio+0x80/0xf8
  |   generic_access_phys+0x20c/0x2b8
  |   __access_remote_vm+0x46c/0x5b8
  |   access_remote_vm+0x18/0x30
  |   environ_read+0x238/0x3e8
  |   vfs_read+0xe4/0x2b0
  |   ksys_read+0xcc/0x178
  |   __arm64_sys_read+0x4c/0x68

Extract only the memory type from the user 'pgprot_t' in ioremap_prot() and assert that we're being passed a user mapping, to protect us against any changes in future that may require additional handling. To avoid falsely flagging users of ioremap(), provide our own ioremap() macro which simply wraps __ioremap_prot().

Quoted 6 lines
Cc Zeng Heng <[email protected]>
Cc Jinjiang Tu <[email protected]>
Cc Catalin Marinas <[email protected]>
Fixes 893dea9ccd08 ("arm64: Add HAVE_IOREMAP_PROT support")
Reported-by Jinjiang Tu <[email protected]>
Signed-off-by Will Deacon <[email protected]>
Reviewed-by Catalin Marinas <[email protected]>
author maintainer Applied
#5 lore raw
In reply to Will Deacon
Quoted 9 lines · Will Deacon

These two patches aim to resolve the syzkaller splat reported by Jinjiang at [1].

Rather than introduce a new arch hook, as was proposed in [1], this series instead augments the existing ioremap_prot() hook on arm64.

Cheers,

[...]

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

[1/2] arm64: io: Rename ioremap_prot() to __ioremap_prot()

      https://git.kernel.org/arm64/c/f6bf47ab32e0
[2/2] arm64: io: Extract user memory type in ioremap_prot()
      https://git.kernel.org/arm64/c/8f098037139b

Cheers,

Signature · 5 lines
Will

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