Re: [PATCH v3] proc: add config & param to block forcing mem writes

[Adrian Ratiu]

On Friday, July 26, 2024 13:18 EEST, Ard Biesheuvel <ardb at kernel.org> wrote:

> On Fri, 26 Jul 2024 at 11:11, Adrian Ratiu <adrian.ratiu at collabora.com> wrote:
> >
> > This adds a Kconfig option and boot param to allow removing
> > the FOLL_FORCE flag from /proc/pid/mem write calls because
> > it can be abused.
> >
> > The traditional forcing behavior is kept as default because
> > it can break GDB and some other use cases.
> >
> > Previously we tried a more sophisticated approach allowing
> > distributions to fine-tune /proc/pid/mem behavior, however
> > that got NAK-ed by Linus [1], who prefers this simpler
> > approach with semantics also easier to understand for users.
> >
> > Link: https://lore.kernel.org/lkml/CAHk-=wiGWLChxYmUA5HrT5aopZrB7_2VTa0NLZcxORgkUe5tEQ@mail.gmail.com/ [1]
> > Cc: Doug Anderson <dianders at chromium.org>
> > Cc: Jeff Xu <jeffxu at google.com>
> > Cc: Jann Horn <jannh at google.com>
> > Cc: Kees Cook <kees at kernel.org>
> > Cc: Christian Brauner <brauner at kernel.org>
> > Suggested-by: Linus Torvalds <torvalds at linux-foundation.org>
> > Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> > Signed-off-by: Adrian Ratiu <adrian.ratiu at collabora.com>
> > ---
> > Changes in v3:
> > * Simplified code to use shorthand ifs and a
> >   lookup_constant() table.
> >
> > Changes in v2:
> > * Added bootparam on top of Linus' patch.
> > * Slightly reworded commit msg.
> > ---
> >  .../admin-guide/kernel-parameters.txt         | 10 ++++
> >  fs/proc/base.c                                | 54 ++++++++++++++++++-
> >  security/Kconfig                              | 32 +++++++++++
> >  3 files changed, 95 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index c1134ad5f06d..793301f360ec 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -4791,6 +4791,16 @@
> >         printk.time=    Show timing data prefixed to each printk message line
> >                         Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
> >
> > +       proc_mem.force_override= [KNL]
> > +                       Format: {always | ptrace | never}
> > +                       Traditionally /proc/pid/mem allows users to override memory
> > +                       permissions. This allows people to limit that.
> 
> Better to use passive tense here rather than referring to 'users' and 'people'.
> 
> 'Traditionally, /proc/pid/mem allows memory permissions to be
> overridden without restrictions.
> This option may be set to restrict that'
> 
> > +                       Can be one of:
> > +                       - 'always' traditional behavior always allows mem overrides.
> 
> punctuation please
> 
> > +                       - 'ptrace' only allow for active ptracers.
> > +                       - 'never'  never allow mem permission overrides.
> 
> Please be consistent: 'mem overrides' or 'mem permission overrides' in
> both instances.
> 
> > +                       If not specified, default is always.
> 
> 'always'
> 
> > +
> >         processor.max_cstate=   [HW,ACPI]
> >                         Limit processor to maximum C-state
> >                         max_cstate=9 overrides any DMI blacklist limit.
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 72a1acd03675..0ca3fc3d9e0e 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -85,6 +85,7 @@
> >  #include <linux/elf.h>
> >  #include <linux/pid_namespace.h>
> >  #include <linux/user_namespace.h>
> > +#include <linux/fs_parser.h>
> >  #include <linux/fs_struct.h>
> >  #include <linux/slab.h>
> >  #include <linux/sched/autogroup.h>
> > @@ -117,6 +118,35 @@
> >  static u8 nlink_tid __ro_after_init;
> >  static u8 nlink_tgid __ro_after_init;
> >
> > +enum proc_mem_force {
> > +       PROC_MEM_FORCE_ALWAYS,
> > +       PROC_MEM_FORCE_PTRACE,
> > +       PROC_MEM_FORCE_NEVER
> > +};
> > +
> > +static enum proc_mem_force proc_mem_force_override __ro_after_init =
> > +       IS_ENABLED(CONFIG_PROC_MEM_ALWAYS_FORCE) ? PROC_MEM_FORCE_ALWAYS :
> > +       IS_ENABLED(CONFIG_PROC_MEM_FORCE_PTRACE) ? PROC_MEM_FORCE_PTRACE :
> > +       PROC_MEM_FORCE_NEVER;
> > +
> > +struct constant_table proc_mem_force_table[] = {
> 
> This can be static const __initconst
> 
> > +       { "always", PROC_MEM_FORCE_ALWAYS },
> > +       { "ptrace", PROC_MEM_FORCE_PTRACE },
> > +       { }
> > +};
> > +
> > +static int __init early_proc_mem_force_override(char *buf)
> > +{
> > +       if (!buf)
> > +               return -EINVAL;
> > +
> 
> Can this ever happen?

Not sure, many calls simply ignore this case while others
like this [1] printk example do test it. I'm inclined to think
it can't happen however it's still to good to error check.

Thanks for all the suggestions, I'll leave this a bit for others
to get a chance to review, then send another iteration.

[1] https://elixir.bootlin.com/linux/v6.10.1/source/kernel/printk/printk.c#L1051