On Thursday, June 06, 2024 20:45 EEST, Kees Cook <k...@kernel.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?
