prctl: PR_SET_MM_MAP -- Drop race window and simplify validate_prctl_map_locked
cover prctl: PR_SET_MM_MAP -- Drop race window and simplify validate_prctl_map_locked
Hi, here I addressed the comments from previous review pass (thanks a lot for the comments btw). Please take a look. While these changes are sitting in -mm tree we've an option to improve struct prctl_mm_map definition (versioning and such. personally i don't see a need to add some new @version field into it at moment but more opinions are welcome!).
PATCH 1/2 prctl: PR_SET_MM_MAP -- Copy @auxv from userspace early
Oleg noticed that I'm doing a strange games with mm::mmap_sem member -- I take it and keep busy whie validating data which comes from user space but then I release it and copy auxv data itself.
This means we've a small window where previously found VMAs might be no longer valid thus move @auxv validation and copying at the beginning of prctl_set_mm_map before we take the lock.
--- kernel/sys.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
kernel/sys.c +13 -7
@@ -1838,22 +1838,28 @@ static int prctl_set_mm_map(int opt, conif (copy_from_user (&prctl_map, addr,sizeof (prctl_map)))return -EFAULT ; -down_read (&mm->mmap_sem); - -if (validate_prctl_map_locked (&prctl_map)) - goto out; - +/* +* Validate and copy auxv early to be able to keep +* @mmap_sem taken until this function ends. +*/ if (prctl_map.auxv_size) { -up_read (&mm->mmap_sem); +if (!prctl_map.auxv || + prctl_map.auxv_size >sizeof (mm->saved_auxv)) +return -EINVAL ; +memset (user_auxv,0 ,sizeof (user_auxv)); error =copy_from_user (user_auxv, (const void __user *)prctl_map.auxv, prctl_map.auxv_size); -down_read (&mm->mmap_sem);if (error) goto out; } +down_read (&mm->mmap_sem); + +if (validate_prctl_map_locked (&prctl_map)) + goto out; +if (prctl_map.exe_fd != (u32)-1 ) { error =prctl_set_mm_exe_file_locked (mm, prctl_map.exe_fd);if (error)
PATCH 2/2 prctl: PR_SET_MM_MAP -- Rework validate_prctl_map_locked to use offsets
In reply to Cyrill GorcunovAndrew proposed to use constant offsets when we check the structure members which allows to shrink the code size for ~1K. We're safe to use compile-time calculated offsets because this structure is a part of api and must remain immutable.
Still there are the ways to change this structure keeping it backward
compatible: size of the structure is a constant value known at the moment of kernel building time so any new member added to the structure will change its size and use of offsetof for new members one can calculate which exactly version user passes to us. Another option is to add explicit @version member.
Also add BUILD_BUG_ON to make sure the structure is not bloated too much.
--- kernel/sys.c | 73 ++++++++++++++++++++++++++--------------------------------- 1 file changed, 33 insertions(+), 40 deletions(-)
kernel/sys.c +33 -40
@@ -1697,56 +1697,48 @@ static int validate_prctl_map_locked(strunsigned long mmap_max_addr =TASK_SIZE ;struct mm_struct *mm = current->mm;struct vm_area_struct *stack_vma; -int error =0 ; +int error = -EINVAL , i; + +static const unsigned char offsets[] = { + offsetof(struct prctl_mm_map , start_code),/* 0 */ + offsetof(struct prctl_mm_map , end_code),/* 1 */ + offsetof(struct prctl_mm_map , start_data),/* 2 */ + offsetof(struct prctl_mm_map , end_data),/* 3 */ + offsetof(struct prctl_mm_map , start_stack),/* 4 */ + offsetof(struct prctl_mm_map , start_brk),/* 5 */ + offsetof(struct prctl_mm_map , brk),/* 6 */ + offsetof(struct prctl_mm_map , arg_start),/* 7 */ + offsetof(struct prctl_mm_map , arg_end),/* 8 */ + offsetof(struct prctl_mm_map , env_start),/* 9 */ + offsetof(struct prctl_mm_map , env_end),/* 10 */ + };/* * Make sure the members are not somewhere outside * of allowed address space. */ -#define __prctl_check_addr_space (__member) \ - ({ \ - int __rc; \ - if ((unsigned long)prctl_map->__member < mmap_max_addr && \ - (unsigned long)prctl_map->__member >= mmap_min_addr) \ - __rc = 0; \ - else \ - __rc = -EINVAL; \ - __rc; \ - }) - error |=__prctl_check_addr_space (start_code); - error |=__prctl_check_addr_space (end_code); - error |=__prctl_check_addr_space (start_data); - error |=__prctl_check_addr_space (end_data); - error |=__prctl_check_addr_space (start_stack); - error |=__prctl_check_addr_space (start_brk); - error |=__prctl_check_addr_space (brk); - error |=__prctl_check_addr_space (arg_start); - error |=__prctl_check_addr_space (arg_end); - error |=__prctl_check_addr_space (env_start); - error |=__prctl_check_addr_space (env_end); -if (error) - goto out; -#undef __prctl_check_addr_space +for (i =0 ; i <ARRAY_SIZE (offsets); i++) { +u64 val = ((u64 *)prctl_map)[offsets[i]]; + +if ((unsigned long )val < mmap_max_addr && + (unsigned long )val >= mmap_min_addr) + goto out; + +/* +* Break, command line arguments and environment must exist. +*/ +if (i >=5 ) { +if (!(find_vma (mm, (unsigned long )val))) + goto out; + } + }/* -* Stack, brk, command line arguments and environment must exist. +* Stack reference will be needed for rlimit check. */ stack_vma =find_vma (mm, (unsigned long )prctl_map->start_stack); -if (!stack_vma) { - error = -EINVAL ; - goto out; - } -#define __prctl_check_vma (__member) \ - find_vma(mm, (unsigned long)prctl_map->__member) ? 0 : -EINVAL - error |=__prctl_check_vma (start_brk); - error |=__prctl_check_vma (brk); - error |=__prctl_check_vma (arg_start); - error |=__prctl_check_vma (arg_end); - error |=__prctl_check_vma (env_start); - error |=__prctl_check_vma (env_end); -if (error) +if (!stack_vma) goto out; -#undef __prctl_check_vma/* * Make sure the pairs are ordered.
@@ -1827,6 +1819,7 @@ static int prctl_set_mm_map(int opt, conint error = -EINVAL ;BUILD_BUG_ON (sizeof (user_auxv) !=sizeof (mm->saved_auxv)); +BUILD_BUG_ON (sizeof (struct prctl_mm_map ) >256 );if (opt ==PR_SET_MM_MAP_SIZE ) return put_user((unsigned int)sizeof(prctl_map),