Acked-by: Andrey Vagin <ava...@virtuozzo.com>
On Mon, May 16, 2016 at 11:28:51AM +0300, Cyrill Gorcunov wrote: > This is a backport of commit > > ML: bdb4d100afe9818aebd1d98ced575c5ef143456c > > From: Calvin Owens <calvinow...@fb.com> > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is > only exposed if CONFIG_CHECKPOINT_RESTORE is set. > > Each mapped file region gets a symlink in /proc/<pid>/map_files/ > corresponding to the virtual address range at which it is mapped. The > symlinks work like the symlinks in /proc/<pid>/fd/, so you can follow them > to the backing file even if that backing file has been unlinked. > > Currently, files which are mapped, unlinked, and closed are impossible to > stat() from userspace. Exposing /proc/<pid>/map_files/ closes this > functionality "hole". > > Not being able to stat() such files makes noticing and explicitly > accounting for the space they use on the filesystem impossible. You can > work around this by summing up the space used by every file in the > filesystem and subtracting that total from what statfs() tells you, but > that obviously isn't great, and it becomes unworkable once your filesystem > becomes large enough. > > This patch moves map_files/ out from behind CONFIG_CHECKPOINT_RESTORE, and > adjusts the permissions enforced on it as follows: > > * proc_map_files_lookup() > * proc_map_files_readdir() > * map_files_d_revalidate() > > Remove the CAP_SYS_ADMIN restriction, leaving only the current > restriction requiring PTRACE_MODE_READ. The information made > available to userspace by these three functions is already > available in /proc/PID/maps with MODE_READ, so I don't see any > reason to limit them any further (see below for more detail). > > * proc_map_files_follow_link() > > This stub has been added, and requires that the user have > CAP_SYS_ADMIN in order to follow the links in map_files/, > since there was concern on LKML both about the potential for > bypassing permissions on ancestor directories in the path to > files pointed to, and about what happens with more exotic > memory mappings created by some drivers (ie dma-buf). > > In older versions of this patch, I changed every permission check in > the four functions above to enforce MODE_ATTACH instead of MODE_READ. > This was an oversight on my part, and after revisiting the discussion > it seems that nobody was concerned about anything outside of what is > made possible by ->follow_link(). So in this version, I've left the > checks for PTRACE_MODE_READ as-is. > > [a...@linux-foundation.org: catch up with concurrent proc_pid_follow_link() > changes] > Signed-off-by: Calvin Owens <calvinow...@fb.com> > Reviewed-by: Kees Cook <keesc...@chromium.org> > Cc: Andy Lutomirski <l...@amacapital.net> > Cc: Cyrill Gorcunov <gorcu...@openvz.org> > Cc: Joe Perches <j...@perches.com> > Cc: Kirill A. Shutemov <kir...@shutemov.name> > Signed-off-by: Andrew Morton <a...@linux-foundation.org> > Signed-off-by: Linus Torvalds <torva...@linux-foundation.org> > Signed-off-by: Cyrill Gorcunov <gorcu...@openvz.org> > --- > > Kostya, please wait for Ack from Andrew. The patch on its own is not > bound to some of the bug we're working on now but usefull in general > and probably will help us with renaming of memfd restored memory > in criu (we use memfd to be able to restore anonymous shared memory > in userns case but memfd mangles the backend name, we didn't find > any problem with it yet, but been talking to Andrew and he agreed > that we might need to do something with this problem, and this patch > is first step). > > fs/proc/base.c | 44 +++++++++++++++++++++++--------------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > Index: linux-pcs7.git/fs/proc/base.c > =================================================================== > --- linux-pcs7.git.orig/fs/proc/base.c > +++ linux-pcs7.git/fs/proc/base.c > @@ -1925,8 +1925,6 @@ end_instantiate: > return filldir(dirent, name, len, filp->f_pos, ino, type); > } > > -#ifdef CONFIG_CHECKPOINT_RESTORE > - > /* > * dname_to_vma_addr - maps a dentry name into two unsigned longs > * which represent vma start and end addresses. > @@ -1953,11 +1951,6 @@ static int map_files_d_revalidate(struct > if (flags & LOOKUP_RCU) > return -ECHILD; > > - if (!capable(CAP_SYS_ADMIN)) { > - status = -EPERM; > - goto out_notask; > - } > - > inode = dentry->d_inode; > task = get_proc_task(inode); > if (!task) > @@ -2048,6 +2041,28 @@ struct map_files_info { > unsigned char name[4*sizeof(long)+2]; /* max: %lx-%lx\0 */ > }; > > +/* > + * Only allow CAP_SYS_ADMIN to follow the links, due to concerns about how > the > + * symlinks may be used to bypass permissions on ancestor directories in the > + * path to the file in question. > + */ > +static void *proc_map_files_follow_link(struct dentry *dentry, struct > nameidata *nd) > +{ > + if (!capable(CAP_SYS_ADMIN)) > + return ERR_PTR(-EPERM); > + > + return proc_pid_follow_link(dentry, nd); > +} > + > +/* > + * Identical to proc_pid_link_inode_operations except for follow_link() > + */ > +static const struct inode_operations proc_map_files_link_inode_operations = { > + .readlink = proc_pid_readlink, > + .follow_link = proc_map_files_follow_link, > + .setattr = proc_setattr, > +}; > + > static struct dentry * > proc_map_files_instantiate(struct inode *dir, struct dentry *dentry, > struct task_struct *task, const void *ptr) > @@ -2063,7 +2078,7 @@ proc_map_files_instantiate(struct inode > ei = PROC_I(inode); > ei->op.proc_get_link = proc_map_files_get_link; > > - inode->i_op = &proc_pid_link_inode_operations; > + inode->i_op = &proc_map_files_link_inode_operations; > inode->i_size = 64; > inode->i_mode = S_IFLNK; > > @@ -2087,10 +2102,6 @@ static struct dentry *proc_map_files_loo > struct dentry *result; > struct mm_struct *mm; > > - result = ERR_PTR(-EPERM); > - if (!capable(CAP_SYS_ADMIN)) > - goto out; > - > result = ERR_PTR(-ENOENT); > task = get_proc_task(dir); > if (!task) > @@ -2143,10 +2154,6 @@ proc_map_files_readdir(struct file *filp > ino_t ino; > int ret; > > - ret = -EPERM; > - if (!capable(CAP_SYS_ADMIN)) > - goto out; > - > ret = -ENOENT; > task = get_proc_task(inode); > if (!task) > @@ -2353,9 +2360,6 @@ static const struct file_operations proc > .release = seq_release_private, > }; > > - > -#endif /* CONFIG_CHECKPOINT_RESTORE */ > - > #ifdef CONFIG_VE > static long proc_aio_ioctl(struct file *file, unsigned int cmd, unsigned > long arg) > { > @@ -2911,9 +2915,7 @@ static const struct inode_operations pro > static const struct pid_entry tgid_base_stuff[] = { > DIR("task", S_IRUGO|S_IXUGO, proc_task_inode_operations, > proc_task_operations), > DIR("fd", S_IRUSR|S_IXUSR, proc_fd_inode_operations, > proc_fd_operations), > -#ifdef CONFIG_CHECKPOINT_RESTORE > DIR("map_files", S_IRUSR|S_IXUSR, proc_map_files_inode_operations, > proc_map_files_operations), > -#endif > DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, > proc_fdinfo_operations), > DIR("ns", S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, > proc_ns_dir_operations), > #ifdef CONFIG_NET _______________________________________________ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel