From a82cdd3456ed014f213f07f3791aed83df12ab2f Mon Sep 17 00:00:00 2001 From: Bahadir Balban Date: Tue, 5 Feb 2008 15:41:14 +0000 Subject: [PATCH] User pointer validity checks. Added routines that check whether a user pointer is accessible by the kernel, and if not ask the pager to map-in those pages. I haven't implemented yet the bit that asks the pager for paging-in. --- include/l4/arch/arm/exception.h | 3 + include/l4/generic/space.h | 2 + include/l4/glue/arm/memory.h | 6 ++ src/api/kip.c | 31 +++----- src/arch/arm/exception.c | 16 +++- src/arch/arm/v5/mm.c | 129 ++++++++++++++++++++------------ src/generic/SConscript | 2 +- src/generic/space.c | 38 ++++++++++ 8 files changed, 155 insertions(+), 72 deletions(-) create mode 100644 src/generic/space.c diff --git a/include/l4/arch/arm/exception.h b/include/l4/arch/arm/exception.h index dd1fb9b..f8f0f2f 100644 --- a/include/l4/arch/arm/exception.h +++ b/include/l4/arch/arm/exception.h @@ -85,4 +85,7 @@ static inline int in_user() return !in_kernel(); } +int pager_pagein_request(unsigned long vaddr, unsigned long size, + unsigned int flags); + #endif diff --git a/include/l4/generic/space.h b/include/l4/generic/space.h index 09291fc..085a19a 100644 --- a/include/l4/generic/space.h +++ b/include/l4/generic/space.h @@ -18,4 +18,6 @@ #define MAP_SVC_DEFAULT_FLAGS MAP_SVC_RW_FLAGS #define MAP_IO_DEFAULT_FLAGS MAP_SVC_IO_FLAGS +int check_access(unsigned long vaddr, unsigned long size, unsigned int flags); + #endif /* __SPACE_H__ */ diff --git a/include/l4/glue/arm/memory.h b/include/l4/glue/arm/memory.h index 0dca263..fcb4092 100644 --- a/include/l4/glue/arm/memory.h +++ b/include/l4/glue/arm/memory.h @@ -116,6 +116,12 @@ void remove_mapping(unsigned long vaddr); void remove_mapping_pgd(unsigned long vaddr, pgd_table_t *pgd); void prealloc_phys_pagedesc(void); +int check_mapping_pgd(unsigned long vaddr, unsigned long size, + unsigned int flags, pgd_table_t *pgd); + +int check_mapping(unsigned long vaddr, unsigned long size, + unsigned int flags); + void copy_pgd_kern_all(pgd_table_t *); pte_t virt_to_pte(unsigned long virtual); pte_t virt_to_pte_from_pgd(unsigned long virtual, pgd_table_t *pgd); diff --git a/src/api/kip.c b/src/api/kip.c index 5841084..66c5848 100644 --- a/src/api/kip.c +++ b/src/api/kip.c @@ -1,12 +1,12 @@ /* * Kernel Interface Page and sys_kdata_read() * - * Copyright (C) 2007 Bahadir Balban - * + * Copyright (C) 2007, 2008 Bahadir Balban */ - #include #include +#include +#include #include INC_API(kip.h) #include INC_API(syscall.h) #include INC_GLUE(memlayout.h) @@ -19,30 +19,25 @@ UNIT("kip") struct kip kip; int __sys_kread(int rd, void *dest) { int err = 0; + unsigned long vaddr = (unsigned long)dest; switch(rd) { case KDATA_PAGE_MAP: - /* - * FIXME:FIXME: Check if address is mapped here first!!! - * Also check if process has enough buffer for physmem to fit!!! - */ printk("Handling KDATA_PAGE_MAP request.\n"); + if (check_access(vaddr, sizeof(page_map), MAP_USR_RW_FLAGS) < 0) + return -EINVAL; memcpy(dest, &page_map, sizeof(page_map)); break; case KDATA_BOOTDESC: printk("Handling KDATA_BOOTDESC request.\n"); - /* - * FIXME:FIXME: Check if address is mapped here first!!! - * Also check if process has enough buffer for physmem to fit!!! - */ + if (check_access(vaddr, bootdesc->desc_size, MAP_USR_RW_FLAGS) < 0) + return -EINVAL; memcpy(dest, bootdesc, bootdesc->desc_size); break; case KDATA_BOOTDESC_SIZE: printk("Handling KDATA_BOOTDESC_SIZE request.\n"); - /* - * FIXME:FIXME: Check if address is mapped here first!!! - * Also check if process has enough buffer for physmem to fit!!! - */ + if (check_access(vaddr, sizeof(unsigned int), MAP_USR_RW_FLAGS) < 0) + return -EINVAL; *(unsigned int *)dest = bootdesc->desc_size; break; @@ -69,10 +64,8 @@ int sys_kread(struct syscall_args *a) int rd = (int)arg[0]; /* Request descriptor */ /* Error checking */ - if ((rd < 0) || (addr <= 0)) { - printk("%s: Invalid arguments.\n", __FUNCTION__); - return -1; - } + if (rd < 0) + return -EINVAL; return __sys_kread(rd, addr); } diff --git a/src/arch/arm/exception.c b/src/arch/arm/exception.c index a00cb68..d9532f8 100644 --- a/src/arch/arm/exception.c +++ b/src/arch/arm/exception.c @@ -1,7 +1,7 @@ /* - * Debug print support for unexpected exceptions + * Memory exception handling in process context. * - * Copyright (C) 2007 Bahadir Balban + * Copyright (C) 2007, 2008 Bahadir Balban */ #include #include @@ -62,11 +62,19 @@ void fault_ipc_to_pager(u32 faulty_pc, u32 fsr, u32 far) ipc_sendrecv(current->pagerid, current->pagerid); /* - * Pager is now notified and handling the fault. We now sleep on - * another queue. + * FIXME: CHECK TASK KILL REPLY !!! + * Here, pager has handled the request and sent us back a message. + * It is natural that a pager might want to kill the task due to + * illegal access. Here we ought to check this and kill it rather + * than return back to it. */ } +int pager_pagein_request(unsigned long addr, unsigned long size, unsigned int flags) +{ + return 0; +} + int check_aborts(u32 faulted_pc, u32 fsr, u32 far) { int ret = 0; diff --git a/src/arch/arm/v5/mm.c b/src/arch/arm/v5/mm.c index 81353e8..17ceed9 100644 --- a/src/arch/arm/v5/mm.c +++ b/src/arch/arm/v5/mm.c @@ -191,54 +191,6 @@ void attach_pmd(pgd_table_t *pgd, pmd_table_t *pmd, unsigned int vaddr) pgd->entry[pgd_i] |= PGD_TYPE_COARSE; } -/* - * Maps @paddr to @vaddr, covering @size bytes also allocates new pmd if - * necessary. This flavor explicitly supplies the pgd to modify. This is useful - * when modifying userspace of processes that are not currently running. (Only - * makes sense for userspace mappings since kernel mappings are common.) - */ -void add_mapping_pgd(unsigned int paddr, unsigned int vaddr, - unsigned int size, unsigned int flags, - pgd_table_t *pgd) -{ - pmd_table_t *pmd; - unsigned int numpages = (size >> PAGE_BITS); - - if (size < PAGE_SIZE) { - printascii("Error: Mapping size must be in bytes not pages.\n"); - while(1); - } - if (size & PAGE_MASK) - numpages++; - - /* Convert generic map flags to pagetable-specific */ - BUG_ON(!(flags = space_flags_to_ptflags(flags))); - - /* Map all consecutive pages that cover given size */ - for (int i = 0; i < numpages; i++) { - /* Check if another mapping already has a pmd attached. */ - pmd = pmd_exists(pgd, vaddr); - if (!pmd) { - /* - * If this is the first vaddr in - * this pmd, allocate new pmd - */ - pmd = alloc_pmd(); - - /* Attach pmd to its entry in pgd */ - attach_pmd(pgd, pmd, vaddr); - } - - /* Attach paddr to this pmd */ - __add_mapping(page_align(paddr), - page_align(vaddr), flags, pmd); - - /* Go to the next page to be mapped */ - paddr += PAGE_SIZE; - vaddr += PAGE_SIZE; - } -} - #if 0 /* Maps @paddr to @vaddr, covering @size bytes, * also allocates new pmd if necessary. */ @@ -326,12 +278,93 @@ void add_mapping(unsigned int paddr, unsigned int vaddr, } #endif +/* + * Maps @paddr to @vaddr, covering @size bytes also allocates new pmd if + * necessary. This flavor explicitly supplies the pgd to modify. This is useful + * when modifying userspace of processes that are not currently running. (Only + * makes sense for userspace mappings since kernel mappings are common.) + */ +void add_mapping_pgd(unsigned int paddr, unsigned int vaddr, + unsigned int size, unsigned int flags, + pgd_table_t *pgd) +{ + pmd_table_t *pmd; + unsigned int numpages = (size >> PAGE_BITS); + + if (size < PAGE_SIZE) { + printascii("Error: Mapping size must be in bytes not pages.\n"); + while(1); + } + if (size & PAGE_MASK) + numpages++; + + /* Convert generic map flags to pagetable-specific */ + BUG_ON(!(flags = space_flags_to_ptflags(flags))); + + /* Map all consecutive pages that cover given size */ + for (int i = 0; i < numpages; i++) { + /* Check if another mapping already has a pmd attached. */ + pmd = pmd_exists(pgd, vaddr); + if (!pmd) { + /* + * If this is the first vaddr in + * this pmd, allocate new pmd + */ + pmd = alloc_pmd(); + + /* Attach pmd to its entry in pgd */ + attach_pmd(pgd, pmd, vaddr); + } + + /* Attach paddr to this pmd */ + __add_mapping(page_align(paddr), + page_align(vaddr), flags, pmd); + + /* Go to the next page to be mapped */ + paddr += PAGE_SIZE; + vaddr += PAGE_SIZE; + } +} + void add_mapping(unsigned int paddr, unsigned int vaddr, unsigned int size, unsigned int flags) { add_mapping_pgd(paddr, vaddr, size, flags, current->pgd); } +/* + * Checks if a virtual address range has same or more permissive + * flags than the given ones, returns 0 if not, and 1 if OK. + */ +int check_mapping_pgd(unsigned long vaddr, unsigned long size, + unsigned int flags, pgd_table_t *pgd) +{ + unsigned int npages = __pfn(align_up(size, PAGE_SIZE)); + pte_t pte; + + /* Convert generic map flags to pagetable-specific */ + BUG_ON(!(flags = space_flags_to_ptflags(flags))); + + for (int i = 0; i < npages; i++) { + pte = virt_to_pte(vaddr + i * PAGE_SIZE); + + /* Check if pte perms are equal or gt given flags */ + if ((pte & PTE_PROT_MASK) >= (flags & PTE_PROT_MASK)) + continue; + else + return 0; + } + + return 1; +} + + +int check_mapping(unsigned long vaddr, unsigned long size, + unsigned int flags) +{ + return check_mapping_pgd(vaddr, size, flags, current->pgd); +} + /* FIXME: Empty PMDs should be returned here !!! */ void __remove_mapping(pmd_table_t *pmd, unsigned long vaddr) { diff --git a/src/generic/SConscript b/src/generic/SConscript index ffcff03..a32433d 100644 --- a/src/generic/SConscript +++ b/src/generic/SConscript @@ -4,7 +4,7 @@ Import('env') # The set of source files associated with this SConscript file. -src_local = ['physmem.c', 'irq.c', 'scheduler.c', 'time.c', 'tcb.c', 'pgalloc.c', 'kmalloc.c'] +src_local = ['physmem.c', 'irq.c', 'scheduler.c', 'time.c', 'tcb.c', 'pgalloc.c', 'kmalloc.c', 'space.c'] obj = env.Object(src_local) Return('obj') diff --git a/src/generic/space.c b/src/generic/space.c new file mode 100644 index 0000000..a29a7ab --- /dev/null +++ b/src/generic/space.c @@ -0,0 +1,38 @@ +/* + * Addess space related routines. + * + * Copyright (C) 2008 Bahadir Balban + */ +#include INC_GLUE(memory.h) +#include INC_GLUE(memlayout.h) +#include INC_ARCH(exception.h) +#include +#include +#include + +/* + * Checks whether the given user address is a valid userspace address. + * If so, whether it is currently mapped into its own address space. + * If its not mapped-in, it generates a page-in request to the thread's + * pager. If fault hasn't cleared, aborts. + */ +int check_access(unsigned long vaddr, unsigned long size, unsigned int flags) +{ + int err; + + /* Do not allow ridiculously big sizes */ + if (size >= USER_AREA_SIZE) + return -EINVAL; + + /* Check if in user range, but this is more up to the pager to decide */ + if (!(vaddr >= USER_AREA_START && vaddr < USER_AREA_END)) + return -EINVAL; + + /* If not mapped, ask pager whether this is possible */ + if (!check_mapping(vaddr, size, flags)) + if((err = pager_pagein_request(vaddr, size, flags)) < 0) + return err; + + return 0; +} +