[RESEND PATCH v6 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[RESEND PATCH v6 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[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>
---
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(-)

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);
 
-- 
2.30.1.766.gb4fecdf3b7-goog

[RESEND PATCH v6 2/2] procfs/dmabuf: Add inode number to /proc/*/fdinfo

[Kalesh Singh]

And 'ino' field to /proc/<pid>/fdinfo/<FD> and
/proc/<pid>/task/<tid>/fdinfo/<FD>.

The inode numbers can be used to uniquely identify DMA buffers
in user space and avoids a dependency on /proc/<pid>/fd/* when
accounting per-process DMA buffer sizes.

Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
---
Hi everyone,

This a resend of the inital posting at [1].
There was no objections on the last threads, along with some positive
feedback from Randy. If there is no other concern I would like to
have this considered for mainline.

Thanks,
Kalesh

[1] https://lore.kernel.org/linux-doc/20210208155315.1367371-2-kaleshsingh@google.com/

Changes in v5:
  - Fixed tab vs spaces, per Randy
  - Renamed inode_no to ino, per Matthew
Changes in v4:
  - Add inode number as common field in fdinfo, per Christian
Changes in v3:
  - Add documentation in proc.rst, per Randy
Changes in v2:
  - Update patch description

---
 Documentation/filesystems/proc.rst | 37 +++++++++++++++++++++++++-----
 fs/proc/fd.c                       |  5 ++--
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 48fbfc336ebf..33d08fbb0022 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -1909,18 +1909,20 @@ if precise results are needed.
 3.8	/proc/<pid>/fdinfo/<fd> - Information about opened file
 ---------------------------------------------------------------
 This file provides information associated with an opened file. The regular
-files have at least three fields -- 'pos', 'flags' and 'mnt_id'. The 'pos'
-represents the current offset of the opened file in decimal form [see lseek(2)
-for details], 'flags' denotes the octal O_xxx mask the file has been
-created with [see open(2) for details] and 'mnt_id' represents mount ID of
-the file system containing the opened file [see 3.5 /proc/<pid>/mountinfo
-for details].
+files have at least four fields -- 'pos', 'flags', 'mnt_id' and 'ino'.
+The 'pos' represents the current offset of the opened file in decimal
+form [see lseek(2) for details], 'flags' denotes the octal O_xxx mask the
+file has been created with [see open(2) for details] and 'mnt_id' represents
+mount ID of the file system containing the opened file [see 3.5
+/proc/<pid>/mountinfo for details]. 'ino' represents the inode number of
+the file.
 
 A typical output is::
 
 	pos:	0
 	flags:	0100002
 	mnt_id:	19
+	ino:	63107
 
 All locks associated with a file descriptor are shown in its fdinfo too::
 
@@ -1937,6 +1939,7 @@ Eventfd files
 	pos:	0
 	flags:	04002
 	mnt_id:	9
+	ino:	63107
 	eventfd-count:	5a
 
 where 'eventfd-count' is hex value of a counter.
@@ -1949,6 +1952,7 @@ Signalfd files
 	pos:	0
 	flags:	04002
 	mnt_id:	9
+	ino:	63107
 	sigmask:	0000000000000200
 
 where 'sigmask' is hex value of the signal mask associated
@@ -1962,6 +1966,7 @@ Epoll files
 	pos:	0
 	flags:	02
 	mnt_id:	9
+	ino:	63107
 	tfd:        5 events:       1d data: ffffffffffffffff pos:0 ino:61af sdev:7
 
 where 'tfd' is a target file descriptor number in decimal form,
@@ -1978,6 +1983,8 @@ For inotify files the format is the following::
 
 	pos:	0
 	flags:	02000000
+	mnt_id:	9
+	ino:	63107
 	inotify wd:3 ino:9e7e sdev:800013 mask:800afce ignored_mask:0 fhandle-bytes:8 fhandle-type:1 f_handle:7e9e0000640d1b6d
 
 where 'wd' is a watch descriptor in decimal form, i.e. a target file
@@ -2000,6 +2007,7 @@ For fanotify files the format is::
 	pos:	0
 	flags:	02
 	mnt_id:	9
+	ino:	63107
 	fanotify flags:10 event-flags:0
 	fanotify mnt_id:12 mflags:40 mask:38 ignored_mask:40000003
 	fanotify ino:4f969 sdev:800013 mflags:0 mask:3b ignored_mask:40000000 fhandle-bytes:8 fhandle-type:1 f_handle:69f90400c275b5b4
@@ -2024,6 +2032,7 @@ Timerfd files
 	pos:	0
 	flags:	02
 	mnt_id:	9
+	ino:	63107
 	clockid: 0
 	ticks: 0
 	settime flags: 01
@@ -2038,6 +2047,22 @@ details]. 'it_value' is remaining time until the timer expiration.
 with TIMER_ABSTIME option which will be shown in 'settime flags', but 'it_value'
 still exhibits timer's remaining time.
 
+DMA Buffer files
+~~~~~~~~~~~~~~~~
+
+::
+
+	pos:	0
+	flags:	04002
+	mnt_id:	9
+	ino:	63107
+	size:   32768
+	count:  2
+	exp_name:  system-heap
+
+where 'size' is the size of the DMA buffer in bytes. 'count' is the file count of
+the DMA buffer file. 'exp_name' is the name of the DMA buffer exporter.
+
 3.9	/proc/<pid>/map_files - Information about memory mapped files
 ---------------------------------------------------------------------
 This directory contains symbolic links which represent memory mapped files
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 6a80b40fd2fe..172c86270b31 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -54,9 +54,10 @@ static int seq_show(struct seq_file *m, void *v)
 	if (ret)
 		return ret;
 
-	seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\n",
+	seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\nino:\t%lu\n",
 		   (long long)file->f_pos, f_flags,
-		   real_mount(file->f_path.mnt)->mnt_id);
+		   real_mount(file->f_path.mnt)->mnt_id,
+		   file_inode(file)->i_ino);
 
 	/* show_fd_locks() never deferences files so a stale value is safe */
 	show_fd_locks(m, file, files);
-- 
2.30.1.766.gb4fecdf3b7-goog

Re: [RESEND PATCH v6 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[Christian König]

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&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C38c98420f0564e15117f08d8e2549ff5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637508200431130855%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=deJBlAk6%2BEQkfAC8iRK95xhV1%2FiO9Si%2Bylc5Z0QzzrM%3D&amp;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);
>   

Re: [RESEND PATCH v6 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[Kalesh Singh]

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&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C38c98420f0564e15117f08d8e2549ff5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637508200431130855%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=deJBlAk6%2BEQkfAC8iRK95xhV1%2FiO9Si%2Bylc5Z0QzzrM%3D&amp;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.
>

Re: [RESEND PATCH v6 2/2] procfs/dmabuf: Add inode number to /proc/*/fdinfo

[Kees Cook]

On Mon, Mar 08, 2021 at 05:06:41PM +0000, Kalesh Singh wrote:
> And 'ino' field to /proc/<pid>/fdinfo/<FD> and
> /proc/<pid>/task/<tid>/fdinfo/<FD>.
> 
> The inode numbers can be used to uniquely identify DMA buffers
> in user space and avoids a dependency on /proc/<pid>/fd/* when
> accounting per-process DMA buffer sizes.
> 
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [RESEND PATCH v6 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[Kees Cook]

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?

-- 
Kees Cook

Re: [RESEND PATCH v6 1/2] procfs: Allow reading fdinfo with PTRACE_MODE_READ

[Kalesh Singh]

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