diff mbox series

[v10,4/9] proc: instantiate only pids that we can ptrace on 'hidepid=4' mount option

Message ID 20200327172331.418878-5-gladkov.alexey@gmail.com (mailing list archive)
State New, archived
Headers show
Series proc: modernize proc to support multiple private instances | expand

Commit Message

Alexey Gladkov March 27, 2020, 5:23 p.m. UTC 
If "hidepid=4" mount option is set then do not instantiate pids that
we can not ptrace. "hidepid=4" means that procfs should only contain
pids that the caller can ptrace.

Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
 fs/proc/base.c          | 15 +++++++++++++++
 fs/proc/root.c          | 13 ++++++++++---
 include/linux/proc_fs.h |  1 +
 3 files changed, 26 insertions(+), 3 deletions(-)

Comments

Kees Cook March 28, 2020, 8:40 p.m. UTC | #1 
On Fri, Mar 27, 2020 at 06:23:26PM +0100, Alexey Gladkov wrote:
> If "hidepid=4" mount option is set then do not instantiate pids that
> we can not ptrace. "hidepid=4" means that procfs should only contain
> pids that the caller can ptrace.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
>  fs/proc/base.c          | 15 +++++++++++++++
>  fs/proc/root.c          | 13 ++++++++++---
>  include/linux/proc_fs.h |  1 +
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 43a28907baf9..1ebe9eba48ea 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -701,6 +701,14 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
>  				 struct task_struct *task,
>  				 int hide_pid_min)
>  {
> +	/*
> +	 * If 'hidpid' mount option is set force a ptrace check,
> +	 * we indicate that we are using a filesystem syscall
> +	 * by passing PTRACE_MODE_READ_FSCREDS
> +	 */
> +	if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE)
> +		return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> +
>  	if (fs_info->hide_pid < hide_pid_min)
>  		return true;
>  	if (in_group_p(fs_info->pid_gid))
> @@ -3319,7 +3327,14 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
>  	if (!task)
>  		goto out;
>  
> +	/* Limit procfs to only ptraceable tasks */
> +	if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE) {
> +		if (!has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS))
> +			goto out_put_task;
> +	}
> +
>  	result = proc_pid_instantiate(dentry, task, NULL);
> +out_put_task:
>  	put_task_struct(task);
>  out:
>  	return result;
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 616e8976185c..62eae22403d2 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -47,6 +47,14 @@ static const struct fs_parameter_spec proc_fs_parameters[] = {
>  	{}
>  };
>  
> +static inline int valid_hidepid(unsigned int value)
> +{
> +	return (value == HIDEPID_OFF ||
> +		value == HIDEPID_NO_ACCESS ||
> +		value == HIDEPID_INVISIBLE ||
> +		value == HIDEPID_NOT_PTRACEABLE);

This likely easier to do with a ...MAX value? i.e.

	return (value < HIDEPID_OFF || value >= HIDEPID_MAX);

> +}
> +
>  static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  {
>  	struct proc_fs_context *ctx = fc->fs_private;
> @@ -63,10 +71,9 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		break;
>  
>  	case Opt_hidepid:
> +		if (!valid_hidepid(result.uint_32))
> +			return invalf(fc, "proc: unknown value of hidepid.\n");
>  		ctx->hidepid = result.uint_32;
> -		if (ctx->hidepid < HIDEPID_OFF ||
> -		    ctx->hidepid > HIDEPID_INVISIBLE)
> -			return invalfc(fc, "hidepid value must be between 0 and 2.\n");
>  		break;
>  
>  	default:
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index 7d852dbca253..21d19353fdc7 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -32,6 +32,7 @@ enum {
>  	HIDEPID_OFF	  = 0,
>  	HIDEPID_NO_ACCESS = 1,
>  	HIDEPID_INVISIBLE = 2,
> +	HIDEPID_NOT_PTRACEABLE = 4, /* Limit pids to only ptraceable pids */

This isn't a bit field -- shouldn't this be "3"?

	...
	HIDEPID_NOT_PTRACEABLE = 3,
	HIDEPID_MAX

etc?

>  };
>  
>  struct proc_fs_info {
> -- 
> 2.25.2
>
Alexey Gladkov March 28, 2020, 9:23 p.m. UTC | #2 
On Sat, Mar 28, 2020 at 01:40:03PM -0700, Kees Cook wrote:
> On Fri, Mar 27, 2020 at 06:23:26PM +0100, Alexey Gladkov wrote:
> > If "hidepid=4" mount option is set then do not instantiate pids that
> > we can not ptrace. "hidepid=4" means that procfs should only contain
> > pids that the caller can ptrace.
> > 
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> > Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> > ---
> >  fs/proc/base.c          | 15 +++++++++++++++
> >  fs/proc/root.c          | 13 ++++++++++---
> >  include/linux/proc_fs.h |  1 +
> >  3 files changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 43a28907baf9..1ebe9eba48ea 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -701,6 +701,14 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
> >  				 struct task_struct *task,
> >  				 int hide_pid_min)
> >  {
> > +	/*
> > +	 * If 'hidpid' mount option is set force a ptrace check,
> > +	 * we indicate that we are using a filesystem syscall
> > +	 * by passing PTRACE_MODE_READ_FSCREDS
> > +	 */
> > +	if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE)
> > +		return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> > +
> >  	if (fs_info->hide_pid < hide_pid_min)
> >  		return true;
> >  	if (in_group_p(fs_info->pid_gid))
> > @@ -3319,7 +3327,14 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
> >  	if (!task)
> >  		goto out;
> >  
> > +	/* Limit procfs to only ptraceable tasks */
> > +	if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE) {
> > +		if (!has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS))
> > +			goto out_put_task;
> > +	}
> > +
> >  	result = proc_pid_instantiate(dentry, task, NULL);
> > +out_put_task:
> >  	put_task_struct(task);
> >  out:
> >  	return result;
> > diff --git a/fs/proc/root.c b/fs/proc/root.c
> > index 616e8976185c..62eae22403d2 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -47,6 +47,14 @@ static const struct fs_parameter_spec proc_fs_parameters[] = {
> >  	{}
> >  };
> >  
> > +static inline int valid_hidepid(unsigned int value)
> > +{
> > +	return (value == HIDEPID_OFF ||
> > +		value == HIDEPID_NO_ACCESS ||
> > +		value == HIDEPID_INVISIBLE ||
> > +		value == HIDEPID_NOT_PTRACEABLE);
> 
> This likely easier to do with a ...MAX value? i.e.
> 
> 	return (value < HIDEPID_OFF || value >= HIDEPID_MAX);
> 
> > +}
> > +
> >  static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >  {
> >  	struct proc_fs_context *ctx = fc->fs_private;
> > @@ -63,10 +71,9 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >  		break;
> >  
> >  	case Opt_hidepid:
> > +		if (!valid_hidepid(result.uint_32))
> > +			return invalf(fc, "proc: unknown value of hidepid.\n");
> >  		ctx->hidepid = result.uint_32;
> > -		if (ctx->hidepid < HIDEPID_OFF ||
> > -		    ctx->hidepid > HIDEPID_INVISIBLE)
> > -			return invalfc(fc, "hidepid value must be between 0 and 2.\n");
> >  		break;
> >  
> >  	default:
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index 7d852dbca253..21d19353fdc7 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -32,6 +32,7 @@ enum {
> >  	HIDEPID_OFF	  = 0,
> >  	HIDEPID_NO_ACCESS = 1,
> >  	HIDEPID_INVISIBLE = 2,
> > +	HIDEPID_NOT_PTRACEABLE = 4, /* Limit pids to only ptraceable pids */
> 
> This isn't a bit field -- shouldn't this be "3"?
> 
> 	...
> 	HIDEPID_NOT_PTRACEABLE = 3,
> 	HIDEPID_MAX
> 
> etc?

I decided to choose 4 so that if later we need to be able to make a mask.
I am not sure that this parameter will not have values that cannot be used
together. Since now these parameters are becoming part of the public api,
I decided to add flexibility.
diff mbox series

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 43a28907baf9..1ebe9eba48ea 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -701,6 +701,14 @@  static bool has_pid_permissions(struct proc_fs_info *fs_info,
 				 struct task_struct *task,
 				 int hide_pid_min)
 {
+	/*
+	 * If 'hidpid' mount option is set force a ptrace check,
+	 * we indicate that we are using a filesystem syscall
+	 * by passing PTRACE_MODE_READ_FSCREDS
+	 */
+	if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE)
+		return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
+
 	if (fs_info->hide_pid < hide_pid_min)
 		return true;
 	if (in_group_p(fs_info->pid_gid))
@@ -3319,7 +3327,14 @@  struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
 	if (!task)
 		goto out;
 
+	/* Limit procfs to only ptraceable tasks */
+	if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE) {
+		if (!has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS))
+			goto out_put_task;
+	}
+
 	result = proc_pid_instantiate(dentry, task, NULL);
