Re: [PATCH v5 2/2] proc: restrict /proc/pid/mem

[Adrian Ratiu]

On Thursday, June 06, 2024 20:45 EEST, Kees Cook <kees@xxxxxxxxxx> wrote:

> On Wed, Jun 05, 2024 at 07:49:31PM +0300, Adrian Ratiu wrote:
> > +	proc_mem.restrict_foll_force= [KNL]
> > +			Format: {all | ptracer}
> > +			Restricts the use of the FOLL_FORCE flag for /proc/*/mem access.
> > +			If restricted, the FOLL_FORCE flag will not be added to vm accesses.
> > +			Can be one of:
> > +			- 'all' restricts all access unconditionally.
> > +			- 'ptracer' allows access only for ptracer processes.
> > +			If not specified, FOLL_FORCE is always used.
> 
> It dawns on me that we likely need an "off" setting for these in case it
> was CONFIG-enabled...

Indeed, having CONFIG-enabled and disabling entirely via kernel
params is a valid usecase (eg for debug images with no restriction).

Will do in v6.

> 
> > +static int __init early_proc_mem_restrict_##name(char *buf)			\
> > +{										\
> > +	if (!buf)								\
> > +		return -EINVAL;							\
> > +										\
> > +	if (strcmp(buf, "all") == 0)						\
> > +		static_key_slow_inc(&proc_mem_restrict_##name##_all.key);	\
> > +	else if (strcmp(buf, "ptracer") == 0)					\
> > +		static_key_slow_inc(&proc_mem_restrict_##name##_ptracer.key);	\
> > +	return 0;								\
> > +}										\
> > +early_param("proc_mem.restrict_" #name, early_proc_mem_restrict_##name)
> 
> Why slow_inc here instead of the normal static_key_enable/disable?

No real reason, my mind was just more attuned to the inc/dec
semantics, however in this case we can just use enable/disable,
especially if they're faster.

I'll do this in v6.

> 
> And we should report misparsing too, so perhaps:

Ack

> > +static int __mem_open_access_permitted(struct file *file, struct task_struct *task)
> > +{
> > +	bool is_ptracer;
> > +
> > +	rcu_read_lock();
> > +	is_ptracer = current == ptrace_parent(task);
> > +	rcu_read_unlock();
> > +
> > +	if (file->f_mode & FMODE_WRITE) {
> > +		/* Deny if writes are unconditionally disabled via param */
> > +		if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_DEFAULT,
> > +					&proc_mem_restrict_open_write_all))
> > +			return -EACCES;
> > +
> > +		/* Deny if writes are allowed only for ptracers via param */
> > +		if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_PTRACE_DEFAULT,
> > +					&proc_mem_restrict_open_write_ptracer) &&
> > +		    !is_ptracer)
> > +			return -EACCES;
> > +	}
> > +
> > +	if (file->f_mode & FMODE_READ) {
> > +		/* Deny if reads are unconditionally disabled via param */
> > +		if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_DEFAULT,
> > +					&proc_mem_restrict_open_read_all))
> > +			return -EACCES;
> > +
> > +		/* Deny if reads are allowed only for ptracers via param */
> > +		if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_PTRACE_DEFAULT,
> > +					&proc_mem_restrict_open_read_ptracer) &&
> > +		    !is_ptracer)
> > +			return -EACCES;
> > +	}
> > +
> > +	return 0; /* R/W are not restricted */
> > +}
> 
> Given how deeply some of these behaviors may be in userspace, it might
> be more friendly to report the new restrictions with a pr_notice() so
> problems can be more easily tracked down. For example:
> 
> static void report_mem_rw_rejection(const char *action, struct task_struct *task)
> {
> 	pr_warn_ratelimited("Denied %s of /proc/%d/mem (%s) by pid %d (%s)\n",
> 			    action, task_pid_nr(task), task->comm,
> 			    task_pid_nr(current), current->comm);
> }
> 
> ...
> 
> 	if (file->f_mode & FMODE_WRITE) {
> 		/* Deny if writes are unconditionally disabled via param */
> 		if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_DEFAULT,
> 					&proc_mem_restrict_open_write_all)) {
> 			report_mem_rw_reject("all open-for-write");
> 			return -EACCES;
> 		}
> 
> 		/* Deny if writes are allowed only for ptracers via param */
> 		if (static_branch_maybe(CONFIG_PROC_MEM_RESTRICT_OPEN_WRITE_PTRACE_DEFAULT,
> 					&proc_mem_restrict_open_write_ptracer) &&
> 		    !is_ptracer)
> 			report_mem_rw_reject("non-ptracer open-for-write");
> 			return -EACCES;
> 	}
> 
> etc

Yes, will do in v6.

> Can we adjust the Kconfigs to match the bootparam arguments? i.e.
> instead of two for each mode, how about one with 3 settings ("all",
> "ptrace", or "off")

Sure. Thank you for all the code! All your help designing this
and code contributions are very much appreciated!

Do you want to be listed as co-author in v6?