procfs: Add /proc/<pid>/mapped_files

Commit b76437579d1344b6 ("procfs: mark thread stack correctly in proc/<pid>/maps") introduced logic to mark thread stacks with the "[stack:%d]" marker in /proc/<pid>/maps.

This causes reading /proc/<pid>/maps to take O(N^2) time, where N is the number of threads sharing an address space, since each line of output requires iterating over the VMA list looking for ranges that correspond to the stack pointer in any task's register set. When dealing with highly-threaded Java applications, reading this file can take hours and trigger softlockup dumps.

Eliminating the "[stack:%d]" marker is not a viable option since it's been there for some time, and I don't see a way to do the stack check more efficiently that wouldn't end up making the whole thing really ugly.

Just thinking out loud: Could one simply mark a VMA as being used for stack during the clone call (is there room in vm_flags, or does VM_GROWSDOWN already tell the whole story?), and then write the TID into a new field in the VMA - I think one could make a union with vm_pgoff so as not to enlarge the structure.

Quoted 5 lines · Rasmus Villemoes

Just thinking out loud: Could one simply mark a VMA as being used for stack during the clone call (is there room in vm_flags, or does VM_GROWSDOWN already tell the whole story?), and then write the TID into a new field in the VMA - I think one could make a union with vm_pgoff so as not to enlarge the structure.

vm_flags does not have space IIRC (that was my first approach at implementing this) and VM_GROWSDOWN is not sufficient.

If we can make a union with vm_pgoff like you say, we probably don't need a flag value; a non-zero value could indicate that it is a thread stack.

One problem with caching the value on clone like this though is that the stack could change due to a setcontext, but AFAICT we don't care about that for the process stack either.

Quoted 9 lines · Siddhesh Poyarekar

Quoted 5 lines · Rasmus Villemoes

Just thinking out loud: Could one simply mark a VMA as being used for stack during the clone call (is there room in vm_flags, or does VM_GROWSDOWN already tell the whole story?), and then write the TID into a new field in the VMA - I think one could make a union with vm_pgoff so as not to enlarge the structure.

vm_flags does not have space IIRC (that was my first approach at implementing this) and VM_GROWSDOWN is not sufficient.

Looking at include/linux/mm.h:

#define VM_GROWSDOWN    0x00000100      /* general info on the segment */
#define VM_PFNMAP       0x00000400      /* Page-ranges managed without "struct page", just pure PFN */
#define VM_DENYWRITE    0x00000800      /* ETXTBSY on write attempts.. */

It would seem that 0x00000200 is available (unless defined and used somewhere else).

Quoted 3 lines

If we can make a union with vm_pgoff like you say, we probably don't need a flag value; a non-zero value could indicate that it is a thread stack.

Well, only when combined with checking vm_file for being NULL. One would also need to ensure that vm_pgoff is 0 for any non-stack, non-file-backed VMA. At which point it is somewhat ugly.

Quoted 3 lines

One problem with caching the value on clone like this though is that the stack could change due to a setcontext, but AFAICT we don't care about that for the process stack either.

If it is important, I guess one could update the info when a task calls setcontext.

Rasmus

Quoted 10 lines

Well, only when combined with checking vm_file for being NULL. One would also need to ensure that vm_pgoff is 0 for any non-stack, non-file-backed VMA. At which point it is somewhat ugly.

Quoted 3 lines

One problem with caching the value on clone like this though is that the stack could change due to a setcontext, but AFAICT we don't care about that for the process stack either.

If it is important, I guess one could update the info when a task calls setcontext.

If I understand the current behavior, the "[stack]" marker will get put next to *any* mapping that encompasses the current value in the task's %sp, regardless of how the mapping was created or ucontext stuff. If you use flags on the VMA structs things could potentially be marked as stacks even though %sp points somewhere else.

It's probable that nobody cares (you'd obviously have to be doing crazy things to be pointing %sp at arbitrary places), but that's why I was hesitant to mess with it.

Quoted 10 lines · On Wed, 14 Jan 2015 13:03:26 -0800 Calvin Owens <[email protected]> wrote:

If I understand the current behavior, the "[stack]" marker will get put next to *any* mapping that encompasses the current value in the task's %sp, regardless of how the mapping was created or ucontext stuff. If you use flags on the VMA structs things could potentially be marked as stacks even though %sp points somewhere else.

It's probable that nobody cares (you'd obviously have to be doing crazy things to be pointing %sp at arbitrary places), but that's why I was hesitant to mess with it.

Fixing the N^2 search would of course be much better than adding a new proc file to sidestep it.

Could we do something like refreshing the new vma.vm_flags:VM_IS_STACK on each thread at the time when /proc/PID/maps is opened? So do a walk of the threads, use each thread's sp to hunt down the thread's stack's vma, then set VM_IS_STACK and fill in the new vma.stack_tid field?

There are still several flags unused in vma.vm_flags btw.

I'm not sure that we can repurpose vm_pgoff (or vm_private_data) for
this: a badly behaved thread could make its sp point at a random vma then trick the kernel into scribbling on that vma's vm_proff?

Adding a new field to the vma wouldn't kill us, I guess. That would remove the need for a VM_IS_STACK.

Quoted 5 lines

There are still several flags unused in vma.vm_flags btw.

I'm not sure that we can repurpose vm_pgoff (or vm_private_data) for
this: a badly behaved thread could make its sp point at a random vma then trick the kernel into scribbling on that vma's vm_proff?

Well, we could still check vm_file for being NULL before writing to vm_pgoff/vm_stack_tid.

Quoted 6 lines · On Thu, 15 Jan 2015 00:51:50 +0100 Rasmus Villemoes <[email protected]>…

Quoted 5 lines

There are still several flags unused in vma.vm_flags btw.

I'm not sure that we can repurpose vm_pgoff (or vm_private_data) for
this: a badly behaved thread could make its sp point at a random vma then trick the kernel into scribbling on that vma's vm_proff?

Quoted 2 lines

Well, we could still check vm_file for being NULL before writing to vm_pgoff/vm_stack_tid.

Yes, I guess that would work. We'd need to check that nobody else is already playing similar games with vm_pgoff.

Commit b76437579d1344b6 ("procfs: mark thread stack correctly in proc/<pid>/maps") introduced logic to mark thread stacks with the "[stack:%d]" marker in /proc/<pid>/maps.

This causes reading /proc/<pid>/maps to take O(N^2) time, where N is the number of threads sharing an address space, since each line of output requires iterating over the VMA list looking for ranges that correspond to the stack pointer in any task's register set. When dealing with highly-threaded Java applications, reading this file can take hours and trigger softlockup dumps.

