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

[Alexey Gladkov]

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@xxxxxxxxxxxx>
> > Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> > Signed-off-by: Djalal Harouni <tixxdz@xxxxxxxxx>
> > Reviewed-by: Alexey Dobriyan <adobriyan@xxxxxxxxx>
> > Signed-off-by: Alexey Gladkov <gladkov.alexey@xxxxxxxxx>
> > ---
> >  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.

-- 
Rgrds, legion