| Message ID | 20220908093919.843346-1-vincent.whitchurch@axis.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | proc: Enable smaps_rollup without ptrace rights | expand |
On Thu, 8 Sep 2022 11:39:19 +0200 Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > smaps_rollup is currently only allowed on processes which the user has > ptrace permissions for, since it uses a common proc open function used > by other files like mem and smaps. > > However, while smaps provides detailed, individual information about > each memory map in the process (justifying its ptrace rights > requirement), smaps_rollup only provides a summary of the memory usage, > which is not unlike the information available from other places like the > status and statm files, which do not need ptrace permissions. > > The first line of smaps_rollup could however be sensitive, since it > exposes the randomized start and end of the process' address space. > This information however does not seem essential to smap_rollup's > purpose and could be replaced with placeholder values to preserve the > format without leaking information. (I could not find any user space in > Debian or Android which uses the information in the first line.) > > Replace the start with 0 and end with ~0 and allow smaps_rollup to be > opened and read regardless of ptrace permissions. What is the motivation for this? Use case? End-user value and such?
On Thu, Sep 08, 2022 at 11:59:34PM +0200, Andrew Morton wrote: > On Thu, 8 Sep 2022 11:39:19 +0200 Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > > smaps_rollup is currently only allowed on processes which the user has > > ptrace permissions for, since it uses a common proc open function used > > by other files like mem and smaps. > > > > However, while smaps provides detailed, individual information about > > each memory map in the process (justifying its ptrace rights > > requirement), smaps_rollup only provides a summary of the memory usage, > > which is not unlike the information available from other places like the > > status and statm files, which do not need ptrace permissions. > > > > The first line of smaps_rollup could however be sensitive, since it > > exposes the randomized start and end of the process' address space. > > This information however does not seem essential to smap_rollup's > > purpose and could be replaced with placeholder values to preserve the > > format without leaking information. (I could not find any user space in > > Debian or Android which uses the information in the first line.) > > > > Replace the start with 0 and end with ~0 and allow smaps_rollup to be > > opened and read regardless of ptrace permissions. > > What is the motivation for this? Use case? End-user value and such? My use case is similar to Sergey's[0]: to be able to gather memory usage information from a daemon/script running without root permissions or ptrace rights. Values like Pss are only available from smaps_rollup, and not from the other files like status and statm which already provide memory usage information without requiring elevated privileges. [0] https://lore.kernel.org/lkml/20200929024018.GA529@jagdpanzerIV.localdomain/ smaps_rollup is however much more expensive than those other files, so I guess that could be an argument for treating it differently, even if the content itself does not need to be protected.
diff --git a/fs/proc/base.c b/fs/proc/base.c index 93f7e3d971e4..9482eb3954de 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -792,14 +792,16 @@ static const struct file_operations proc_single_file_operations = { .release = single_release, }; - -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) +static struct mm_struct *__proc_mem_open(struct inode *inode, unsigned int mode, bool creds) { struct task_struct *task = get_proc_task(inode); struct mm_struct *mm = ERR_PTR(-ESRCH); if (task) { - mm = mm_access(task, mode | PTRACE_MODE_FSCREDS); + if (creds) + mm = mm_access(task, mode | PTRACE_MODE_FSCREDS); + else + mm = get_task_mm(task); put_task_struct(task); if (!IS_ERR_OR_NULL(mm)) { @@ -813,6 +815,16 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) return mm; } +struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) +{ + return __proc_mem_open(inode, mode, true); +} + +struct mm_struct *proc_mem_open_nocreds(struct inode *inode) +{ + return __proc_mem_open(inode, 0, false); +} + static int __mem_open(struct inode *inode, struct file *file, unsigned int mode) { struct mm_struct *mm = proc_mem_open(inode, mode); diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 06a80f78433d..5c906661b018 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -293,6 +293,7 @@ struct proc_maps_private { } __randomize_layout; struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode); +struct mm_struct *proc_mem_open_nocreds(struct inode *inode); extern const struct file_operations proc_pid_maps_operations; extern const struct file_operations proc_pid_numa_maps_operations; diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 4e0023643f8b..13f910b51dce 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -969,8 +969,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v) vma = vma->vm_next; } - show_vma_header_prefix(m, priv->mm->mmap->vm_start, - last_vma_end, 0, 0, 0, 0); + show_vma_header_prefix(m, 0, ~0lu, 0, 0, 0, 0); seq_pad(m, ' '); seq_puts(m, "[rollup]\n"); @@ -1015,7 +1014,7 @@ static int smaps_rollup_open(struct inode *inode, struct file *file) goto out_free; priv->inode = inode; - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); + priv->mm = proc_mem_open_nocreds(inode); if (IS_ERR(priv->mm)) { ret = PTR_ERR(priv->mm);
smaps_rollup is currently only allowed on processes which the user has ptrace permissions for, since it uses a common proc open function used by other files like mem and smaps. However, while smaps provides detailed, individual information about each memory map in the process (justifying its ptrace rights requirement), smaps_rollup only provides a summary of the memory usage, which is not unlike the information available from other places like the status and statm files, which do not need ptrace permissions. The first line of smaps_rollup could however be sensitive, since it exposes the randomized start and end of the process' address space. This information however does not seem essential to smap_rollup's purpose and could be replaced with placeholder values to preserve the format without leaking information. (I could not find any user space in Debian or Android which uses the information in the first line.) Replace the start with 0 and end with ~0 and allow smaps_rollup to be opened and read regardless of ptrace permissions. Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- fs/proc/base.c | 18 +++++++++++++++--- fs/proc/internal.h | 1 + fs/proc/task_mmu.c | 5 ++--- 3 files changed, 18 insertions(+), 6 deletions(-)