Eliminating the "[stack:%d]" marker is not a viable option since it's been there for some time, and I don't see a way to do the stack check more efficiently that wouldn't end up making the whole thing really ugly.

The use case I'm specifically concerned with is the lsof command, so this patch adds an additional file, "mapped_files", that simply iterates over the VMAs associated with the task and outputs a newline-delimited list of the pathnames of the files associated with the VMAs, if any.

This gives lsof and suchlike a way to determine the pathnames of files mapped into a process without incurring the O(N^2) behavior of the maps file.

Quoted 4 lines

This gives lsof and suchlike a way to determine the pathnames of files mapped into a process without incurring the O(N^2) behavior of the maps file.

We already have /proc/PID/map_files/ directory which lists all mapped files. Should we consider relaxing permission checking there and move it outside CONFIG_CHECKPOINT_RESTORE instead?

Restriction to CAP_SYSADMIN for follow_link is undertansble, but why do we restrict readdir and readlink?

On Wed, Jan 14, 2015 at 05:25:01PM +0200, Kirill A. Shutemov wrote:
...

Quoted 9 lines

Quoted 4 lines

This gives lsof and suchlike a way to determine the pathnames of files mapped into a process without incurring the O(N^2) behavior of the maps file.

We already have /proc/PID/map_files/ directory which lists all mapped files. Should we consider relaxing permission checking there and move it outside CONFIG_CHECKPOINT_RESTORE instead?

Quoted 2 lines

Restriction to CAP_SYSADMIN for follow_link is undertansble, but why do we restrict readdir and readlink?

We didn't think this functionality might be needed someone but us (criu camp), so that the rule of thumb was CONFIG_CHECKPOINT_RESTORE + CAP_SYSADMIN, until otherwise strictly needed. So I think now we can relax security rules a bit and allow to readdir and such for owners.

Quoted 8 lines

Quoted 3 lines

Restriction to CAP_SYSADMIN for follow_link is undertansble, but why do we restrict readdir and readlink?

We didn't think this functionality might be needed someone but us (criu camp), so that the rule of thumb was CONFIG_CHECKPOINT_RESTORE + CAP_SYSADMIN, until otherwise strictly needed. So I think now we can relax security rules a bit and allow to readdir and such for owners.

Ah, I feel silly for missing that. I'll send a patch to move map_files out from behind CONFIG_CHECKPOINT_RESTORE and change the permissions.

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. To avoid exposing files to processes for whom they may not be visible, a follow_link() stub is added to the inode_operations struct attached to the symlinks that prevent them from being followed without CAP_SYS_ADMIN.

Signed-off-by Calvin Owens <[email protected]>

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. To avoid exposing files to processes for whom they may not be visible, a follow_link() stub is added to the inode_operations struct attached to the symlinks that prevent them from being followed without CAP_SYS_ADMIN.

Signed-off-by Calvin Owens <[email protected]>

---
 fs/proc/base.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

fs/proc/base.c +2 -7

index 3f3d7ae..7d48003 100644
@@ -1632,8 +1632,6 @@ end_instantiate:
 	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.

@@ -1660,11 +1658,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 	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)

@@ -1753,6 +1746,28 @@ struct map_files_info {
 	unsigned char	name[4*sizeof(long)+2]; /* max: %lx-%lx\0 */
 };
 
+/*
+ * Allowing any user to follow the symlinks in /proc/<pid>/map_files/ could

Quoted 47 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. To avoid exposing files to processes for whom they may not be visible, a follow_link() stub is added to the inode_operations struct attached to the symlinks that prevent them from being followed without CAP_SYS_ADMIN.

Signed-off-by Calvin Owens <[email protected]>

---
 fs/proc/base.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

fs/proc/base.c +2 -7

index 3f3d7ae..7d48003 100644
@@ -1632,8 +1632,6 @@ end_instantiate:
 	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.

@@ -1660,11 +1658,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 	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)

@@ -1753,6 +1746,28 @@ struct map_files_info {
 	unsigned char	name[4*sizeof(long)+2]; /* max: %lx-%lx\0 */
 };
 
