Re: [PATCH v4 1/1] exec: seal system mappings

[Jeff Xu <jeffxu@chromium.org>]

On Mon, Nov 25, 2024 at 12:40 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Nov 25, 2024 at 08:20:21PM +0000, jeffxu@chromium.org wrote:
> > +/*
> > + * Kernel cmdline override for CONFIG_SEAL_SYSTEM_MAPPINGS
> > + */
> > +enum seal_system_mappings_type {
> > +     SEAL_SYSTEM_MAPPINGS_DISABLED,
> > +     SEAL_SYSTEM_MAPPINGS_ENABLED
> > +};
> > +
> > +static enum seal_system_mappings_type seal_system_mappings_v __ro_after_init =
> > +     IS_ENABLED(CONFIG_SEAL_SYSTEM_MAPPINGS) ? SEAL_SYSTEM_MAPPINGS_ENABLED :
> > +     SEAL_SYSTEM_MAPPINGS_DISABLED;
> > +
> > +static const struct constant_table value_table_sys_mapping[] __initconst = {
> > +     { "no", SEAL_SYSTEM_MAPPINGS_DISABLED},
> > +     { "yes", SEAL_SYSTEM_MAPPINGS_ENABLED},
> > +     { }
> > +};
> > +
> > +static int __init early_seal_system_mappings_override(char *buf)
> > +{
> > +     if (!buf)
> > +             return -EINVAL;
> > +
> > +     seal_system_mappings_v = lookup_constant(value_table_sys_mapping,
> > +                     buf, seal_system_mappings_v);
> > +     return 0;
> > +}
> > +
> > +early_param("exec.seal_system_mappings", early_seal_system_mappings_override);
>
> Are you paid by the line?
> This all seems ridiculously overcomplicated.
> Look at (first example I found) kgdbwait:
>
The example you provided doesn't seem to support the kernel cmd-line ?

> static int __init opt_kgdb_wait(char *str)
> {
>         kgdb_break_asap = 1;
>
>         kdb_init(KDB_INIT_EARLY);
>         if (kgdb_io_module_registered &&
>             IS_ENABLED(CONFIG_ARCH_HAS_EARLY_DEBUG))
>                 kgdb_initial_breakpoint();
>
>         return 0;
> }
> early_param("kgdbwait", opt_kgdb_wait);
>
There is an existing pattern of supporting kernel cmd line + KCONFIG
which I followed [1],
IMO, this fits this user-case really well, if you have a better
example, I'm happy to look.

[1] https://lore.kernel.org/lkml/20240802080225.89408-1-adrian.ratiu@collabora.com/

> I don't understand why you've created a new 'exec' namespace, and why
> this feature fits in 'exec'.  That seems like an implementation detail.
> I'd lose the "exec." prefix.

I would prefer some prefix to group these types of features.
vdso/vvar are sealed during the execve() call, so I choose "exec".
The next work I'm planning is sealing the NX stack, it would start
with the same prefix.

 If exec is not an intuitive prefix, I'm also happy with "process." prefix.

Thanks for reviewing

-Jeff