[PATCH v2] sysctl: control functionality of /proc/pid/mem

[PATCH v2] sysctl: control functionality of /proc/pid/mem

[Kees Cook]

Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
allowed to work: 0: disabled, 1: read only, 2: read/write.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/sysctl/kernel.txt |   14 ++++++++++++++
 fs/proc/base.c                  |   14 +++++++++++++-
 kernel/sysctl.c                 |   14 ++++++++++++++
 3 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 8c20fbd..6d52dba 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -56,6 +56,7 @@ show up in /proc/sys/kernel:
 - printk_delay
 - printk_ratelimit
 - printk_ratelimit_burst
+- proc_pid_mem
 - randomize_va_space
 - real-root-dev               ==> Documentation/initrd.txt
 - reboot-cmd                  [ SPARC only ]
@@ -477,6 +478,19 @@ send before ratelimiting kicks in.
 
 ==============================================================
 
+proc_pid_mem:
+
+This option can be used to select the level of access given to potential
+ptracers when using the per-process "mem" file in /proc/pid/mem.
+
+0 - Disable entirely.
+
+1 - Allow potential ptracers read access to process memory, but not writes.
+
+2 - Allow potential ptracers read and write access to process memory.
+
+==============================================================
+
 randomize_va_space:
 
 This option can be used to select the type of process address
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9cde9ed..53133c7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -109,6 +109,8 @@ struct pid_entry {
 	union proc_op op;
 };
 
+int sysctl_proc_pid_mem = 2;
+
 #define NOD(NAME, MODE, IOP, FOP, OP) {			\
 	.name = (NAME),					\
 	.len  = sizeof(NAME) - 1,			\
