| Message ID | 20190430081844.22597-3-mkoutny@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reduce mmap_sem usage for args manipulation | expand |
On 30.04.2019 11:18, Michal Koutný wrote: > Despite comment of validate_prctl_map claims there are no capability > checks, it is not completely true since commit 4d28df6152aa ("prctl: > Allow local CAP_SYS_ADMIN changing exe_file"). Extract the check out of > the function and make the function perform purely arithmetic checks. > > This patch should not change any behavior, it is mere refactoring for > following patch. > > CC: Kirill Tkhai <ktkhai@virtuozzo.com> > CC: Cyrill Gorcunov <gorcunov@gmail.com> > Signed-off-by: Michal Koutný <mkoutny@suse.com> Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com> > --- > kernel/sys.c | 45 ++++++++++++++++++++------------------------- > 1 file changed, 20 insertions(+), 25 deletions(-) > > diff --git a/kernel/sys.c b/kernel/sys.c > index 12df0e5434b8..e1acb444d7b0 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1882,10 +1882,12 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd) > } > > /* > + * Check arithmetic relations of passed addresses. > + * > * WARNING: we don't require any capability here so be very careful > * in what is allowed for modification from userspace. > */ > -static int validate_prctl_map(struct prctl_mm_map *prctl_map) > +static int validate_prctl_map_addr(struct prctl_mm_map *prctl_map) > { > unsigned long mmap_max_addr = TASK_SIZE; > struct mm_struct *mm = current->mm; > @@ -1949,24 +1951,6 @@ static int validate_prctl_map(struct prctl_mm_map *prctl_map) > prctl_map->start_data)) > goto out; > > - /* > - * Someone is trying to cheat the auxv vector. > - */ > - if (prctl_map->auxv_size) { > - if (!prctl_map->auxv || prctl_map->auxv_size > sizeof(mm->saved_auxv)) > - goto out; > - } > - > - /* > - * Finally, make sure the caller has the rights to > - * change /proc/pid/exe link: only local sys admin should > - * be allowed to. > - */ > - if (prctl_map->exe_fd != (u32)-1) { > - if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) > - goto out; > - } > - > error = 0; > out: > return error; > @@ -1993,11 +1977,17 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data > if (copy_from_user(&prctl_map, addr, sizeof(prctl_map))) > return -EFAULT; > > - error = validate_prctl_map(&prctl_map); > + error = validate_prctl_map_addr(&prctl_map); > if (error) > return error; > > if (prctl_map.auxv_size) { > + /* > + * Someone is trying to cheat the auxv vector. > + */ > + if (!prctl_map.auxv || prctl_map.auxv_size > sizeof(mm->saved_auxv)) > + return -EINVAL; > + > memset(user_auxv, 0, sizeof(user_auxv)); > if (copy_from_user(user_auxv, > (const void __user *)prctl_map.auxv, > @@ -2010,6 +2000,14 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data > } > > if (prctl_map.exe_fd != (u32)-1) { > + /* > + * Make sure the caller has the rights to > + * change /proc/pid/exe link: only local sys admin should > + * be allowed to. > + */ > + if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) > + return -EINVAL; > + > error = prctl_set_mm_exe_file(mm, prctl_map.exe_fd); > if (error) > return error; > @@ -2097,7 +2095,7 @@ static int prctl_set_mm(int opt, unsigned long addr, > unsigned long arg4, unsigned long arg5) > { > struct mm_struct *mm = current->mm; > - struct prctl_mm_map prctl_map; > + struct prctl_mm_map prctl_map = { .auxv = NULL, .auxv_size = 0, .exe_fd = -1 }; > struct vm_area_struct *vma; > int error; > > @@ -2139,9 +2137,6 @@ static int prctl_set_mm(int opt, unsigned long addr, > prctl_map.arg_end = mm->arg_end; > prctl_map.env_start = mm->env_start; > prctl_map.env_end = mm->env_end; > - prctl_map.auxv = NULL; > - prctl_map.auxv_size = 0; > - prctl_map.exe_fd = -1; > > switch (opt) { > case PR_SET_MM_START_CODE: > @@ -2181,7 +2176,7 @@ static int prctl_set_mm(int opt, unsigned long addr, > goto out; > } > > - error = validate_prctl_map(&prctl_map); > + error = validate_prctl_map_addr(&prctl_map); > if (error) > goto out; > >
diff --git a/kernel/sys.c b/kernel/sys.c index 12df0e5434b8..e1acb444d7b0 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1882,10 +1882,12 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd) } /* + * Check arithmetic relations of passed addresses. + * * WARNING: we don't require any capability here so be very careful * in what is allowed for modification from userspace. */ -static int validate_prctl_map(struct prctl_mm_map *prctl_map) +static int validate_prctl_map_addr(struct prctl_mm_map *prctl_map) { unsigned long mmap_max_addr = TASK_SIZE; struct mm_struct *mm = current->mm; @@ -1949,24 +1951,6 @@ static int validate_prctl_map(struct prctl_mm_map *prctl_map) prctl_map->start_data)) goto out; - /* - * Someone is trying to cheat the auxv vector. - */ - if (prctl_map->auxv_size) { - if (!prctl_map->auxv || prctl_map->auxv_size > sizeof(mm->saved_auxv)) - goto out; - } - - /* - * Finally, make sure the caller has the rights to - * change /proc/pid/exe link: only local sys admin should - * be allowed to. - */ - if (prctl_map->exe_fd != (u32)-1) { - if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) - goto out; - } - error = 0; out: return error; @@ -1993,11 +1977,17 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data if (copy_from_user(&prctl_map, addr, sizeof(prctl_map))) return -EFAULT; - error = validate_prctl_map(&prctl_map); + error = validate_prctl_map_addr(&prctl_map); if (error) return error; if (prctl_map.auxv_size) { + /* + * Someone is trying to cheat the auxv vector. + */ + if (!prctl_map.auxv || prctl_map.auxv_size > sizeof(mm->saved_auxv)) + return -EINVAL; + memset(user_auxv, 0, sizeof(user_auxv)); if (copy_from_user(user_auxv, (const void __user *)prctl_map.auxv, @@ -2010,6 +2000,14 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data } if (prctl_map.exe_fd != (u32)-1) { + /* + * Make sure the caller has the rights to + * change /proc/pid/exe link: only local sys admin should + * be allowed to. + */ + if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) + return -EINVAL; + error = prctl_set_mm_exe_file(mm, prctl_map.exe_fd); if (error) return error; @@ -2097,7 +2095,7 @@ static int prctl_set_mm(int opt, unsigned long addr, unsigned long arg4, unsigned long arg5) { struct mm_struct *mm = current->mm; - struct prctl_mm_map prctl_map; + struct prctl_mm_map prctl_map = { .auxv = NULL, .auxv_size = 0, .exe_fd = -1 }; struct vm_area_struct *vma; int error; @@ -2139,9 +2137,6 @@ static int prctl_set_mm(int opt, unsigned long addr, prctl_map.arg_end = mm->arg_end; prctl_map.env_start = mm->env_start; prctl_map.env_end = mm->env_end; - prctl_map.auxv = NULL; - prctl_map.auxv_size = 0; - prctl_map.exe_fd = -1; switch (opt) { case PR_SET_MM_START_CODE: @@ -2181,7 +2176,7 @@ static int prctl_set_mm(int opt, unsigned long addr, goto out; } - error = validate_prctl_map(&prctl_map); + error = validate_prctl_map_addr(&prctl_map); if (error) goto out;
Despite comment of validate_prctl_map claims there are no capability checks, it is not completely true since commit 4d28df6152aa ("prctl: Allow local CAP_SYS_ADMIN changing exe_file"). Extract the check out of the function and make the function perform purely arithmetic checks. This patch should not change any behavior, it is mere refactoring for following patch. CC: Kirill Tkhai <ktkhai@virtuozzo.com> CC: Cyrill Gorcunov <gorcunov@gmail.com> Signed-off-by: Michal Koutný <mkoutny@suse.com> --- kernel/sys.c | 45 ++++++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 25 deletions(-)