+/*
+ * Allowing any user to follow the symlinks in /proc/<pid>/map_files/ could

fs/proc/base.c struct map_files_info { · line 1760

Quoted 10 lines

fs/proc/base.c +10 -0

+ * allow processes to access files that should not be visible to them, so
+ * restrict follow_link() to CAP_SYS_ADMIN for these files.
+ */
+static void *proc_map_files_follow_link(struct dentry *d, struct nameidata *n)
+{
+	if (!capable(CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
+	return proc_pid_follow_link(d, n);
+}

I have thought a bit more about this and not sure it's reasonable to limit it to CAP_SYS_ADMIN. What scenario are we protecting from?

Initially, I thought about something like this: privileged process opens a file, map part of it, closes the file and drop privileges with hope to limit further access to mapped window of the file. But I don't see what would stop the unprivileged process from accessing rest of the file using mremap(2). And if a process can do this, anybody who can ptrace(2) the process can do this.

Am I missing something?

Signature · 1 line

 Kirill A. Shutemov

Quoted 69 lines · Kirill A. Shutemov

Quoted 47 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. To avoid exposing files to processes for whom they may not be visible, a follow_link() stub is added to the inode_operations struct attached to the symlinks that prevent them from being followed without CAP_SYS_ADMIN.

Signed-off-by Calvin Owens <[email protected]>

---
 fs/proc/base.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

fs/proc/base.c +2 -7

index 3f3d7ae..7d48003 100644
@@ -1632,8 +1632,6 @@ end_instantiate:
 	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.

@@ -1660,11 +1658,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 	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)

@@ -1753,6 +1746,28 @@ struct map_files_info {
 	unsigned char	name[4*sizeof(long)+2]; /* max: %lx-%lx\0 */
 };
 
+/*
+ * Allowing any user to follow the symlinks in /proc/<pid>/map_files/ could

fs/proc/base.c struct map_files_info { · line 1760

Quoted 10 lines

fs/proc/base.c +10 -0

+ * allow processes to access files that should not be visible to them, so
+ * restrict follow_link() to CAP_SYS_ADMIN for these files.
+ */
+static void *proc_map_files_follow_link(struct dentry *d, struct nameidata *n)
+{
+	if (!capable(CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
+	return proc_pid_follow_link(d, n);
+}

I have thought a bit more about this and not sure it's reasonable to limit it to CAP_SYS_ADMIN. What scenario are we protecting from?

Initially, I thought about something like this: privileged process opens a file, map part of it, closes the file and drop privileges with hope to limit further access to mapped window of the file. But I don't see what would stop the unprivileged process from accessing rest of the file using mremap(2). And if a process can do this, anybody who can ptrace(2) the process can do this.

Quoted 1 line

Am I missing something?

The specific case I was thinking of is a process in a chroot with a mounted /proc inside of it: if a process inside the chroot has the same UID as a process outside of it, the chroot'ed process could follow the symlinks in map_files/ and poke files it can't actually see, right?

Quoted 72 lines · Calvin Owens

Quoted 70 lines · Kirill A. Shutemov

Quoted 47 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. To avoid exposing files to processes for whom they may not be visible, a follow_link() stub is added to the inode_operations struct attached to the symlinks that prevent them from being followed without CAP_SYS_ADMIN.

Signed-off-by Calvin Owens <[email protected]>

---
 fs/proc/base.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

fs/proc/base.c +2 -7

index 3f3d7ae..7d48003 100644
@@ -1632,8 +1632,6 @@ end_instantiate:
 	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.

@@ -1660,11 +1658,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 	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)

@@ -1753,6 +1746,28 @@ struct map_files_info {
 	unsigned char	name[4*sizeof(long)+2]; /* max: %lx-%lx\0 */
 };
 
+/*
+ * Allowing any user to follow the symlinks in /proc/<pid>/map_files/ could

fs/proc/base.c struct map_files_info { · line 1760

Quoted 10 lines

fs/proc/base.c +10 -0

+ * allow processes to access files that should not be visible to them, so
+ * restrict follow_link() to CAP_SYS_ADMIN for these files.
+ */
+static void *proc_map_files_follow_link(struct dentry *d, struct nameidata *n)
+{
+	if (!capable(CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
+	return proc_pid_follow_link(d, n);
+}

I have thought a bit more about this and not sure it's reasonable to limit it to CAP_SYS_ADMIN. What scenario are we protecting from?

Initially, I thought about something like this: privileged process opens a file, map part of it, closes the file and drop privileges with hope to limit further access to mapped window of the file. But I don't see what would stop the unprivileged process from accessing rest of the file using mremap(2). And if a process can do this, anybody who can ptrace(2) the process can do this.

Am I missing something?

Quoted 4 lines

The specific case I was thinking of is a process in a chroot with a mounted /proc inside of it: if a process inside the chroot has the same UID as a process outside of it, the chroot'ed process could follow the symlinks in map_files/ and poke files it can't actually see, right?

It depends on how you define "poke". If you mean touch content of the file, then, well, you can do it now. You cannot do anything which requires file descriptor -- open(), ftrancate(), etc.

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

---
Changes in v2: Removed the follow_link() stub that returned -EPERM if
the caller didn't have CAP_SYS_ADMIN, since the caller in my chroot() scenario gets -EACCES anyway.

 fs/proc/base.c | 18 ------------------
 1 file changed, 18 deletions(-)

fs/proc/base.c +0 -18

index 3f3d7ae..67b15ac 100644
@@ -1632,8 +1632,6 @@ end_instantiate:
 	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.

@@ -1660,11 +1658,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 	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)

@@ -1792,10 +1785,6 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 	int result;
 	struct mm_struct *mm;
 
-	result = -EPERM;
-	if (!capable(CAP_SYS_ADMIN))
-		goto out;
-
 	result = -ENOENT;
 	task = get_proc_task(dir);
 	if (!task)

@@ -1849,10 +1838,6 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
 	struct map_files_info *p;
 	int ret;
 
-	ret = -EPERM;
-	if (!capable(CAP_SYS_ADMIN))
-		goto out;
-
 	ret = -ENOENT;
 	task = get_proc_task(file_inode(file));
 	if (!task)

@@ -2040,7 +2025,6 @@ static const struct file_operations proc_timers_operations = {
 	.llseek		= seq_lseek,
 	.release	= seq_release_private,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
 
 static int proc_pident_instantiate(struct inode *dir,
 	struct dentry *dentry, struct task_struct *task, const void *ptr)

@@ -2537,9 +2521,7 @@ static const struct inode_operations proc_task_inode_operations;
 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

Signature · 7 lines

1.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 5 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Quoted 5 lines · Kirill A. Shutemov

Quoted 4 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

Quoted 6 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov…

Quoted 5 lines · Kirill A. Shutemov

Quoted 4 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation

akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

Quoted 2 lines

akpm3:/usr/src/25> grep -r map_files Documentation

If akpm's comments weren't clear: this needs to be fixed. Everything in /proc should appear in Documentation.

Quoted 14 lines

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

How is mmap offset represented in this output?

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Both maps and map_files uses ptrace_may_access (via mm_acces) with PTRACE_MODE_READ, so I'm happy from a info leak perspective.

Are mount namespaces handled in this output?

Quoted 5 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

Quoted 5 lines · Kees Cook

Quoted 2 lines

akpm3:/usr/src/25> grep -r map_files Documentation

If akpm's comments weren't clear: this needs to be fixed. Everything in /proc should appear in Documentation.

I'll do that.

Quoted 15 lines

Quoted 14 lines

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

Quoted 1 line

How is mmap offset represented in this output?

We're printing vm_area_struct:[vm_start;vm_end] only.

Quoted 5 lines

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Quoted 4 lines

Both maps and map_files uses ptrace_may_access (via mm_acces) with PTRACE_MODE_READ, so I'm happy from a info leak perspective.

Are mount namespaces handled in this output?

Could you clarify this moment, i'm not sure i get it.

Quoted 6 lines

Quoted 6 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

Quoted 4 lines

Quoted 2 lines

Are mount namespaces handled in this output?

Could you clarify this moment, i'm not sure i get it.

I changed how I asked this question in my review of the documentation, but it looks like these symlinks aren't "regular" symlinks (that are up to the follower to have access to the file system path shown), but rather they bypass VFS. As a result, I'm wondering how things like mount namespaces might change this behavior: what is shown, the path from the perspective of the target, or from the viewer (which may be in separate mount namespaces).

Quoted 4 lines

Quoted 2 lines

Are mount namespaces handled in this output?

Could you clarify this moment, i'm not sure i get it.

I changed how I asked this question in my review of the documentation, but it looks like these symlinks aren't "regular" symlinks (that are up to the follower to have access to the file system path shown), but rather they bypass VFS. As a result, I'm wondering how things like mount namespaces might change this behavior: what is shown, the path from the perspective of the target, or from the viewer (which may be in separate mount namespaces).

Quoted 6 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov…

Quoted 5 lines · Kirill A. Shutemov

Quoted 4 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:     
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:     
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case?

Quoted 3 lines

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:     
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:     
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

I didn't find any bugs in map-files (and we use it for long time already) so I think it is safe.

Quoted 5 lines · On Tue, 27 Jan 2015 09:46:47 +0300 Cyrill Gorcunov…

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

Quoted 2 lines

I didn't find any bugs in map-files (and we use it for long time already) so I think it is safe.

You've been using map_files the way it was supposed to be used so no, any bugs won't show up. What happens if you don your evil black hat and use map_files in ways that weren't anticipated? Attack it?

Quoted 2 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <[email protected]> wrote:

Quoted 1 line · Kirill A. Shutemov

On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:     
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:     
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov…

Quoted 1 line · Kirill A. Shutemov

On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

The biggest difference is that if you do something like this:

        fd = open("/stuff", O_BLAH);
        map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0);
        close(fd);
        unlink("/stuff");

...then map_files/ gives you a way to get a file descriptor for "/stuff", which you couldn't do with /proc/pid/maps.

It's also something of a win if you just want to see what is mapped at a specific address, since you can just readlink() the symlink for the address range you care about and it will go grab the appropriate VMA and give you the answer. /proc/pid/maps requires walking the VMA tree, which is quite expensive for processes with many thousands of threads, even without the O(N^2) issue.

(You have to know what address range you want though, since readdir() on map_files/ obviously has to walk the VMA tree just like /proc/N/maps.)

Quoted 37 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Yes, the only difference is that you can follow the link as per above. I'll resend with a new message explaining that and the deletion thing.

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

While I was messing with it I used it a good bit and didn't see any issues, although I didn't actively try to fuzz it or anything. I'd be happy to write something to test hammering it in weird ways if you like. I'm also happy to write testcases for namespaces.

So far as security issues, as others have pointed out you can't follow the links unless you can ptrace the process in question, which seems like a pretty solid guarantee. As Cyrill pointed out in the discussion about the documentation, that's the same protection as /proc/N/fd/*, and those links function in the same way.

Quoted 89 lines · Calvin Owens

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov…

Quoted 1 line · Kirill A. Shutemov

On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

The biggest difference is that if you do something like this:

        fd = open("/stuff", O_BLAH);
        map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0);
        close(fd);
        unlink("/stuff");

...then map_files/ gives you a way to get a file descriptor for "/stuff", which you couldn't do with /proc/pid/maps.

It's also something of a win if you just want to see what is mapped at a specific address, since you can just readlink() the symlink for the address range you care about and it will go grab the appropriate VMA and give you the answer. /proc/pid/maps requires walking the VMA tree, which is quite expensive for processes with many thousands of threads, even without the O(N^2) issue.

(You have to know what address range you want though, since readdir() on map_files/ obviously has to walk the VMA tree just like /proc/N/maps.)

Quoted 37 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Yes, the only difference is that you can follow the link as per above. I'll resend with a new message explaining that and the deletion thing.

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

While I was messing with it I used it a good bit and didn't see any issues, although I didn't actively try to fuzz it or anything. I'd be happy to write something to test hammering it in weird ways if you like. I'm also happy to write testcases for namespaces.

Quoted 5 lines

So far as security issues, as others have pointed out you can't follow the links unless you can ptrace the process in question, which seems like a pretty solid guarantee. As Cyrill pointed out in the discussion about the documentation, that's the same protection as /proc/N/fd/*, and those links function in the same way.

My concern here is that fd/* are connected as streams, and while that has a certain level of badness as an external-to-the-process attacker, PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is required for access to /proc/N/mem). Since these fds are the things mapped into memory on a process, writing to them is a subset of access to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient.

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Quoted 96 lines · Kees Cook

Quoted 89 lines · Calvin Owens

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <[email protected]> wrote:

Quoted 1 line · Kirill A. Shutemov

On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

The biggest difference is that if you do something like this:

         fd = open("/stuff", O_BLAH);
         map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0);
         close(fd);
         unlink("/stuff");

...then map_files/ gives you a way to get a file descriptor for "/stuff", which you couldn't do with /proc/pid/maps.

It's also something of a win if you just want to see what is mapped at a specific address, since you can just readlink() the symlink for the address range you care about and it will go grab the appropriate VMA and give you the answer. /proc/pid/maps requires walking the VMA tree, which is quite expensive for processes with many thousands of threads, even without the O(N^2) issue.

(You have to know what address range you want though, since readdir() on map_files/ obviously has to walk the VMA tree just like /proc/N/maps.)

Quoted 37 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Yes, the only difference is that you can follow the link as per above. I'll resend with a new message explaining that and the deletion thing.

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

While I was messing with it I used it a good bit and didn't see any issues, although I didn't actively try to fuzz it or anything. I'd be happy to write something to test hammering it in weird ways if you like. I'm also happy to write testcases for namespaces.

Quoted 5 lines

So far as security issues, as others have pointed out you can't follow the links unless you can ptrace the process in question, which seems like a pretty solid guarantee. As Cyrill pointed out in the discussion about the documentation, that's the same protection as /proc/N/fd/*, and those links function in the same way.

Quoted 6 lines

My concern here is that fd/* are connected as streams, and while that has a certain level of badness as an external-to-the-process attacker, PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is required for access to /proc/N/mem). Since these fds are the things mapped into memory on a process, writing to them is a subset of access to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient.

If you haven't done close() on a mmapped file, doesn't fd/* allow the same access to the corresponding regions of memory? Or am I missing something?

But that said, I can't think of any reason making it MODE_ATTACH would be a problem. Would you rather that be enforced on follow_link() like the original patch did, or enforce it for the whole directory?

Quoted 108 lines · Calvin Owens

Quoted 96 lines · Kees Cook

Quoted 89 lines · Calvin Owens

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov…

Quoted 1 line · Kirill A. Shutemov

On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

The biggest difference is that if you do something like this:

        fd = open("/stuff", O_BLAH);
        map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0);
        close(fd);
        unlink("/stuff");

...then map_files/ gives you a way to get a file descriptor for "/stuff", which you couldn't do with /proc/pid/maps.

It's also something of a win if you just want to see what is mapped at a specific address, since you can just readlink() the symlink for the address range you care about and it will go grab the appropriate VMA and give you the answer. /proc/pid/maps requires walking the VMA tree, which is quite expensive for processes with many thousands of threads, even without the O(N^2) issue.

(You have to know what address range you want though, since readdir() on map_files/ obviously has to walk the VMA tree just like /proc/N/maps.)

Quoted 37 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Yes, the only difference is that you can follow the link as per above. I'll resend with a new message explaining that and the deletion thing.

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

While I was messing with it I used it a good bit and didn't see any issues, although I didn't actively try to fuzz it or anything. I'd be happy to write something to test hammering it in weird ways if you like. I'm also happy to write testcases for namespaces.

Quoted 5 lines

So far as security issues, as others have pointed out you can't follow the links unless you can ptrace the process in question, which seems like a pretty solid guarantee. As Cyrill pointed out in the discussion about the documentation, that's the same protection as /proc/N/fd/*, and those links function in the same way.

Quoted 6 lines

My concern here is that fd/* are connected as streams, and while that has a certain level of badness as an external-to-the-process attacker, PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is required for access to /proc/N/mem). Since these fds are the things mapped into memory on a process, writing to them is a subset of access to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient.

If you haven't done close() on a mmapped file, doesn't fd/* allow the same access to the corresponding regions of memory? Or am I missing something?

Quoted 5 lines

But that said, I can't think of any reason making it MODE_ATTACH would be a problem. Would you rather that be enforced on follow_link() like the original patch did, or enforce it for the whole directory?

Whole directory would probably be better, as even just the mapped ranges could be considered sensitive information.

Ideally, the check should be done on both follow_link(), and the directory itself.

Quoted 96 lines · Kees Cook

Quoted 89 lines · Calvin Owens

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <[email protected]> wrote:

Quoted 1 line · Kirill A. Shutemov

On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

The biggest difference is that if you do something like this:

        fd = open("/stuff", O_BLAH);
        map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0);
        close(fd);
        unlink("/stuff");

...then map_files/ gives you a way to get a file descriptor for "/stuff", which you couldn't do with /proc/pid/maps.

It's also something of a win if you just want to see what is mapped at a specific address, since you can just readlink() the symlink for the address range you care about and it will go grab the appropriate VMA and give you the answer. /proc/pid/maps requires walking the VMA tree, which is quite expensive for processes with many thousands of threads, even without the O(N^2) issue.

(You have to know what address range you want though, since readdir() on map_files/ obviously has to walk the VMA tree just like /proc/N/maps.)

Quoted 37 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Yes, the only difference is that you can follow the link as per above. I'll resend with a new message explaining that and the deletion thing.

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

While I was messing with it I used it a good bit and didn't see any issues, although I didn't actively try to fuzz it or anything. I'd be happy to write something to test hammering it in weird ways if you like. I'm also happy to write testcases for namespaces.

Quoted 5 lines

So far as security issues, as others have pointed out you can't follow the links unless you can ptrace the process in question, which seems like a pretty solid guarantee. As Cyrill pointed out in the discussion about the documentation, that's the same protection as /proc/N/fd/*, and those links function in the same way.

Quoted 6 lines

My concern here is that fd/* are connected as streams, and while that has a certain level of badness as an external-to-the-process attacker, PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is required for access to /proc/N/mem). Since these fds are the things mapped into memory on a process, writing to them is a subset of access to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient.

If you haven't done close() on a mmapped file, doesn't fd/* allow the same access to the corresponding regions of memory? Or am I missing something?

Quoted 104 lines · Calvin Owens

Quoted 96 lines · Kees Cook

Quoted 89 lines · Calvin Owens

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov…

Quoted 1 line · Kirill A. Shutemov

On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed.

This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps?

The biggest difference is that if you do something like this:

        fd = open("/stuff", O_BLAH);
        map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0);
        close(fd);
        unlink("/stuff");

...then map_files/ gives you a way to get a file descriptor for "/stuff", which you couldn't do with /proc/pid/maps.

It's also something of a win if you just want to see what is mapped at a specific address, since you can just readlink() the symlink for the address range you care about and it will go grab the appropriate VMA and give you the answer. /proc/pid/maps requires walking the VMA tree, which is quite expensive for processes with many thousands of threads, even without the O(N^2) issue.

(You have to know what address range you want though, since readdir() on map_files/ obviously has to walk the VMA tree just like /proc/N/maps.)

Quoted 37 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

This patch moves the folder out from behind CHECKPOINT_RESTORE, and removes the CAP_SYS_ADMIN restrictions. Following the links requires the ability to ptrace the process in question, so this doesn't allow an attacker to do anything they couldn't already do before.

Signed-off-by Calvin Owens <[email protected]>

Cc +linux-api@

Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well.

hm, who's that. Kees comes to mind.

And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This:

akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25>

does not help.

The 640708a2cff7f81 changelog says:

:     This one behaves similarly to the /proc/<pid>/fd/ one - it contains
:     symlinks one for each mapping with file, the name of a symlink is
:     "vma->vm_start-vma->vm_end", the target is the file.  Opening a symlink
:     results in a file that point exactly to the same inode as them vma's one.
:
:     For example the ls -l of some arbitrary /proc/<pid>/map_files/
:
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so
:      | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so

Quoted 4 lines

afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog).

Yes, the only difference is that you can follow the link as per above. I'll resend with a new message explaining that and the deletion thing.

Quoted 4 lines

There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free?

While I was messing with it I used it a good bit and didn't see any issues, although I didn't actively try to fuzz it or anything. I'd be happy to write something to test hammering it in weird ways if you like. I'm also happy to write testcases for namespaces.

Quoted 5 lines

So far as security issues, as others have pointed out you can't follow the links unless you can ptrace the process in question, which seems like a pretty solid guarantee. As Cyrill pointed out in the discussion about the documentation, that's the same protection as /proc/N/fd/*, and those links function in the same way.

Quoted 6 lines

My concern here is that fd/* are connected as streams, and while that has a certain level of badness as an external-to-the-process attacker, PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is required for access to /proc/N/mem). Since these fds are the things mapped into memory on a process, writing to them is a subset of access to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient.

Quoted 4 lines

If you haven't done close() on a mmapped file, doesn't fd/* allow the same access to the corresponding regions of memory? Or am I missing something?

But if you have called close(), then you can't currently do things like ftruncate or ioctl on the mapped file. These things don't persist across execve(), but the do persist across calls to setresuid, etc that drop privileges. The latter part makes me a tiny bit nervous.

It also might be worth checking for drivers or arch code that creates vmas that are backed by a different struct file than the struct file that was mmapped in the first place.

--Andy

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed. It also allows access to file descriptors for files that have been deleted and closed but are still mmapped into a process, which can be very useful for introspection and debugging.

fs/proc/base.c +7 -0

+/*
+ * Enforce stronger PTRACE_MODE_ATTACH permissions on the symlinks under
+ * /proc/<pid>/map_files, since these links may refer to deleted or O_TMPFILE
+ * files that users might assume are inaccessible regardless of their
+ * ownership/permissions.
+ */
+static void *proc_map_files_follow_link(struct dentry *dentry, struct nameidata *nd)

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed. It also allows access to file descriptors for files that have been deleted and closed but are still mmapped into a process, which can be very useful for introspection and debugging.

Quoted 7 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed. It also allows access to file descriptors for files that have been deleted and closed but are still mmapped into a process, which can be very useful for introspection and debugging.

Guys, I'm really-really sorry for not replying the email that long. If I understand correctly all concerns were addressed, right?

Quoted 3 lines

Guys, I'm really-really sorry for not replying the email that long. If I understand correctly all concerns were addressed, right?

Ping!

I thought everybody was happy after the permission check was changed to be PTRACE_MODE_ATTACH. But I'll resend one more time if you prefer.

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed. It also allows access to file descriptors for files that have been deleted and closed but are still mmapped into a process, which can be very useful for introspection and debugging.

fs/proc/base.c +15 -0

+/*
+ * Enforce stronger PTRACE_MODE_ATTACH permissions on the symlinks under
+ * /proc/<pid>/map_files, since these links may refer to deleted or O_TMPFILE
+ * files that users might assume are inaccessible regardless of their
+ * ownership/permissions.
+ */
+static void *proc_map_files_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+	struct inode *inode = d_inode(dentry);
+	struct task_struct *task;
+	int allowed = 0;
+
+	task = get_proc_task(inode);
+	if (task) {
+		allowed = ptrace_may_access(task, PTRACE_MODE_ATTACH);

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed. It also allows access to file descriptors for files that have been deleted and closed but are still mmapped into a process, which can be very useful for introspection and debugging.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and

removes the CAP_SYS_ADMIN restrictions. With that change alone, following the links would have required PTRACE_MODE_READ like the links in /proc/<pid>/fd/*.

Quoted 9 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed. It also allows access to file descriptors for files that have been deleted and closed but are still mmapped into a process, which can be very useful for introspection and debugging.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and

I'm fine with this.

Quoted 3 lines

removes the CAP_SYS_ADMIN restrictions. With that change alone, following the links would have required PTRACE_MODE_READ like the links in /proc/<pid>/fd/*.

I'm still not at all convinced that this is safe. Here are a few ways that it could have unintended consequences:

1. Mmap a dma-buf and then open /proc/self/map_files/addr. You get an fd pointing at a different inode than you mapped. (kdbus would have the same problem if it were merged.)

2. Open a file with O_RDONLY, mmap it with PROT_READ, close the file, then open /proc/self/map_files/addr with O_RDWR. I don't see anything preventing that from succeeding.

3. Open a file, mmap it, close the fd, chroot, drop privileges, open /proc/self/map_files/addr, then call ftruncate.

So NAK as-is, I think.

Fixing #1 would involve changing the way mmap works, I think. Fixing #2 would require similar infrastructure to what we'd need to fix the existing /proc/pid/fd mode holes. I have no clue how to even approach fixing #3.

What's the use case of this patch?

--Andy

Quoted 24 lines · Andy Lutomirski

Quoted 9 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is very useful for enumerating the files mapped into a process when the more verbose information in /proc/<pid>/maps is not needed. It also allows access to file descriptors for files that have been deleted and closed but are still mmapped into a process, which can be very useful for introspection and debugging.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and

I'm fine with this.

Quoted 3 lines

removes the CAP_SYS_ADMIN restrictions. With that change alone, following the links would have required PTRACE_MODE_READ like the links in /proc/<pid>/fd/*.

I'm still not at all convinced that this is safe. Here are a few ways that it could have unintended consequences:

1. Mmap a dma-buf and then open /proc/self/map_files/addr. You get an fd pointing at a different inode than you mapped. (kdbus would have the same problem if it were merged.)

Quoted 3 lines

2. Open a file with O_RDONLY, mmap it with PROT_READ, close the file, then open /proc/self/map_files/addr with O_RDWR. I don't see anything preventing that from succeeding.

Hmm, that's a good point: it lets you bypass the permission checks on all the path components you would normally walk through to get to the file. But it still only works if you actually have permission to open the file in question for writing.

Also, this is already how the /proc/N/fd/* symlinks work, isn't it?

Quoted 3 lines

3. Open a file, mmap it, close the fd, chroot, drop privileges, open /proc/self/map_files/addr, then call ftruncate.

This doesn't work unless the privileges you dropped to actually allow you to open the mmapped file for writing. It's really the same fundamental problem as (2), where you're allowing direct access to a file without trying to walk the path down to it, right?

Quoted 2 lines

So NAK as-is, I think.

Limiting ->follow_link() to CAP_SYS_ADMIN wouldn't affect anything I imagine using this interface for (see below), so I have no problem with putting that back in. I think that would alleviate all your concerns above, right?

(That said, I don't think it makes sense to limit readdir() or readlink() on map_files/* to CAP_SYS_ADMIN, since that alone is a subset of what you can get from /proc/N/maps.)

Quoted 7 lines

Fixing #1 would involve changing the way mmap works, I think. Fixing #2 would require similar infrastructure to what we'd need to fix the existing /proc/pid/fd mode holes. I have no clue how to even approach fixing #3.

What's the use case of this patch?

The biggest use case: it enables you to stat() files that have been deleted but are still mapped by some process.

This enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

It also allows you to know how much space a specific mapped-but-deleted file is using on a specific filesystem, which is currently impossible from userspace AFAIK.

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

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.

In earlier versions of this patch, I changed the ptrace checks in the functions above to enforce MODE_ATTACH instead of MODE_READ. That was an oversight: all the information exposed by the above three functions is already available with MODE_READ from /proc/PID/maps. I was only being asked to strengthen the protection around functionality provided by follow_link(), not the above.

So, I've left the checks for MODE_READ as-is, since AFAICS all objections raised so far are addressed by the new CAP_SYS_ADMIN check in follow_link(), explained below.

* 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).

Cc Andrew Morton <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Joe Perches <[email protected]>

Cc Kees Cook <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Calvin Owens <[email protected]>

---
Changes in v6: Require CAP_SYS_ADMIN for follow_link(). Leave other
PTRACE_MODE_READ checks as-is, since CAP_SYS_ADMIN alone addresses all concerns raised AFAICS.

Changes in v5: s/dentry->d_inode/d_inode(dentry)/g

Changes in v4: Return -ESRCH from follow_link() when get_proc_task()
returns NULL.

Changes in v3: Changed permission checks to use PTRACE_MODE_ATTACH
instead of PTRACE_MODE_READ, and added a stub to enforce MODE_ATTACH on follow_link() as well.

Changes in v2: Removed the follow_link() stub that returned -EPERM if
the caller didn't have CAP_SYS_ADMIN, since the caller in my chroot() scenario gets -EACCES anyway.

 fs/proc/base.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

fs/proc/base.c +0 -7

index 093ca14..0270191 100644
@@ -1641,8 +1641,6 @@ end_instantiate:
        return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.

@@ -1669,11 +1667,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
        if (flags & LOOKUP_RCU)
                return -ECHILD;
 
-       if (!capable(CAP_SYS_ADMIN)) {
-               status = -EPERM;
-               goto out_notask;
-       }
-
        inode = d_inode(dentry);
        task = get_proc_task(inode);
        if (!task)

fs/proc/base.c +18 -13

+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 int
 proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
                           struct task_struct *task, const void *ptr)

@@ -1777,7 +1792,7 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
        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;
 

@@ -1801,10 +1816,6 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
        int result;
        struct mm_struct *mm;
 
-       result = -EPERM;
-       if (!capable(CAP_SYS_ADMIN))
-               goto out;
-
        result = -ENOENT;
        task = get_proc_task(dir);
        if (!task)

@@ -1858,10 +1869,6 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
        struct map_files_info *p;
        int ret;
 
-       ret = -EPERM;
-       if (!capable(CAP_SYS_ADMIN))
-               goto out;
-
        ret = -ENOENT;
        task = get_proc_task(file_inode(file));
        if (!task)

@@ -2050,7 +2057,6 @@ static const struct file_operations proc_timers_operations = {
        .llseek         = seq_lseek,
        .release        = seq_release_private,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
 
 static int proc_pident_instantiate(struct inode *dir,
        struct dentry *dentry, struct task_struct *task, const void *ptr)

@@ -2549,9 +2555,7 @@ static const struct inode_operations proc_task_inode_operations;
 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
--

1.8.1

Quoted 94 lines · Calvin Owens

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

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.

In earlier versions of this patch, I changed the ptrace checks in the functions above to enforce MODE_ATTACH instead of MODE_READ. That was an oversight: all the information exposed by the above three functions is already available with MODE_READ from /proc/PID/maps. I was only being asked to strengthen the protection around functionality provided by follow_link(), not the above.

So, I've left the checks for MODE_READ as-is, since AFAICS all objections raised so far are addressed by the new CAP_SYS_ADMIN check in follow_link(), explained below.

* 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).

Cc Andrew Morton <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Joe Perches <[email protected]>

Cc Kees Cook <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Signed-off-by Calvin Owens <[email protected]>

---
Changes in v6: Require CAP_SYS_ADMIN for follow_link(). Leave other
PTRACE_MODE_READ checks as-is, since CAP_SYS_ADMIN alone addresses all concerns raised AFAICS.

Changes in v5: s/dentry->d_inode/d_inode(dentry)/g

Changes in v4: Return -ESRCH from follow_link() when get_proc_task()
returns NULL.

Changes in v3: Changed permission checks to use PTRACE_MODE_ATTACH
instead of PTRACE_MODE_READ, and added a stub to enforce MODE_ATTACH on follow_link() as well.

Changes in v2: Removed the follow_link() stub that returned -EPERM if
the caller didn't have CAP_SYS_ADMIN, since the caller in my chroot() scenario gets -EACCES anyway.

 fs/proc/base.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

fs/proc/base.c +0 -7

index 093ca14..0270191 100644
@@ -1641,8 +1641,6 @@ end_instantiate:
        return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.

@@ -1669,11 +1667,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
        if (flags & LOOKUP_RCU)
                return -ECHILD;
 
-       if (!capable(CAP_SYS_ADMIN)) {
-               status = -EPERM;
-               goto out_notask;
-       }
-
        inode = d_inode(dentry);
        task = get_proc_task(inode);
        if (!task)

fs/proc/base.c struct map_files_info { · line 1762

Quoted 8 lines

fs/proc/base.c +5 -0

@@ -1762,6 +1755,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.
+ */

Cool, I think this looks good. Thanks!

Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 70 lines

fs/proc/base.c +18 -13

+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 int
 proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
                           struct task_struct *task, const void *ptr)

@@ -1777,7 +1792,7 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
        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;
 

@@ -1801,10 +1816,6 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
        int result;
        struct mm_struct *mm;
 
-       result = -EPERM;
-       if (!capable(CAP_SYS_ADMIN))
-               goto out;
-
        result = -ENOENT;
        task = get_proc_task(dir);
        if (!task)

@@ -1858,10 +1869,6 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
        struct map_files_info *p;
        int ret;
 
-       ret = -EPERM;
-       if (!capable(CAP_SYS_ADMIN))
-               goto out;
-
        ret = -ENOENT;
        task = get_proc_task(file_inode(file));
        if (!task)

@@ -2050,7 +2057,6 @@ static const struct file_operations proc_timers_operations = {
        .llseek         = seq_lseek,
        .release        = seq_release_private,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
 
 static int proc_pident_instantiate(struct inode *dir,
        struct dentry *dentry, struct task_struct *task, const void *ptr)

@@ -2549,9 +2555,7 @@ static const struct inode_operations proc_task_inode_operations;
 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
--

Quoted 2 lines

1.8.1

--
Kees Cook
Chrome OS Security

Quoted 5 lines

Cool, I think this looks good. Thanks!

Reviewed-by Kees Cook <[email protected]>

Looks good to me, too.

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

Quoted 8 lines · On Mon, 8 Jun 2015 20:39:33 -0700 Calvin Owens <[email protected]> wrote:

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

Why is that information useful?

I could perhaps think of some use for "How much disk space is being consumed by files that are deleted but still open", but to count the mmapped-then-unlinked files while excluding the opened-then-unlinked files seems damned peculiar.

IOW, this changelog failed to explain the value of the patch. Bad changelog! Please sell it to us. Preferably with real-world use cases.

Quoted 11 lines · Andrew Morton

Quoted 8 lines · On Mon, 8 Jun 2015 20:39:33 -0700 Calvin Owens <[email protected]> wrote:

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

Quoted 6 lines

Why is that information useful?

I could perhaps think of some use for "How much disk space is being consumed by files that are deleted but still open", but to count the mmapped-then-unlinked files while excluding the opened-then-unlinked files seems damned peculiar.

Let's phrase the question a bit more generically:

"How much disk space is being consumed by files that have been unlinked, but are still referenced by some process?"

There are two pieces to this problem:
1) Unlinked files that are still open (whether mapped or not)
2) Unlinked files that are not open, but are still mapped

You can track down everything in (1) using /proc/<pid>/fd/*, and you can use stat() to figure out how much space they're using.

Does that all seem sensible?

Quoted 28 lines · On Tue, 9 Jun 2015 18:39:02 -0700 Calvin Owens <[email protected]> wrote:

Quoted 11 lines · Andrew Morton

Quoted 8 lines · On Mon, 8 Jun 2015 20:39:33 -0700 Calvin Owens <[email protected]> wrote:

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

Quoted 6 lines

Why is that information useful?

I could perhaps think of some use for "How much disk space is being consumed by files that are deleted but still open", but to count the mmapped-then-unlinked files while excluding the opened-then-unlinked files seems damned peculiar.

Let's phrase the question a bit more generically:

"How much disk space is being consumed by files that have been unlinked, but are still referenced by some process?"

There are two pieces to this problem:
1) Unlinked files that are still open (whether mapped or not)
2) Unlinked files that are not open, but are still mapped

Quoted 2 lines

You can track down everything in (1) using /proc/<pid>/fd/*, and you can use stat() to figure out how much space they're using.

This doesn't work if the mapped file has been unlinked? What does the /proc/pid/map_files listing look like for these?

Quoted 1 line

Does that all seem sensible?

Spose so. Please capture all this info in the changelog.

It all seems a bit awkward though. If we want to know "how much disk space is this process using" (or similar) then I wonder what a syscall (or prctl mode?) which does this would look like.

Quoted 30 lines · Andrew Morton

Quoted 28 lines · On Tue, 9 Jun 2015 18:39:02 -0700 Calvin Owens <[email protected]> wrote:

Quoted 11 lines · Andrew Morton

Quoted 8 lines · On Mon, 8 Jun 2015 20:39:33 -0700 Calvin Owens <[email protected]> wrote:

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

Quoted 6 lines

Why is that information useful?

I could perhaps think of some use for "How much disk space is being consumed by files that are deleted but still open", but to count the mmapped-then-unlinked files while excluding the opened-then-unlinked files seems damned peculiar.

Let's phrase the question a bit more generically:

"How much disk space is being consumed by files that have been unlinked, but are still referenced by some process?"

There are two pieces to this problem:
1) Unlinked files that are still open (whether mapped or not)
2) Unlinked files that are not open, but are still mapped

Quoted 5 lines

Quoted 2 lines

You can track down everything in (1) using /proc/<pid>/fd/*, and you can use stat() to figure out how much space they're using.

This doesn't work if the mapped file has been unlinked? What does the /proc/pid/map_files listing look like for these?

It says "(deleted)" like /proc/*/exe or any other symlink.

Quoted 35 lines · On Thu, 11 Jun 2015 14:10:45 +0300 Alexey Dobriyan <[email protected]> wrote:

Quoted 33 lines · Andrew Morton

Quoted 30 lines · On Tue, 9 Jun 2015 18:39:02 -0700 Calvin Owens <[email protected]> wrote:

Quoted 11 lines · Andrew Morton

Quoted 8 lines · On Mon, 8 Jun 2015 20:39:33 -0700 Calvin Owens <[email protected]> wrote:

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat() deleted files that are still mapped by some process, which enables a much quicker and more accurate answer to the question "How much disk space is being consumed by files that are deleted but still mapped?" than is currently possible.

Quoted 6 lines

Why is that information useful?

I could perhaps think of some use for "How much disk space is being consumed by files that are deleted but still open", but to count the mmapped-then-unlinked files while excluding the opened-then-unlinked files seems damned peculiar.

Let's phrase the question a bit more generically:

"How much disk space is being consumed by files that have been unlinked, but are still referenced by some process?"

There are two pieces to this problem:
1) Unlinked files that are still open (whether mapped or not)
2) Unlinked files that are not open, but are still mapped