@@ -699,9 +701,13 @@ static const struct file_operations proc_single_file_operations = {
 
 static int mem_open(struct inode* inode, struct file* file)
 {
-	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
+	struct task_struct *task;
 	struct mm_struct *mm;
 
+	if (sysctl_proc_pid_mem < 1)
+		return -EACCES;
+
+	task = get_proc_task(file->f_path.dentry->d_inode);
 	if (!task)
 		return -ESRCH;
 
@@ -726,6 +732,9 @@ static ssize_t mem_read(struct file * file, char __user * buf,
 	unsigned long src = *ppos;
 	struct mm_struct *mm = file->private_data;
 
+	if (sysctl_proc_pid_mem < 1)
+		return -EACCES;
+
 	if (!mm)
 		return 0;
 
@@ -770,6 +779,9 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
 	unsigned long dst = *ppos;
 	struct mm_struct *mm = file->private_data;
 
+	if (sysctl_proc_pid_mem < 2)
+		return -EACCES;
+
 	if (!mm)
 		return 0;
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f487f25..dda911f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -109,6 +109,9 @@ extern int sysctl_nr_trim_pages;
 #ifdef CONFIG_BLOCK
 extern int blk_iopoll_enabled;
 #endif
+#ifdef CONFIG_PROC_FS
+extern int sysctl_proc_pid_mem;
+#endif
 
 /* Constants used for minimum and  maximum */
 #ifdef CONFIG_LOCKUP_DETECTOR
@@ -1004,6 +1007,17 @@ static struct ctl_table kern_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 #endif
+#ifdef CONFIG_PROC_FS
+	{
+		.procname	= "proc_pid_mem",
+		.data		= &sysctl_proc_pid_mem,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &two,
+	},
+#endif
 	{ }
 };
 
-- 
1.7.0.4

Re: [PATCH v2] sysctl: control functionality of /proc/pid/mem

[Randy Dunlap]

On 01/21/2012 01:06 AM, Kees Cook wrote:
> Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
> allowed to work: 0: disabled, 1: read only, 2: read/write.

Maybe mention that the default is 2 (or did you mean to change that sooner
or later?).


> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/sysctl/kernel.txt |   14 ++++++++++++++
>  fs/proc/base.c                  |   14 +++++++++++++-
>  kernel/sysctl.c                 |   14 ++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletions(-)

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH v2] sysctl: control functionality of /proc/pid/mem

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> Add the "proc_pid_mem" sysctl to control whether or not /proc/pid/mem is
> allowed to work: 0: disabled, 1: read only, 2: read/write.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/sysctl/kernel.txt |   14 ++++++++++++++
>  fs/proc/base.c                  |   14 +++++++++++++-
>  kernel/sysctl.c                 |   14 ++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 8c20fbd..6d52dba 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -56,6 +56,7 @@ show up in /proc/sys/kernel:
>  - printk_delay
>  - printk_ratelimit
>  - printk_ratelimit_burst
> +- proc_pid_mem
>  - randomize_va_space
>  - real-root-dev               ==> Documentation/initrd.txt
>  - reboot-cmd                  [ SPARC only ]
> @@ -477,6 +478,19 @@ send before ratelimiting kicks in.
>  
>  ==============================================================
>  
> +proc_pid_mem:
> +
> +This option can be used to select the level of access given to potential
> +ptracers when using the per-process "mem" file in /proc/pid/mem.
> +
> +0 - Disable entirely.
> +
> +1 - Allow potential ptracers read access to process memory, but not writes.
> +
> +2 - Allow potential ptracers read and write access to process memory.
> +
> +==============================================================
> +
>  randomize_va_space:
>  
>  This option can be used to select the type of process address
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 9cde9ed..53133c7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -109,6 +109,8 @@ struct pid_entry {
>  	union proc_op op;
>  };
>  
> +int sysctl_proc_pid_mem = 2;
> +
>  #define NOD(NAME, MODE, IOP, FOP, OP) {			\
>  	.name = (NAME),					\
>  	.len  = sizeof(NAME) - 1,			\
> @@ -699,9 +701,13 @@ static const struct file_operations proc_single_file_operations = {
>  
>  static int mem_open(struct inode* inode, struct file* file)
>  {
> -	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
> +	struct task_struct *task;
>  	struct mm_struct *mm;
>  
> +	if (sysctl_proc_pid_mem < 1)
> +		return -EACCES;
> +
> +	task = get_proc_task(file->f_path.dentry->d_inode);
>  	if (!task)
>  		return -ESRCH;
>  
> @@ -726,6 +732,9 @@ static ssize_t mem_read(struct file * file, char __user * buf,
>  	unsigned long src = *ppos;
>  	struct mm_struct *mm = file->private_data;
>  
> +	if (sysctl_proc_pid_mem < 1)
> +		return -EACCES;
> +
>  	if (!mm)
>  		return 0;
>  
> @@ -770,6 +779,9 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
>  	unsigned long dst = *ppos;
>  	struct mm_struct *mm = file->private_data;
>  
> +	if (sysctl_proc_pid_mem < 2)
> +		return -EACCES;
> +
>  	if (!mm)
>  		return 0;
>  
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f487f25..dda911f 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -109,6 +109,9 @@ extern int sysctl_nr_trim_pages;
>  #ifdef CONFIG_BLOCK
>  extern int blk_iopoll_enabled;
>  #endif
> +#ifdef CONFIG_PROC_FS
> +extern int sysctl_proc_pid_mem;
> +#endif
>  
>  /* Constants used for minimum and  maximum */
>  #ifdef CONFIG_LOCKUP_DETECTOR
> @@ -1004,6 +1007,17 @@ static struct ctl_table kern_table[] = {
>  		.proc_handler	= proc_dointvec,
>  	},
>  #endif
> +#ifdef CONFIG_PROC_FS
    ^^^^^^^^^^^^^^^^^^^^^^

That ifdef is entertaining.  CONFIG_SYSCTL depends on CONFIG_PROC_FS
so which interesting case did you imagine this ifdef would be false?
Did you test to ensure the code is not compiled in that interesting case?

> +	{
> +		.procname	= "proc_pid_mem",
> +		.data		= &sysctl_proc_pid_mem,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &two,
> +	},
> +#endif
>  	{ }
>  };

Re: [PATCH v2] sysctl: control functionality of /proc/pid/mem

[Kees Cook]

On Mon, Jan 23, 2012 at 6:41 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>> +#ifdef CONFIG_PROC_FS
>    ^^^^^^^^^^^^^^^^^^^^^^
>
> That ifdef is entertaining.  CONFIG_SYSCTL depends on CONFIG_PROC_FS
> so which interesting case did you imagine this ifdef would be false?
> Did you test to ensure the code is not compiled in that interesting case?

I noticed that when I added it, and decided to err on the side of more
configs if some day /proc/sys got moved out of /proc. Anyway, I can
easily remove it. v3 coming up...

-Kees

-- 
Kees Cook
ChromeOS Security