[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2724ac-6662e300-3-2336898@243510220>
Date: Fri, 07 Jun 2024 11:38:12 +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...
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?
Powered by blists - more mailing lists