You can track down everything in (1) using /proc/<pid>/fd/*, and you can use stat() to figure out how much space they're using.

Quoted 4 lines

Quoted 2 lines

This doesn't work if the mapped file has been unlinked? What does the /proc/pid/map_files listing look like for these?

It says "(deleted)" like /proc/*/exe or any other symlink.

Actually the symlink directs at "/home/akpm/foo (deleted)".

And lo, if you do `stat -L' on the symlink, you get the info for the unlinked-but-still-mmapped inode. I never knew that. And I wouldn't have learned it from the documentation, which is careful to keep all this a secret.

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:

Quoted 19 lines · On Thu, 18 Jun 2015 19:32:18 -0700 Calvin Owens <[email protected]> wrote:

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.

Quoted 2 lines

This patch moves map_files/ out from behind CONFIG_CHECKPOINT_RESTORE, and adjusts the permissions enforced on it as follows:

proc_pid_follow_link() got changed while you weren't looking, causing

fs/proc/base.c: In function 'proc_map_files_follow_link':
fs/proc/base.c:1963: warning: passing argument 2 of 'proc_pid_follow_link' from incompatible pointer type fs/proc/base.c:1578: note: expected 'void **' but argument is of type 'struct nameidata *' fs/proc/base.c:1963: warning: return discards qualifiers from pointer target type fs/proc/base.c: At top level:
fs/proc/base.c:1971: warning: initialization from incompatible pointer type

I just changed it to pass NULL:

fs/proc/base.c renamed from fs/proc/base.c~procfs-always-expose-proc-pid-map_files-and-make-it-readable-fix +3 -2

@@ -1955,12 +1955,13 @@ struct map_files_info {
  * 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)
+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);
+	return proc_pid_follow_link(dentry, NULL);
 }
 
 /*

_

Commit b76437579d1344b6 ("procfs: mark thread stack correctly in proc/<pid>/maps") introduced logic to mark thread stacks with the "[stack:%d]" marker in /proc/<pid>/maps.

This causes reading /proc/<pid>/maps to take O(N^2) time, where N is the number of threads sharing an address space, since each line of output requires iterating over the VMA list looking for ranges that correspond to the stack pointer in any task's register set. When dealing with highly-threaded Java applications, reading this file can take hours and trigger softlockup dumps.

Eliminating the "[stack:%d]" marker is not a viable option since it's been there for some time, and I don't see a way to do the stack check more efficiently that wouldn't end up making the whole thing really ugly.