Received-SPF: pass (google.com: domain of linux-kernel+bounces-204834-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 6 Jun 2024 10:45:41 -0700
From: Kees Cook <kees@kernel.org>
To: Adrian Ratiu <adrian.ratiu@collabora.com>
Cc: linux-fsdevel@vger.kernel.org, linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
	linux-doc@vger.kernel.org, kernel@collabora.com, gbiv@google.com,
	ryanbeltran@google.com, inglorion@google.com, ajordanr@google.com,
	jorgelo@chromium.org, Guenter Roeck <groeck@chromium.org>,
	Doug Anderson <dianders@chromium.org>, Jann Horn <jannh@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Christian Brauner <brauner@kernel.org>, Jeff Xu <jeffxu@google.com>,
	Mike Frysinger <vapier@chromium.org>
Subject: Re: [PATCH v5 2/2] proc: restrict /proc/pid/mem
Message-ID: <202406060917.8DEE8E3@keescook>
References: <20240605164931.3753-1-adrian.ratiu@collabora.com>
 <20240605164931.3753-2-adrian.ratiu@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240605164931.3753-2-adrian.ratiu@collabora.com>

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);


-- 
Kees Cook