From d93ed095640345495ace3b653ea87b66815c7c81 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Thu, 2 Aug 2012 09:32:33 +0100 Subject: [PATCH] Improved safety for user memory accesses. * Changed IS_USER_ADDRESS to check an address using USER_BASE and USER_SIZE, rather than just !IS_KERNEL_ADDRESS. The old check would allow user buffers to point into the physical memory map area. * Added an unmapped hole at the end of the bottom half of the address space which catches buffers that cross into the uncanonical address region. This also removes the need to check for uncanonical return addresses in the syscall handler, it is no longer possible for the return address to be uncanonical under normal circumstances. All cases in which the return address might be changed by the kernel are still handled via the IRET path. --- headers/private/kernel/arch/arm/arch_kernel.h | 4 ++-- headers/private/kernel/arch/m68k/arch_kernel.h | 4 ++-- headers/private/kernel/arch/mipsel/arch_kernel.h | 4 ++-- headers/private/kernel/arch/ppc/arch_kernel.h | 4 ++-- headers/private/kernel/arch/x86/arch_kernel.h | 15 +++++++++------ headers/private/kernel/kernel.h | 9 ++++++++- src/system/kernel/arch/x86/64/interrupts.S | 13 ------------- 7 files changed, 25 insertions(+), 28 deletions(-) diff --git a/headers/private/kernel/arch/arm/arch_kernel.h b/headers/private/kernel/arch/arm/arch_kernel.h index af0a9e26f7..44a05e74a6 100644 --- a/headers/private/kernel/arch/arm/arch_kernel.h +++ b/headers/private/kernel/arch/arm/arch_kernel.h @@ -23,10 +23,10 @@ #define USER_BASE 0x100000 #define USER_BASE_ANY USER_BASE #define USER_SIZE (0x80000000 - (0x10000 + 0x100000)) -#define USER_TOP (USER_BASE + USER_SIZE) +#define USER_TOP (USER_BASE + (USER_SIZE - 1)) #define KERNEL_USER_DATA_BASE 0x6fff0000 #define USER_STACK_REGION 0x70000000 -#define USER_STACK_REGION_SIZE (USER_TOP - USER_STACK_REGION) +#define USER_STACK_REGION_SIZE ((USER_TOP - USER_STACK_REGION) + 1) #endif /* _KERNEL_ARCH_ARM_KERNEL_H */ diff --git a/headers/private/kernel/arch/m68k/arch_kernel.h b/headers/private/kernel/arch/m68k/arch_kernel.h index 85e9168d33..7f9806999a 100644 --- a/headers/private/kernel/arch/m68k/arch_kernel.h +++ b/headers/private/kernel/arch/m68k/arch_kernel.h @@ -23,10 +23,10 @@ #define USER_BASE 0x100000 #define USER_BASE_ANY USER_BASE #define USER_SIZE (0x80000000 - (0x10000 + 0x100000)) -#define USER_TOP (USER_BASE + USER_SIZE) +#define USER_TOP (USER_BASE + (USER_SIZE - 1)) #define KERNEL_USER_DATA_BASE 0x6fff0000 #define USER_STACK_REGION 0x70000000 -#define USER_STACK_REGION_SIZE (USER_TOP - USER_STACK_REGION) +#define USER_STACK_REGION_SIZE ((USER_TOP - USER_STACK_REGION) + 1) #endif /* _KERNEL_ARCH_M68K_KERNEL_H */ diff --git a/headers/private/kernel/arch/mipsel/arch_kernel.h b/headers/private/kernel/arch/mipsel/arch_kernel.h index 6eea165c55..42f3c8fbaf 100644 --- a/headers/private/kernel/arch/mipsel/arch_kernel.h +++ b/headers/private/kernel/arch/mipsel/arch_kernel.h @@ -26,11 +26,11 @@ #define USER_BASE 0x100000 #define USER_BASE_ANY USER_BASE #define USER_SIZE (0x80000000 - (0x10000 + 0x100000)) -#define USER_TOP (USER_BASE + USER_SIZE) +#define USER_TOP (USER_BASE + (USER_SIZE - 1)) #define KERNEL_USER_DATA_BASE 0x6fff0000 #define USER_STACK_REGION 0x70000000 -#define USER_STACK_REGION_SIZE (USER_TOP - USER_STACK_REGION) +#define USER_STACK_REGION_SIZE ((USER_TOP - USER_STACK_REGION) + 1) #endif /* _KERNEL_ARCH_MIPSEL_KERNEL_H */ diff --git a/headers/private/kernel/arch/ppc/arch_kernel.h b/headers/private/kernel/arch/ppc/arch_kernel.h index 3886034f11..c7d448084b 100644 --- a/headers/private/kernel/arch/ppc/arch_kernel.h +++ b/headers/private/kernel/arch/ppc/arch_kernel.h @@ -23,10 +23,10 @@ #define USER_BASE 0x100000 #define USER_BASE_ANY USER_BASE #define USER_SIZE (0x80000000 - (0x10000 + 0x100000)) -#define USER_TOP (USER_BASE + USER_SIZE) +#define USER_TOP (USER_BASE + (USER_SIZE - 1)) #define KERNEL_USER_DATA_BASE 0x6fff0000 #define USER_STACK_REGION 0x70000000 -#define USER_STACK_REGION_SIZE (USER_TOP - USER_STACK_REGION) +#define USER_STACK_REGION_SIZE ((USER_TOP - USER_STACK_REGION) + 1) #endif /* _KERNEL_ARCH_PPC_KERNEL_H */ diff --git a/headers/private/kernel/arch/x86/arch_kernel.h b/headers/private/kernel/arch/x86/arch_kernel.h index 87e676b58d..366852526d 100644 --- a/headers/private/kernel/arch/x86/arch_kernel.h +++ b/headers/private/kernel/arch/x86/arch_kernel.h @@ -40,14 +40,17 @@ #define KERNEL_PMAP_SIZE 0x8000000000 // Userspace address space layout. +// There is a 2MB hole just before the end of the bottom half of the address +// space. This means that if userland passes in a buffer that crosses into the +// uncanonical address region, it will be caught through a page fault. #define USER_BASE 0x0 #define USER_BASE_ANY 0x100000 -#define USER_SIZE 0x800000000000 -#define USER_TOP (USER_BASE + USER_SIZE) +#define USER_SIZE (0x800000000000 - 0x200000) +#define USER_TOP (USER_BASE + (USER_SIZE - 1)) #define KERNEL_USER_DATA_BASE 0x7fffefff0000 #define USER_STACK_REGION 0x7ffff0000000 -#define USER_STACK_REGION_SIZE (USER_TOP - USER_STACK_REGION) +#define USER_STACK_REGION_SIZE ((USER_TOP - USER_STACK_REGION) + 1) #else // __x86_64__ @@ -68,14 +71,14 @@ * TODO: introduce the 1Mb lower barrier again - it's only used for vm86 mode, * and this should be moved into the kernel (and address space) completely. */ -#define USER_BASE 0x00 +#define USER_BASE 0x0 #define USER_BASE_ANY 0x100000 #define USER_SIZE (KERNEL_BASE - 0x10000) -#define USER_TOP (USER_BASE + USER_SIZE) +#define USER_TOP (USER_BASE + (USER_SIZE - 1)) #define KERNEL_USER_DATA_BASE 0x6fff0000 #define USER_STACK_REGION 0x70000000 -#define USER_STACK_REGION_SIZE (USER_TOP - USER_STACK_REGION) +#define USER_STACK_REGION_SIZE ((USER_TOP - USER_STACK_REGION) + 1) #endif // __x86_64__ diff --git a/headers/private/kernel/kernel.h b/headers/private/kernel/kernel.h index ec5a166f56..a5c01e37ea 100644 --- a/headers/private/kernel/kernel.h +++ b/headers/private/kernel/kernel.h @@ -31,7 +31,14 @@ #endif // Buffers passed in from user-space shouldn't point into the kernel. -#define IS_USER_ADDRESS(x) (!IS_KERNEL_ADDRESS(x)) +#if USER_BASE == 0 +# define IS_USER_ADDRESS(x) ((addr_t)(x) <= USER_TOP) +#elif USER_TOP == __HAIKU_ADDR_MAX +# define IS_USER_ADDRESS(x) ((addr_t)(x) >= USER_BASE) +#else +# define IS_USER_ADDRESS(x) \ + ((addr_t)(x) >= USER_BASE && (addr_t)(x) <= USER_TOP) +#endif #define DEBUG_KERNEL_STACKS // Note, debugging kernel stacks doesn't really work yet. Since the diff --git a/src/system/kernel/arch/x86/64/interrupts.S b/src/system/kernel/arch/x86/64/interrupts.S index acc82a083b..a74b7b4633 100644 --- a/src/system/kernel/arch/x86/64/interrupts.S +++ b/src/system/kernel/arch/x86/64/interrupts.S @@ -393,19 +393,6 @@ FUNCTION(x86_64_syscall_entry): cmpq $SYSCALL_RESTORE_SIGNAL_FRAME, %r14 je .Liret - // If the return address is not canonical, return using the IRET path. - // On Intel's implementation of SYSRET, the canonical address check for the - // return address is performed before the switch to user mode, so a fault - // for a non-canonical address will be triggered in kernel mode. However, by - // then we will have switched onto the user stack meaning the kernel could - // be tricked into executing with the user RSP. Using IRET will handle - // non-canonical addresses properly. - movq IFRAME_user_sp(%rsp), %rax - sarq $47, %rax - incl %eax - cmpl $1, %eax - ja .Liret - // Restore the iframe and RCX/R11 for SYSRET. RESTORE_IFRAME() pop %rcx