From e2ed9cf79a3ec462c170ec564391b288d3068ad9 Mon Sep 17 00:00:00 2001 From: Tom Stellard Date: Wed, 11 Oct 2017 16:57:38 -0700 Subject: [PATCH] Don't mark heap memory as executable v3 Marking heap memory as executable using mprotect() is not allowed by SELinux. mprotect() calls that try to do this will fail on systems with SELinux enabled. This is also a security risk, so it should be fixed even on systems that allow this. Any memory we want to mark as executable must be allocated using mmap(). See https://www.akkadia.org/drepper/selinux-mem.html The two places where we try to mark heap memory as executable both use posix_memalign() to allocate the heap memory. In both cases, the alignment value passed into this function is always equal to PAGE_SIZE, which means that they are safe to replace with mmap(), which guarantees alignment to PAGE_SIZE. In this case PAGE_SIZE has been set to sysconf(_SC_PAGESIZE); v2: - Use MAP_PRIVATE instead of MAP_SHARED. This matches the behavior of memory allocated by posix_memalign() - Ignore alignment hints instead of returning error when we can't accommodate them. - Drop alignment parameter of allocate_exec_aligned_memory() since the only alignment supported is sysconf(_SC_PAGESIZE). - Remove extra parameter from fmm_release(). - Add error path to fmm_allocate_host_cpu() for when mmap fails. v3: - Avoid use after free. Change-Id: I7d51279790d9700bc3fa761c44bfde1c1936019b Signed-off-by: Felix Kuehling --- src/fmm.c | 34 +++++++++++++++++----------------- src/queues.c | 33 ++++++++++++++++----------------- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/fmm.c b/src/fmm.c index 5756df4e62..cc5c4cb1a6 100644 --- a/src/fmm.c +++ b/src/fmm.c @@ -1110,26 +1110,22 @@ void *fmm_allocate_doorbell(uint32_t gpu_id, uint64_t MemorySizeInBytes, static void *fmm_allocate_host_cpu(uint64_t MemorySizeInBytes, HsaMemFlags flags) { - int err; - HSAuint64 page_size; void *mem = NULL; vm_object_t *vm_obj; + int mmap_prot = PROT_READ | PROT_WRITE; - page_size = PageSizeFromFlags(flags.ui32.PageSize); - err = posix_memalign(&mem, page_size, MemorySizeInBytes); - if (err != 0) + if (flags.ui32.ExecuteAccess) + mmap_prot |= PROT_EXEC; + + /* mmap will return a pointer with alignment equal to + * sysconf(_SC_PAGESIZE). + */ + mem = mmap(NULL, MemorySizeInBytes, mmap_prot, + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + + if (mem == MAP_FAILED) return NULL; - if (flags.ui32.ExecuteAccess) { - err = mprotect(mem, MemorySizeInBytes, - PROT_READ | PROT_WRITE | PROT_EXEC); - - if (err != 0) { - free(mem); - return NULL; - } - } - pthread_mutex_lock(&cpuvm_aperture.fmm_mutex); vm_obj = aperture_allocate_object(&cpuvm_aperture, mem, 0, MemorySizeInBytes, flags.Value); @@ -1354,14 +1350,18 @@ void fmm_release(void *address) * refers to the system memory */ if (!found) { + uint64_t size = 0; /* Release the vm object in CPUVM */ pthread_mutex_lock(&cpuvm_aperture.fmm_mutex); object = vm_find_object_by_address(&cpuvm_aperture, address, 0); - if (object) + if (object) { + size = object->size; vm_remove_object(&cpuvm_aperture, object); + } pthread_mutex_unlock(&cpuvm_aperture.fmm_mutex); /* Free the memory from the system */ - free(address); + if (size) + munmap(address, size); } } diff --git a/src/queues.c b/src/queues.c index 7765e76a36..c698c5ca6e 100644 --- a/src/queues.c +++ b/src/queues.c @@ -309,21 +309,20 @@ static HSAKMT_STATUS map_doorbell(HSAuint32 NodeId, HSAuint32 gpu_id, return status; } -static void *allocate_exec_aligned_memory_cpu(uint32_t size, uint32_t align) +static void *allocate_exec_aligned_memory_cpu(uint32_t size) { void *ptr; - int retval; - retval = posix_memalign(&ptr, align, size); - if (retval != 0) - return NULL; + /* mmap will return a pointer with alignment equal to + * sysconf(_SC_PAGESIZE). + * + * MAP_ANONYMOUS initializes the memory to zero. + */ + ptr = mmap(NULL, size, PROT_READ | PROT_WRITE | PROT_EXEC, + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); - retval = mprotect(ptr, size, PROT_READ | PROT_WRITE | PROT_EXEC); - if (retval != 0) { - free(ptr); + if (ptr == MAP_FAILED) return NULL; - } - memset(ptr, 0, size); return ptr; } @@ -395,15 +394,17 @@ void free_exec_aligned_memory_gpu(void *addr, uint32_t size, uint32_t align) hsaKmtFreeMemory(addr, size); } +/* + * Allocates memory aligned to sysconf(_SC_PAGESIZE) + */ static void *allocate_exec_aligned_memory(uint32_t size, - uint32_t align, enum asic_family_type type, uint32_t NodeId) { if (IS_DGPU(type)) - return allocate_exec_aligned_memory_gpu(size, align, NodeId, + return allocate_exec_aligned_memory_gpu(size, PAGE_SIZE, NodeId, false); - return allocate_exec_aligned_memory_cpu(size, align); + return allocate_exec_aligned_memory_cpu(size); } static void free_exec_aligned_memory(void *addr, uint32_t size, uint32_t align, @@ -412,7 +413,7 @@ static void free_exec_aligned_memory(void *addr, uint32_t size, uint32_t align, if (IS_DGPU(type)) free_exec_aligned_memory_gpu(addr, size, align); else - free(addr); + munmap(addr, size); } static void free_queue(struct queue *q) @@ -439,7 +440,6 @@ static int handle_concrete_asic(struct queue *q, if (dev_info->eop_buffer_size > 0) { q->eop_buffer = allocate_exec_aligned_memory(q->dev_info->eop_buffer_size, - PAGE_SIZE, dev_info->asic_family, NodeId); if (!q->eop_buffer) @@ -454,7 +454,6 @@ static int handle_concrete_asic(struct queue *q, args->ctl_stack_size = q->ctl_stack_size; q->ctx_save_restore = allocate_exec_aligned_memory(q->ctx_save_restore_size, - PAGE_SIZE, dev_info->asic_family, NodeId); if (!q->ctx_save_restore) @@ -506,7 +505,7 @@ HSAKMT_STATUS HSAKMTAPI hsaKmtCreateQueue(HSAuint32 NodeId, dev_info = get_device_info_by_dev_id(dev_id); struct queue *q = allocate_exec_aligned_memory(sizeof(*q), - PAGE_SIZE, dev_info->asic_family, + dev_info->asic_family, NodeId); if (!q) return HSAKMT_STATUS_NO_MEMORY;