+out_put_task:
 	put_task_struct(task);
 out:
 	return result;
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 616e8976185c..62eae22403d2 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -47,6 +47,14 @@  static const struct fs_parameter_spec proc_fs_parameters[] = {
 	{}
 };
 
+static inline int valid_hidepid(unsigned int value)
+{
+	return (value == HIDEPID_OFF ||
+		value == HIDEPID_NO_ACCESS ||
+		value == HIDEPID_INVISIBLE ||
+		value == HIDEPID_NOT_PTRACEABLE);
+}
+
 static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
 	struct proc_fs_context *ctx = fc->fs_private;
@@ -63,10 +71,9 @@  static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		break;
 
 	case Opt_hidepid:
+		if (!valid_hidepid(result.uint_32))
+			return invalf(fc, "proc: unknown value of hidepid.\n");
 		ctx->hidepid = result.uint_32;
-		if (ctx->hidepid < HIDEPID_OFF ||
-		    ctx->hidepid > HIDEPID_INVISIBLE)
-			return invalfc(fc, "hidepid value must be between 0 and 2.\n");
 		break;
 
 	default:
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 7d852dbca253..21d19353fdc7 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -32,6 +32,7 @@  enum {
 	HIDEPID_OFF	  = 0,
 	HIDEPID_NO_ACCESS = 1,
 	HIDEPID_INVISIBLE = 2,
+	HIDEPID_NOT_PTRACEABLE = 4, /* Limit pids to only ptraceable pids */
 };
 
 struct proc_fs_info {