* Kees Cook <kees@xxxxxxxxxx> [250213 15:11]:
> On Thu, Feb 13, 2025 at 01:29:46PM -0500, Liam R. Howlett wrote:
> > * Jeff Xu <jeffxu@xxxxxxxxxxxx> [250213 12:17]:
> > > On Wed, Feb 12, 2025 at 7:05 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> > > >
> > > ...
> > > > >
> > > > > In this version, we've improved the handling of system mapping sealing from
> > > > > previous versions, instead of modifying the _install_special_mapping
> > > > > function itself, which would affect all architectures, we now call
> > > > > _install_special_mapping with a sealing flag only within the specific
> > > > > architecture that requires it. This targeted approach offers two key
> > > > > advantages: 1) It limits the code change's impact to the necessary
> > > > > architectures, and 2) It aligns with the software architecture by keeping
> > > > > the core memory management within the mm layer, while delegating the
> > > > > decision of sealing system mappings to the individual architecture, which
> > > > > is particularly relevant since 32-bit architectures never require sealing.
> > > > >
> > > > > Additionally, this patch introduces a new header,
> > > > > include/linux/usrprocess.h, which contains the mseal_system_mappings()
> > > > > function. This function helps the architecture determine if system
> > > > > mapping is enabled within the current kernel configuration. It can be
> > > > > extended in the future to handle opt-in/out prctl for enabling system
> > > > > mapping sealing at the process level or a kernel cmdline feature.
> > > > >
> > > > > A new header file was introduced because it was difficult to find the
> > > > > best location for this function. Although include/linux/mm.h was
> > > > > considered, this feature is more closely related to user processes
> > > > > than core memory management. Additionally, future prctl or kernel
> > > > > cmd-line implementations for this feature would not fit within the
> > > > > scope of core memory management or mseal.c. This is relevant because
> > > > > if we add unit-tests for mseal.c in the future, we would want to limit
> > > > > mseal.c's dependencies to core memory management.
> > > > >
> > > ...
> > > > >
> > > > > diff --git a/include/linux/userprocess.h b/include/linux/userprocess.h
> > > > > new file mode 100644
> > > > > index 000000000000..bd11e2e972c5
> > > > > --- /dev/null
> > > > > +++ b/include/linux/userprocess.h
> > > > > @@ -0,0 +1,18 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +#ifndef _LINUX_USER_PROCESS_H
> > > > > +#define _LINUX_USER_PROCESS_H
> > > > > +#include <linux/mm.h>
> > [...]
> >
> > Also, I don't agree with your stated reason for a new header.
> >
> > Please remove this header until it is needed. It can be added later if
> > it is needed. If we all had tiny headers because we might need them in
> > the future, we'd have serious issues finding anything and the list of
> > included headers would be huge.
> >
> > You have added unnecessary changes and complications to this patch set
> > on v5.
>
> In all fairness, I think v5 is significantly less complex overall. Jeff
> used his best judgement with a new header, and I don't think it's
> unreasonable. That it is unwanted is fine, mm.h it is, but I think it's
> clear the intent was trying to make this as least burdensome for the mm
> subsystem as possible.
This version is certainly easier to follow, but it's still more
complicated than it needs to be. Adding custom headers seems like an
unnecessary addition to previous versions.
I didn't mean this to be a jab or upsetting - and sorry to both of you,
I can see how it could be taken this way.
I was trying to point out that this is the same sort of thing that got
Jeff into trouble with Linus about over-engineering the v8 of the mseal
patch set [1].
...
> > >
> > > VM_SEALED isn't defined in 32-bit systems, and mseal.c isn't part of
> > > the build. This is intentional. Any 32-bit code trying to use the
> > > sealing function or the VM_SEALED flag will immediately fail
> > > compilation. This makes it easier to identify incorrect usage.
> >
> > So you are against using the #define because the VM_SYSTEM_SEAL will be
> > defined, even though it will be VM_NONE? This is no worse than a
> > function that returns 0, and it aligns better with what we have today in
> > that it can be put in the list of other flags.
>
> When I was reading through all of this and considering the history of
> its development goals, it strikes me that a function is easier for the
> future if/when this can be made a boot-time setting.
>
Reworking this change to function as a boot-time parameter, or whatever,
would not be a significant amount of work, if/when the time comes.
Since we don't know what the future holds, it it unnecessary to engineer
in a potential change for a future version when the change is easy
enough to make later and keep the code cleaner now.
> If mm maintainers prefer a #define for now, that's fine of course. The
> value of VM_SYSTEM_SEAL can be VM_NONE on 32-bit.
Thanks. I think having a flag with VM_NONE on 32-bit is just as sane as
a "flags |= system_seal()" call that unconditionally returns 0 on
32-bit.
>
> > Also, my vote for where you should put this code is clear: it should
> > live with the mseal #define in mm.h. You're going to need mm.h anyways,
> > and that's a big hint as to where it should live.
>
> Sounds good.
>
> > > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > > index d0d021b3fa3b..892d2bcdf397 100644
> > > > > --- a/init/Kconfig
> > > > > +++ b/init/Kconfig
> > > > > @@ -1882,6 +1882,24 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
> > > > > config ARCH_HAS_MEMBARRIER_SYNC_CORE
> > > > > bool
> > > > >
> > > > > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > >
> > > > ARCH_SUPPORTS_MSEAL_SYSTEM_MAPPINGS is more clear. HAS may mean it's
> > > > supported or it could mean it's enabled. SUPPORTS is more clear that
> > > > this is set if an arch supports the feature. After all, I think HAS
> > > > here means it "has support for" mseal system mappings.
> > > >
> > > > I see that both are used (HAS and SUPPORTS), but I'm still not sure what
> > > > HAS means in other contexts without digging.
> > > >
> > > The existing pattern is to indicate arch support with
> > > CONFIG_ARCH_HAS_N and security/KConfig to have CONFIG_N as the main
> > > switch. For example, CONFIG_ARCH_HAS_FORTIFY_SOURCE and
> > > CONFIG_FORTIFY_SOURCE. Since the MSEAL_SYSTEM_MAPPINGS is placed in
> > > security/Kconfig, hence I'm following the existing pattern in
> > > security/Kconfig.
> >
> > Okay, thanks. I really think SUPPORTS is more clear, but sticking with
> > whatever your area uses is also fine.
>
> Yeah, I've really scratched my head over what the best practice is here.
> There's a real mixture of "HAS" vs "SUPPORTS" across both hardware
> architectural support, software features, interface implementations,
> and compiler behavior that bridges between those. When I've looked in
> the past, it seemed that "HAS" was in the majority, but I've never been
> able to make sense of it.
>
> Let me look again. Today, HAS shows:
>
> $ git grep 'config .*_HAS_' | grep -v Documentation/ | \
> awk '{print $2}' | cut -d_ -f1 | sort | uniq -c | sort -n
> 1 ARM
> 1 MAC80211
> 1 PGTABLE
> 1 PPC
> 1 RUSTC
> 1 USB
> 2 ARM64
> 2 SIBYTE
> 2 SOC
> 3 FS
> 3 PAHOLE
> 6 SGI
> 6 TOOLCHAIN
> 10 ARC
> 20 AS
> 32 SYS
> 34 CPU
> 38 CC
> 105 ARCH
>
> Looking through them, it's tools (CC, AS, PAHOLE, etc), and
> system/cpu/architecture prefixes, with ARCH being a clear winner.
>
> SUPPORTS looks similar, but isn't as widely used:
>
> $ git grep 'config .*_SUPPORTS_' | grep -v Documentation/ | \
> awk '{print $2}' | cut -d_ -f1 | sort | uniq -c | sort -n
> 1 PPC64
> 1 X86
> 2 CLANG
> 2 GCC
> 2 RUSTC
> 8 CPU
> 38 SYS
> 71 ARCH
>
> The mips arch is using SYS, and distinguishes between HAS and SUPPORTS
> in the sense of "this kernel HAS support for CPU $foo, which SUPPORTS
> features x, y, z".
>
> Looking through ARCH_SUPPORTS, it's all software features. Looking
> through ARCH_HAS, it looks like a mix of hardware features
> and software features. Some software features are more about
> implementation availability, though (so "HAS" makes sense,
> but should maybe be "IMPLEMENTS"?) e.g. ARCH_HAS_SYSCALL_WRAPPER,
> ARCH_HAS_STRNLEN_USER, ARCH_HAS_FORTIFY_SOURCE, ARCH_HAS_ELF_RANDOMIZE,
> ARCH_HAS_STRICT_KERNEL_RWX, ARCH_HAS_STRICT_MODULE_RWX.
>
> So, I think I'd agree: this is about a software features and not an API
> implementation nor hardware feature, so let's use ARCH_SUPPORTS_.
This makes sense. I grepped for both as well and found that HAS
outnumbers SUPPORTS, but there are a lot of each. I figured using
whatever the subsystem uses is probably the right answer - your answer
is better.
Seeing them in the same kconfig option together gave me pause as to why
they were different. Thanks for digging into this!
Regards,
Liam
[1] https://lore.kernel.org/linux-mm/CAHk-=wjGGgfAoiEdPqLdib7VvQgG7uVXpTPzJ9jTW0HesRpPwQ@xxxxxxxxxxxxxx/
