| Message ID | 20210308170651.919148-1-kaleshsingh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v6,1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ | expand |
Am 08.03.21 um 18:06 schrieb Kalesh Singh: > Android captures per-process system memory state when certain low memory > events (e.g a foreground app kill) occur, to identify potential memory > hoggers. In order to measure how much memory a process actually consumes, > it is necessary to include the DMA buffer sizes for that process in the > memory accounting. Since the handle to DMA buffers are raw FDs, it is > important to be able to identify which processes have FD references to > a DMA buffer. > > Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and > /proc/<pid>/fdinfo -- both are only readable by the process owner, > as follows: > 1. Do a readlink on each FD. > 2. If the target path begins with "/dmabuf", then the FD is a dmabuf FD. > 3. stat the file to get the dmabuf inode number. > 4. Read/ proc/<pid>/fdinfo/<fd>, to get the DMA buffer size. > > Accessing other processes' fdinfo requires root privileges. This limits > the use of the interface to debugging environments and is not suitable > for production builds. Granting root privileges even to a system process > increases the attack surface and is highly undesirable. > > Since fdinfo doesn't permit reading process memory and manipulating > process state, allow accessing fdinfo under PTRACE_MODE_READ_FSCRED. > > Suggested-by: Jann Horn <jannh@google.com> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com> Both patches are Acked-by: Christian König <christian.koenig@amd.com> > --- > Hi everyone, > > The initial posting of this patch can be found at [1]. > I didn't receive any feedback last time, so resending here. > Would really appreciate any constructive comments/suggestions. > > Thanks, > Kalesh > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20210208155315.1367371-1-kaleshsingh%40google.com%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C38c98420f0564e15117f08d8e2549ff5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637508200431130855%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=deJBlAk6%2BEQkfAC8iRK95xhV1%2FiO9Si%2Bylc5Z0QzzrM%3D&reserved=0 > > Changes in v2: > - Update patch description > fs/proc/base.c | 4 ++-- > fs/proc/fd.c | 15 ++++++++++++++- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 3851bfcdba56..fd46d8dd0cf4 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -3159,7 +3159,7 @@ 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), > DIR("map_files", S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations), > - DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations), > + DIR("fdinfo", S_IRUGO|S_IXUGO, 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 > DIR("net", S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations), > @@ -3504,7 +3504,7 @@ static const struct inode_operations proc_tid_comm_inode_operations = { > */ > static const struct pid_entry tid_base_stuff[] = { > DIR("fd", S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations), > - DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations), > + DIR("fdinfo", S_IRUGO|S_IXUGO, 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 > DIR("net", S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations), > diff --git a/fs/proc/fd.c b/fs/proc/fd.c > index 07fc4fad2602..6a80b40fd2fe 100644 > --- a/fs/proc/fd.c > +++ b/fs/proc/fd.c > @@ -6,6 +6,7 @@ > #include <linux/fdtable.h> > #include <linux/namei.h> > #include <linux/pid.h> > +#include <linux/ptrace.h> > #include <linux/security.h> > #include <linux/file.h> > #include <linux/seq_file.h> > @@ -72,6 +73,18 @@ static int seq_show(struct seq_file *m, void *v) > > static int seq_fdinfo_open(struct inode *inode, struct file *file) > { > + bool allowed = false; > + struct task_struct *task = get_proc_task(inode); > + > + if (!task) > + return -ESRCH; > + > + allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS); > + put_task_struct(task); > + > + if (!allowed) > + return -EACCES; > + > return single_open(file, seq_show, inode); > } > > @@ -308,7 +321,7 @@ static struct dentry *proc_fdinfo_instantiate(struct dentry *dentry, > struct proc_inode *ei; > struct inode *inode; > > - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR); > + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUGO); > if (!inode) > return ERR_PTR(-ENOENT); >
On Mon, Mar 8, 2021 at 12:54 PM Christian König <christian.koenig@amd.com> wrote: > > Am 08.03.21 um 18:06 schrieb Kalesh Singh: > > Android captures per-process system memory state when certain low memory > > events (e.g a foreground app kill) occur, to identify potential memory > > hoggers. In order to measure how much memory a process actually consumes, > > it is necessary to include the DMA buffer sizes for that process in the > > memory accounting. Since the handle to DMA buffers are raw FDs, it is > > important to be able to identify which processes have FD references to > > a DMA buffer. > > > > Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and > > /proc/<pid>/fdinfo -- both are only readable by the process owner, > > as follows: > > 1. Do a readlink on each FD. > > 2. If the target path begins with "/dmabuf", then the FD is a dmabuf FD. > > 3. stat the file to get the dmabuf inode number. > > 4. Read/ proc/<pid>/fdinfo/<fd>, to get the DMA buffer size. > > > > Accessing other processes' fdinfo requires root privileges. This limits > > the use of the interface to debugging environments and is not suitable > > for production builds. Granting root privileges even to a system process > > increases the attack surface and is highly undesirable. > > > > Since fdinfo doesn't permit reading process memory and manipulating > > process state, allow accessing fdinfo under PTRACE_MODE_READ_FSCRED. > > > > Suggested-by: Jann Horn <jannh@google.com> > > Signed-off-by: Kalesh Singh <kaleshsingh@google.com> > > Both patches are Acked-by: Christian König <christian.koenig@amd.com> Thanks Christian. > > > --- > > Hi everyone, > > > > The initial posting of this patch can be found at [1]. > > I didn't receive any feedback last time, so resending here. > > Would really appreciate any constructive comments/suggestions. > > > > Thanks, > > Kalesh > > > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20210208155315.1367371-1-kaleshsingh%40google.com%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C38c98420f0564e15117f08d8e2549ff5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637508200431130855%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=deJBlAk6%2BEQkfAC8iRK95xhV1%2FiO9Si%2Bylc5Z0QzzrM%3D&reserved=0 > > > > Changes in v2: > > - Update patch description > > fs/proc/base.c | 4 ++-- > > fs/proc/fd.c | 15 ++++++++++++++- > > 2 files changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 3851bfcdba56..fd46d8dd0cf4 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -3159,7 +3159,7 @@ 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), > > DIR("map_files", S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations), > > - DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations), > > + DIR("fdinfo", S_IRUGO|S_IXUGO, 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 > > DIR("net", S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations), > > @@ -3504,7 +3504,7 @@ static const struct inode_operations proc_tid_comm_inode_operations = { > > */ > > static const struct pid_entry tid_base_stuff[] = { > > DIR("fd", S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations), > > - DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations), > > + DIR("fdinfo", S_IRUGO|S_IXUGO, 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 > > DIR("net", S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations), > > diff --git a/fs/proc/fd.c b/fs/proc/fd.c > > index 07fc4fad2602..6a80b40fd2fe 100644 > > --- a/fs/proc/fd.c > > +++ b/fs/proc/fd.c > > @@ -6,6 +6,7 @@ > > #include <linux/fdtable.h> > > #include <linux/namei.h> > > #include <linux/pid.h> > > +#include <linux/ptrace.h> > > #include <linux/security.h> > > #include <linux/file.h> > > #include <linux/seq_file.h> > > @@ -72,6 +73,18 @@ static int seq_show(struct seq_file *m, void *v) > > > > static int seq_fdinfo_open(struct inode *inode, struct file *file) > > { > > + bool allowed = false; > > + struct task_struct *task = get_proc_task(inode); > > + > > + if (!task) > > + return -ESRCH; > > + > > + allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS); > > + put_task_struct(task); > > + > > + if (!allowed) > > + return -EACCES; > > + > > return single_open(file, seq_show, inode); > > } > > > > @@ -308,7 +321,7 @@ static struct dentry *proc_fdinfo_instantiate(struct dentry *dentry, > > struct proc_inode *ei; > > struct inode *inode; > > > > - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR); > > + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUGO); > > if (!inode) > > return ERR_PTR(-ENOENT); > > > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Mon, Mar 08, 2021 at 05:06:40PM +0000, Kalesh Singh wrote: > Android captures per-process system memory state when certain low memory > events (e.g a foreground app kill) occur, to identify potential memory > hoggers. In order to measure how much memory a process actually consumes, > it is necessary to include the DMA buffer sizes for that process in the > memory accounting. Since the handle to DMA buffers are raw FDs, it is > important to be able to identify which processes have FD references to > a DMA buffer. > > Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and > /proc/<pid>/fdinfo -- both are only readable by the process owner, > as follows: > 1. Do a readlink on each FD. > 2. If the target path begins with "/dmabuf", then the FD is a dmabuf FD. > 3. stat the file to get the dmabuf inode number. > 4. Read/ proc/<pid>/fdinfo/<fd>, to get the DMA buffer size. > > Accessing other processes' fdinfo requires root privileges. This limits > the use of the interface to debugging environments and is not suitable > for production builds. Granting root privileges even to a system process > increases the attack surface and is highly undesirable. > > Since fdinfo doesn't permit reading process memory and manipulating > process state, allow accessing fdinfo under PTRACE_MODE_READ_FSCRED. > > Suggested-by: Jann Horn <jannh@google.com> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Who would be best to pick this up? Maybe akpm?
On Wed, Mar 17, 2021 at 10:55 PM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Mar 08, 2021 at 05:06:40PM +0000, Kalesh Singh wrote: > > Android captures per-process system memory state when certain low memory > > events (e.g a foreground app kill) occur, to identify potential memory > > hoggers. In order to measure how much memory a process actually consumes, > > it is necessary to include the DMA buffer sizes for that process in the > > memory accounting. Since the handle to DMA buffers are raw FDs, it is > > important to be able to identify which processes have FD references to > > a DMA buffer. > > > > Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and > > /proc/<pid>/fdinfo -- both are only readable by the process owner, > > as follows: > > 1. Do a readlink on each FD. > > 2. If the target path begins with "/dmabuf", then the FD is a dmabuf FD. > > 3. stat the file to get the dmabuf inode number. > > 4. Read/ proc/<pid>/fdinfo/<fd>, to get the DMA buffer size. > > > > Accessing other processes' fdinfo requires root privileges. This limits > > the use of the interface to debugging environments and is not suitable > > for production builds. Granting root privileges even to a system process > > increases the attack surface and is highly undesirable. > > > > Since fdinfo doesn't permit reading process memory and manipulating > > process state, allow accessing fdinfo under PTRACE_MODE_READ_FSCRED. > > > > Suggested-by: Jann Horn <jannh@google.com> > > Signed-off-by: Kalesh Singh <kaleshsingh@google.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Who would be best to pick this up? Maybe akpm? Thanks Kees. Andrew has queued the patchset for the mm tree. > > -- > Kees Cook
diff --git a/fs/proc/base.c b/fs/proc/base.c index 3851bfcdba56..fd46d8dd0cf4 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3159,7 +3159,7 @@ 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), DIR("map_files", S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations), - DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations), + DIR("fdinfo", S_IRUGO|S_IXUGO, 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 DIR("net", S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations), @@ -3504,7 +3504,7 @@ static const struct inode_operations proc_tid_comm_inode_operations = { */ static const struct pid_entry tid_base_stuff[] = { DIR("fd", S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations), - DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations), + DIR("fdinfo", S_IRUGO|S_IXUGO, 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 DIR("net", S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations), diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 07fc4fad2602..6a80b40fd2fe 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -6,6 +6,7 @@ #include <linux/fdtable.h> #include <linux/namei.h> #include <linux/pid.h> +#include <linux/ptrace.h> #include <linux/security.h> #include <linux/file.h> #include <linux/seq_file.h> @@ -72,6 +73,18 @@ static int seq_show(struct seq_file *m, void *v) static int seq_fdinfo_open(struct inode *inode, struct file *file) { + bool allowed = false; + struct task_struct *task = get_proc_task(inode); + + if (!task) + return -ESRCH; + + allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS); + put_task_struct(task); + + if (!allowed) + return -EACCES; + return single_open(file, seq_show, inode); } @@ -308,7 +321,7 @@ static struct dentry *proc_fdinfo_instantiate(struct dentry *dentry, struct proc_inode *ei; struct inode *inode; - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUGO); if (!inode) return ERR_PTR(-ENOENT);
Android captures per-process system memory state when certain low memory events (e.g a foreground app kill) occur, to identify potential memory hoggers. In order to measure how much memory a process actually consumes, it is necessary to include the DMA buffer sizes for that process in the memory accounting. Since the handle to DMA buffers are raw FDs, it is important to be able to identify which processes have FD references to a DMA buffer. Currently, DMA buffer FDs can be accounted using /proc/<pid>/fd/* and /proc/<pid>/fdinfo -- both are only readable by the process owner, as follows: 1. Do a readlink on each FD. 2. If the target path begins with "/dmabuf", then the FD is a dmabuf FD. 3. stat the file to get the dmabuf inode number. 4. Read/ proc/<pid>/fdinfo/<fd>, to get the DMA buffer size. Accessing other processes' fdinfo requires root privileges. This limits the use of the interface to debugging environments and is not suitable for production builds. Granting root privileges even to a system process increases the attack surface and is highly undesirable. Since fdinfo doesn't permit reading process memory and manipulating process state, allow accessing fdinfo under PTRACE_MODE_READ_FSCRED. Suggested-by: Jann Horn <jannh@google.com> Signed-off-by: Kalesh Singh <kaleshsingh@google.com> --- Hi everyone, The initial posting of this patch can be found at [1]. I didn't receive any feedback last time, so resending here. Would really appreciate any constructive comments/suggestions. Thanks, Kalesh [1] https://lore.kernel.org/r/20210208155315.1367371-1-kaleshsingh@google.com/ Changes in v2: - Update patch description fs/proc/base.c | 4 ++-- fs/proc/fd.c | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-)