Message-ID: <3304e0-6669e580-9f9-33d83680@155585222>
Date: Wed, 12 Jun 2024 19:13:41 +0100
From: "Adrian Ratiu" <adrian.ratiu@...labora.com>
To: "Kees Cook" <kees@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org, linux-doc@...r.kernel.org, kernel@...labora.com, gbiv@...gle.com, ryanbeltran@...gle.com, inglorion@...gle.com, ajordanr@...gle.com, jorgelo@...omium.org, "Guenter Roeck" <groeck@...omium.org>, "Doug Anderson" <dianders@...omium.org>, "Jann Horn" <jannh@...gle.com>, "Andrew Morton" <akpm@...ux-foundation.org>, "Randy Dunlap" <rdunlap@...radead.org>, "Christian Brauner" <brauner@...nel.org>, "Jeff Xu" <jeffxu@...gle.com>, "Mike Frysinger" <vapier@...omium.org>
Subject: Re: [PATCH v5 2/2] proc: restrict /proc/pid/mem

On Thursday, June 06, 2024 20:45 EEST, Kees Cook <kees@...nel.org> 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...
> 
> > +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?
> 
> And we should report misparsing too, so perhaps:
> 
> static int __init early_proc_mem_restrict_##name(char *buf)			\
> {										\
> 	if (!buf)								\
> 		return -EINVAL;							\
> 										\
> 	if (strcmp(buf, "all") == 0) {						\
> 		static_key_enable(&proc_mem_restrict_##name##_all.key);		\
> 		static_key_disable(&proc_mem_restrict_##name##_ptracer.key);	\
> 	} else if (strcmp(buf, "ptracer") == 0) {				\
> 		static_key_disable(&proc_mem_restrict_##name##_all.key);	\
> 		static_key_enable(&proc_mem_restrict_##name##_ptracer.key);	\
> 	} else if (strcmp(buf, "off") == 0) {					\
> 		static_key_disable(&proc_mem_restrict_##name##_all.key);	\
> 		static_key_disable(&proc_mem_restrict_##name##_ptracer.key);	\
> 	} else									\
> 		pr_warn("%s: ignoring unknown option '%s'\n",			\
> 			"proc_mem.restrict_" #name, buf);			\
> 	return 0;								\
> }										\
> early_param("proc_mem.restrict_" #name, early_proc_mem_restrict_##name)
> 
> > +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
> 
> > +static bool __mem_rw_current_is_ptracer(struct file *file)
> > +{
> > +	struct inode *inode = file_inode(file);
> > +	struct task_struct *task = get_proc_task(inode);
> > +	struct mm_struct *mm = NULL;
> > +	int is_ptracer = false, has_mm_access = false;
> > +
> > +	if (task) {
> > +		rcu_read_lock();
> > +		is_ptracer = current == ptrace_parent(task);
> > +		rcu_read_unlock();
> > +
> > +		mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> > +		if (mm && file->private_data == mm) {
> > +			has_mm_access = true;
> > +			mmput(mm);
> > +		}
> > +
> > +		put_task_struct(task);
> > +	}
> > +
> > +	return is_ptracer && has_mm_access;
> > +}
> 
> Thanks; this looks right to me now!
> 
> > +menu "Procfs mem restriction options"
> > +
> > +config PROC_MEM_RESTRICT_FOLL_FORCE_DEFAULT
> > +	bool "Restrict all FOLL_FORCE flag usage"
> > +	default n
> > +	help
> > +	  Restrict all FOLL_FORCE usage during /proc/*/mem RW.
> > +	  Debuggers like GDB require using FOLL_FORCE for basic
> > +	  functionality.
> > +
> > +config PROC_MEM_RESTRICT_FOLL_FORCE_PTRACE_DEFAULT
> > +	bool "Restrict FOLL_FORCE usage except for ptracers"
> > +	default n
> > +	help
> > +	  Restrict FOLL_FORCE usage during /proc/*/mem RW, except
> > +	  for ptracer processes. Debuggers like GDB require using
> > +	  FOLL_FORCE for basic functionality.
> 
> 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")
> 
> choice
> 	prompt "Restrict /proc/pid/mem FOLL_FORCE usage"
> 	default PROC_MEM_RESTRICT_FOLL_FORCE_OFF
> 	help
> 	  Reading and writing of /proc/pid/mem bypasses memory permission
> 	  checks due to the internal use of the FOLL_FORCE flag. This can be
> 	  used by attackers to manipulate process memory contents that
> 	  would have been otherwise protected. However, debuggers, like GDB,
> 	  use this to set breakpoints, etc. To force debuggers to fall back
> 	  to PEEK/POKE, see PROC_MEM_RESTRICT_OPEN_WRITE_ALL.
> 
> 	config PROC_MEM_RESTRICT_FOLL_FORCE_OFF
> 	bool "Do not restrict FOLL_FORCE usage with /proc/pid/mem (regular)"
> 	help
> 	  Regular behavior: continue to use the FOLL_FORCE flag for
> 	  /proc/pid/mem access.
> 
> 	config PROC_MEM_RESTRICT_FOLL_FORCE_PTRACE
> 	bool "Only allow ptracers to use FOLL_FORCE with /proc/pid/mem (safer)"
> 	help
> 	  Only use the FOLL_FORCE flag for /proc/pid/mem access when the
> 	  current task is the active ptracer of the target task. (Safer,
> 	  least disruptive to most usage patterns.)
> 
> 	config PROC_MEM_RESTRICT_FOLL_FORCE_ALL
> 	bool "Do not use FOLL_FORCE with /proc/pid/mem (safest)"
> 	help
> 	  Remove the FOLL_FORCE flag for all /proc/pid/mem accesses.
> 	  (Safest, but may be disruptive to some usage patterns.)
> endchoice
> 
> Then the static_keys can be defined like this mess (I couldn't find a
> cleaner way to do it):
> 
> #define DEFINE_STATIC_KEY_PROC_MEM_ALL(name) \
> 	DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_##name##_all);	\
> 	DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_ptracer);
> #define DEFINE_STATIC_KEY_PROC_MEM_PTRACE(name) \
> 	DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_all);	\
> 	DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_##name##_ptracer);
> #define DEFINE_STATIC_KEY_PROC_MEM_OFF(name) \
> 	DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_all);	\
> 	DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_##name##_ptracer);
> 
> #define DEFINE_STATIC_KEY_PROC_MEM_0(level, name)
> #define DEFINE_STATIC_KEY_PROC_MEM_1(level, name)		\
> 	DEFINE_STATIC_KEY_PROC_MEM_##level(name)
> 
> #define _DEFINE_STATIC_KEY_PROC_MEM_PICK(enabled, level, name)   \
> DEFINE_STATIC_KEY_PROC_MEM_##enabled(level, name)
> 
> #define DEFINE_STATIC_KEY_PROC_MEM_PICK(enabled, level, name)   \
> _DEFINE_STATIC_KEY_PROC_MEM_PICK(enabled, level, name)
> 
> #define DEFINE_STATIC_KEY_PROC_MEM(CFG, name)			\
> DEFINE_STATIC_KEY_PROC_MEM_PICK(IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_##CFG##_ALL), ALL, name)
> DEFINE_STATIC_KEY_PROC_MEM_PICK(IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_##CFG##_PTRACE), PTRACE, name)
> DEFINE_STATIC_KEY_PROC_MEM_PICK(IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_##CFG##_OFF), OFF, name)
> 
> #define DEFINE_EARLY_PROC_MEM_RESTRICT(CFG, name)				\
> DEFINE_STATIC_KEY_PROC_MEM(CFG, name)						\
> static int __init early_proc_mem_restrict_##name(char *buf)			\
> {										\
> 	if (!buf)								\
> 		return -EINVAL;							\
> 										\
> 	if (strcmp(buf, "all") == 0) {						\
> 		static_key_enable(&proc_mem_restrict_##name##_all.key);		\
> 		static_key_disable(&proc_mem_restrict_##name##_ptracer.key);	\
> 	} else if (strcmp(buf, "ptracer") == 0) {				\
> 		static_key_disable(&proc_mem_restrict_##name##_all.key);	\
> 		static_key_enable(&proc_mem_restrict_##name##_ptracer.key);	\
> 	} else if (strcmp(buf, "off") == 0) {					\
> 		static_key_disable(&proc_mem_restrict_##name##_all.key);	\
> 		static_key_disable(&proc_mem_restrict_##name##_ptracer.key);	\
> 	} else									\
> 		pr_warn("%s: ignoring unknown option '%s'\n",			\
> 			"proc_mem.restrict_" #name, buf);			\
> 	return 0;								\
> }										\
> early_param("proc_mem.restrict_" #name, early_proc_mem_restrict_##name)
> 
> DEFINE_EARLY_PROC_MEM_RESTRICT(OPEN_READ, open_read);
> DEFINE_EARLY_PROC_MEM_RESTRICT(OPEN_WRITE, open_write);
> DEFINE_EARLY_PROC_MEM_RESTRICT(WRITE, write);
> DEFINE_EARLY_PROC_MEM_RESTRICT(FOLL_FORCE, foll_force);

Hello again,

I tried very hard to make the above work these past few days and gave up.
Couldn't find a way to get it to compile.
Tried to also debug the compiler preprocess output and my head hurts. :)

Would macros like the following be acceptable?
I know it's more verbose but also much easier to understand and it works.

#if IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_ALL)
DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_open_read_all);
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_ptracer);
#elif IS_ENABLED(CONFIG_PROC_MEM_RESTRICT_OPEN_READ_PTRACE)
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_all);
DEFINE_STATIC_KEY_TRUE_RO(proc_mem_restrict_open_read_ptracer);
#else
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_all);
DEFINE_STATIC_KEY_FALSE_RO(proc_mem_restrict_open_read_ptracer);
#endif

Powered by blists - more mailing lists