| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4e7088eb-b017-4d8b-8e0f-5cb409b112cb@lucifer.local>
Date: Tue, 3 Dec 2024 20:16:40 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Jeff Xu <jeffxu@...omium.org>
Cc: Vlastimil Babka <vbabka@...e.cz>, akpm@...ux-foundation.org,
keescook@...omium.org, jannh@...gle.com, torvalds@...ux-foundation.org,
adhemerval.zanella@...aro.org, oleg@...hat.com,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
linux-mm@...ck.org, jorgelo@...omium.org, sroettger@...gle.com,
ojeda@...nel.org, adobriyan@...il.com, anna-maria@...utronix.de,
mark.rutland@....com, linus.walleij@...aro.org, Jason@...c4.com,
deller@....de, rdunlap@...radead.org, davem@...emloft.net, hch@....de,
peterx@...hat.com, hca@...ux.ibm.com, f.fainelli@...il.com,
gerg@...nel.org, dave.hansen@...ux.intel.com, mingo@...nel.org,
ardb@...nel.org, Liam.Howlett@...cle.com, mhocko@...e.com,
42.hyeyoo@...il.com, peterz@...radead.org, ardb@...gle.com,
enh@...gle.com, rientjes@...gle.com, groeck@...omium.org,
mpe@...erman.id.au
Subject: Re: [PATCH v4 1/1] exec: seal system mappings
NACK.
Unfortunately past experience indicates that engaging in a back-and-forth
is not productive.
Please re-read what I've said and properly address the very serious
concerns raised.
This series is unacceptable in its current form as it allows untested
architectures and known broken configurations to enable this feature.
On Tue, Dec 03, 2024 at 10:19:31AM -0800, Jeff Xu wrote:
> On Mon, Dec 2, 2024 at 11:35 PM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > On Mon, Dec 02, 2024 at 12:38:27PM -0800, Jeff Xu wrote:
> > > On Mon, Dec 2, 2024 at 10:29 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@...cle.com> wrote:
> > > >
> > > > On Mon, Nov 25, 2024 at 08:20:21PM +0000, jeffxu@...omium.org wrote:
> > > > > From: Jeff Xu <jeffxu@...omium.org>
> > > > >
> > > > > Seal vdso, vvar, sigpage, uprobes and vsyscall.
> > > > >
> > > > > Those mappings are readonly or executable only, sealing can protect
> > > > > them from ever changing or unmapped during the life time of the process.
> > > > > For complete descriptions of memory sealing, please see mseal.rst [1].
> > > > >
> > > > > System mappings such as vdso, vvar, and sigpage (for arm) are
> > > > > generated by the kernel during program initialization, and are
> > > > > sealed after creation.
> > > > >
> > > > > Unlike the aforementioned mappings, the uprobe mapping is not
> > > > > established during program startup. However, its lifetime is the same
> > > > > as the process's lifetime [2]. It is sealed from creation.
> > > > >
> > > > > The vdso, vvar, sigpage, and uprobe mappings all invoke the
> > > > > _install_special_mapping() function. As no other mappings utilize this
> > > > > function, it is logical to incorporate sealing logic within
> > > > > _install_special_mapping(). This approach avoids the necessity of
> > > > > modifying code across various architecture-specific implementations.
> > > > >
> > > > > The vsyscall mapping, which has its own initialization function, is
> > > > > sealed in the XONLY case, it seems to be the most common and secure
> > > > > case of using vsyscall.
> > > > >
> > > > > It is important to note that the CHECKPOINT_RESTORE feature (CRIU) may
> > > > > alter the mapping of vdso, vvar, and sigpage during restore
> > > > > operations. Consequently, this feature cannot be universally enabled
> > > > > across all systems.
> > > > >
> > > > > Currently, memory sealing is only functional in a 64-bit kernel
> > > > > configuration.
> > > > >
> > > > > To enable this feature, the architecture needs to be tested to
> > > > > confirm that it doesn't unmap/remap system mappings during the
> > > > > the life time of the process. After the architecture enables
> > > > > ARCH_HAS_SEAL_SYSTEM_MAPPINGS, a distribution can set
> > > > > CONFIG_SEAL_SYSTEM_MAPPING to manage access to the feature.
> > > > > Alternatively, kernel command line (exec.seal_system_mappings)
> > > > > enables this feature also.
> > > > >
> > > > > This feature is tested using ChromeOS and Android on X86_64 and ARM64,
> > > > > therefore ARCH_HAS_SEAL_SYSTEM_MAPPINGS is set for X86_64 and ARM64.
> > > > > Other architectures can enable this after testing. No specific hardware
> > > > > features from the CPU are needed.
> > > > >
> > > > > This feature's security enhancements will benefit ChromeOS, Android,
> > > > > and other secure-by-default systems.
> > > > >
> > > > > [1] Documentation/userspace-api/mseal.rst
> > > > > [2] https://lore.kernel.org/all/CABi2SkU9BRUnqf70-nksuMCQ+yyiWjo3fM4XkRkL-NrCZxYAyg@mail.gmail.com/
> > > > > Signed-off-by: Jeff Xu <jeffxu@...omium.org>
> > > > > ---
> > > > > .../admin-guide/kernel-parameters.txt | 11 ++++++
> > > > > Documentation/userspace-api/mseal.rst | 4 ++
> > > > > arch/arm64/Kconfig | 1 +
> > > > > arch/x86/Kconfig | 1 +
> > > > > arch/x86/entry/vsyscall/vsyscall_64.c | 8 +++-
> > > > > include/linux/mm.h | 12 ++++++
> > > > > init/Kconfig | 25 ++++++++++++
> > > > > mm/mmap.c | 10 +++++
> > > > > mm/mseal.c | 39 +++++++++++++++++++
> > > > > security/Kconfig | 24 ++++++++++++
> > > > > 10 files changed, 133 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > > index e7bfe1bde49e..f63268341739 100644
> > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > @@ -1538,6 +1538,17 @@
> > > > > Permit 'security.evm' to be updated regardless of
> > > > > current integrity status.
> > > > >
> > > > > + exec.seal_system_mappings = [KNL]
> > > > > + Format: { no | yes }
> > > > > + Seal system mappings: vdso, vvar, sigpage, vsyscall,
> > > > > + uprobe.
> > > > > + - 'no': do not seal system mappings.
> > > > > + - 'yes': seal system mappings.
> > > > > + This overrides CONFIG_SEAL_SYSTEM_MAPPINGS=(y/n)
> > > > > + If not specified or invalid, default is the value set by
> > > > > + CONFIG_SEAL_SYSTEM_MAPPINGS.
> > > > > + This option has no effect if CONFIG_64BIT=n
> > > > > +
> > > > > early_page_ext [KNL,EARLY] Enforces page_ext initialization to earlier
> > > > > stages so cover more early boot allocations.
> > > > > Please note that as side effect some optimizations
> > > > > diff --git a/Documentation/userspace-api/mseal.rst b/Documentation/userspace-api/mseal.rst
> > > > > index 41102f74c5e2..bec122318a59 100644
> > > > > --- a/Documentation/userspace-api/mseal.rst
> > > > > +++ b/Documentation/userspace-api/mseal.rst
> > > > > @@ -130,6 +130,10 @@ Use cases
> > > > >
> > > > > - Chrome browser: protect some security sensitive data structures.
> > > > >
> > > > > +- seal system mappings:
> > > > > + kernel config CONFIG_SEAL_SYSTEM_MAPPINGS seals system mappings such
> > > > > + as vdso, vvar, sigpage, uprobes and vsyscall.
> > > > > +
> > > > > When not to use mseal
> > > > > =====================
> > > > > Applications can apply sealing to any virtual memory region from userspace,
> > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > > index 63de71544d95..fc5da8f74342 100644
> > > > > --- a/arch/arm64/Kconfig
> > > > > +++ b/arch/arm64/Kconfig
> > > > > @@ -44,6 +44,7 @@ config ARM64
> > > > > select ARCH_HAS_SETUP_DMA_OPS
> > > > > select ARCH_HAS_SET_DIRECT_MAP
> > > > > select ARCH_HAS_SET_MEMORY
> > > > > + select ARCH_HAS_SEAL_SYSTEM_MAPPINGS
> > > > > select ARCH_STACKWALK
> > > > > select ARCH_HAS_STRICT_KERNEL_RWX
> > > > > select ARCH_HAS_STRICT_MODULE_RWX
> > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > > index 1ea18662942c..5f6bac99974c 100644
> > > > > --- a/arch/x86/Kconfig
> > > > > +++ b/arch/x86/Kconfig
> > > > > @@ -26,6 +26,7 @@ config X86_64
> > > > > depends on 64BIT
> > > > > # Options that are inherently 64-bit kernel only:
> > > > > select ARCH_HAS_GIGANTIC_PAGE
> > > > > + select ARCH_HAS_SEAL_SYSTEM_MAPPINGS
> > > > > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
> > > > > select ARCH_SUPPORTS_PER_VMA_LOCK
> > > > > select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> > > > > diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
> > > > > index 2fb7d53cf333..30e0958915ca 100644
> > > > > --- a/arch/x86/entry/vsyscall/vsyscall_64.c
> > > > > +++ b/arch/x86/entry/vsyscall/vsyscall_64.c
> > > > > @@ -366,8 +366,12 @@ void __init map_vsyscall(void)
> > > > > set_vsyscall_pgtable_user_bits(swapper_pg_dir);
> > > > > }
> > > > >
> > > > > - if (vsyscall_mode == XONLY)
> > > > > - vm_flags_init(&gate_vma, VM_EXEC);
> > > > > + if (vsyscall_mode == XONLY) {
> > > > > + unsigned long vm_flags = VM_EXEC;
> > > > > +
> > > > > + vm_flags |= seal_system_mappings();
> > > > > + vm_flags_init(&gate_vma, vm_flags);
> > > > > + }
> > > > >
> > > > > BUILD_BUG_ON((unsigned long)__fix_to_virt(VSYSCALL_PAGE) !=
> > > > > (unsigned long)VSYSCALL_ADDR);
> > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > index df0a5eac66b7..f787d6c85cbb 100644
> > > > > --- a/include/linux/mm.h
> > > > > +++ b/include/linux/mm.h
> > > > > @@ -4238,4 +4238,16 @@ int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *st
> > > > > int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status);
> > > > > int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> > > > >
> > > > > +#ifdef CONFIG_64BIT
> > > > > +/*
> > > > > + * return VM_SEALED if seal system mapping is enabled.
> > > > > + */
> > > > > +unsigned long seal_system_mappings(void);
> > > > > +#else
> > > > > +static inline unsigned long seal_system_mappings(void)
> > > > > +{
> > > > > + return 0;
> > > > > +}
> > > >
> > > > OK so we can set seal system mappings on a 32-bit system and
> > > > silently... just not do it?...
> > > >
> > > I don't understand what you meant.
> > >
> > > The function returns the vm_flags for seal system mappings.
> > > In 32 bit, it returns 0.
> > >
> > > the caller (in mmap.c) does below:
> > > vm_flags |= seal_system_mappings();
> > >
> > > (The pattern is recommended by Liam. )
> > >
> > > Is that because the function name is misleading ? I can change it to
> > > seal_flags_system_mappings() if there is no objection to the long
> > > name.
> >
> > No, I'm saying that you're making it possible for somebody to enable this
> > feature on a 32-bit system, and to think it's enabled and that they're
> > protected when in fact they're not.
> >
> The kernel cmdline change already has comments about 32-bit: see:
> kernel-parameters.txt
> "This option has no effect if CONFIG_64BIT=n"
>
> > Which is, security-wise, I think rather unwise.
> >
> > Again it's an argument against a cmdline parameter. See below.
> >
> > >
> > > > > +#endif
> > > > > +
> > > > > #endif /* _LINUX_MM_H */
> > > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > > index 1aa95a5dfff8..614719259aa0 100644
> > > > > --- a/init/Kconfig
> > > > > +++ b/init/Kconfig
> > > > > @@ -1860,6 +1860,31 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
> > > > > config ARCH_HAS_MEMBARRIER_SYNC_CORE
> > > > > bool
> > > > >
> > > > > +config ARCH_HAS_SEAL_SYSTEM_MAPPINGS
> > > > > + bool
> > > > > + help
> > > > > + Control SEAL_SYSTEM_MAPPINGS access based on architecture.
> > > > > +
> > > > > + A 64-bit kernel is required for the memory sealing feature.
> > > > > + No specific hardware features from the CPU are needed.
> > > > > +
> > > > > + To enable this feature, the architecture needs to be tested to
> > > > > + confirm that it doesn't unmap/remap system mappings during the
> > > > > + the life time of the process. After the architecture enables this,
> > > > > + a distribution can set CONFIG_SEAL_SYSTEM_MAPPING to manage access
> > > > > + to the feature.
> > > > > +
> > > > > + The CONFIG_SEAL_SYSTEM_MAPPINGS already checks the CHECKPOINT_RESTORE
> > > > > + feature, which is known to remap/unmap vdso. Thus, the presence of
> > > > > + CHECKPOINT_RESTORE is not considered a factor in enabling
> > > > > + ARCH_HAS_SEAL_SYSTEM_MAPPINGS for a architecture.
> > > > > +
> > > > > + For complete list of system mappings, please see
> > > > > + CONFIG_SEAL_SYSTEM_MAPPINGS.
> > > > > +
> > > > > + For complete descriptions of memory sealing, please see
> > > > > + Documentation/userspace-api/mseal.rst
> > > > > +
> > > > > config HAVE_PERF_EVENTS
> > > > > bool
> > > > > help
> > > > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > > > index 57fd5ab2abe7..bc694c555805 100644
> > > > > --- a/mm/mmap.c
> > > > > +++ b/mm/mmap.c
> > > > > @@ -2133,6 +2133,16 @@ struct vm_area_struct *_install_special_mapping(
> > > > > unsigned long addr, unsigned long len,
> > > > > unsigned long vm_flags, const struct vm_special_mapping *spec)
> > > > > {
> > > > > + /*
> > > > > + * At present, all mappings (vdso, vvar, sigpage, and uprobe) that
> > > > > + * invoke the _install_special_mapping function can be sealed.
> > > > > + * Therefore, it is logical to call the seal_system_mappings_enabled()
> > > > > + * function here. In the future, if this is not the case, i.e. if certain
> > > > > + * mappings cannot be sealed, then it would be necessary to move this
> > > > > + * check to the calling function.
> > > > > + */
> > > > > + vm_flags |= seal_system_mappings();
> > > > > +
> > > > > return __install_special_mapping(mm, addr, len, vm_flags, (void *)spec,
> > > > > &special_mapping_vmops);
> > > > > }
> > > > > diff --git a/mm/mseal.c b/mm/mseal.c
> > > > > index ece977bd21e1..80126d6231bb 100644
> > > > > --- a/mm/mseal.c
> > > > > +++ b/mm/mseal.c
> > > > > @@ -7,6 +7,7 @@
> > > > > * Author: Jeff Xu <jeffxu@...omium.org>
> > > > > */
> > > > >
> > > > > +#include <linux/fs_parser.h>
> > > > > #include <linux/mempolicy.h>
> > > > > #include <linux/mman.h>
> > > > > #include <linux/mm.h>
> > > > > @@ -266,3 +267,41 @@ SYSCALL_DEFINE3(mseal, unsigned long, start, size_t, len, unsigned long,
> > > > > {
> > > > > return do_mseal(start, len, flags);
> > > > > }
> > > > > +
> > > > > +/*
> > > > > + * 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);
> > > > > +
> > > > > +unsigned long seal_system_mappings(void)
> > > > > +{
> > > > > + if (seal_system_mappings_v == SEAL_SYSTEM_MAPPINGS_ENABLED)
> > > > > + return VM_SEALED;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > diff --git a/security/Kconfig b/security/Kconfig
> > > > > index 28e685f53bd1..5bbb8d989d79 100644
> > > > > --- a/security/Kconfig
> > > > > +++ b/security/Kconfig
> > > > > @@ -51,6 +51,30 @@ config PROC_MEM_NO_FORCE
> > > > >
> > > > > endchoice
> > > > >
> > > > > +config SEAL_SYSTEM_MAPPINGS
> > > > > + bool "seal system mappings"
> > > >
> > > > I'd prefer an 'mseal' here please, it's becoming hard to grep for this
> > > > stuff. We overload 'seal' too much and I want to be able to identify what
> > > > is a memfd seal and what is an mseal or whatever else...
> > > >
> > > I m OK with MSEAL_
> >
> > Thanks.
> >
> > >
> > > > > + default n
> > > > > + depends on 64BIT
> > > > > + depends on ARCH_HAS_SEAL_SYSTEM_MAPPINGS
> > > > > + depends on !CHECKPOINT_RESTORE
> > > >
> > > > I don't know why we bother setting restrictions on this but allow them to
> > > > be overriden with a boot flag?
> > > >
> > > The idea is a distribution might not enable kernel security features
> > > by default, and kernel cmdline provides flexibility to let users
> > > enable it.
> > >
> > > This is the same approach as proc_mem.force_override kernel cmd line
> > > where Kees recommended [1], I would prefer to keep this as is.
> > >
> > > [1] https://lore.kernel.org/all/202402261110.B8129C002@keescook/
> > >
> >
> > This is flawed on multiple levels. Firstly, from the linked change:
> >
> > +config SECURITY_PROC_MEM_RESTRICT_WRITES
> > + bool "Restrict /proc/<pid>/mem write access"
> > + default n
> > + help
> >
> > There are no 'depends on'. Yours has 'depends on' which you've just
> > rendered totally irrelevant including _allowing the enabling of this
> > feature in broken situations_ like CRIU, as I mentioned below.
> >
> > For another, the linked feature changes behaviour and a user may or may not
> > want to allow the ability to write to /proc/<pid>/mem which is ENTIRELY
> > DIFFERENT from this proposed feature.
> >
> > Under what circumstances could a user possibly want to write VVAR, VDSO,
> > etc. etc.? It just makes absolutely no sense for this to be a boot switch.
> >
> > So the arguments presented there have zero bearing on this series.
> >
> > > > This means somebody with CRIU enabled could enable this and have a broken
> > > > kernel right? We can't allow that.
> >
> > Please do not ignore review comments like this.
> >
> kernel cmdline is a valid user case. The reasoning is explained in
> the previous response, it allows users to enable security features
> without having to rebuild the distribution's kernel.
>
> For the concern that CRIU user enables this feature through kernel
> cmdline mistakenly, there is already instruction and comments
> throughout code to make aware of the impact to CRIU after enabling
> sealing for system-mapping. In my view: users who want to enable this
> feature through kernel cmdline should already have enough context
> about this, i.e. it is an educated decision rather than experimenting.
>
> That said, if we want to protect those CRIU users from mistakenly
> enabling memory sealing, we could add a check to verify CRIU is not
> enabled, i.e. if CRIU is enabled, the kernel cmd line has no effect.
> Although in my view this is an extra complicity without meaningful
> benefit - If it is the user's educated choice, I prefer to honor it.
>
>
>
> > > >
> > > > I'd much prefer we either:
> > > >
> > > > 1. Just have a CONFIG_MSEAL_SYSTEM_MAPPINGS flag. _or_
> > > > 2. Have CONFIG_MSEAL_SYSTEM_MAPPINGS enable, allow kernel flag to disable.
> > > >
> > > > In both cases you #ifdef on CONFIG_MSEAL_SYSTEM_MAPPINGS, and the
> > > > restrictions appply correctly.
> > > >
> > > > If in the future we decide this feature is stable and ready and good to
> > > > enable globally we can just change the default on this to y at some later
> > > > date?
> > > >
> > > > Otherwise it just seems like in a effect the kernel command line flag is a
> > > > debug flag to experiment on arbitrary kernels?
> > > >
> > > > > + help
> > > > > + Seal system mappings such as vdso, vvar, sigpage, vsyscall, uprobes.
> > > > > +
> > > > > + A 64-bit kernel is required for the memory sealing feature.
> > > > > + No specific hardware features from the CPU are needed.
> > > > > +
> > > > > + Depends on the ARCH_HAS_SEAL_SYSTEM_MAPPINGS.
> > > > > +
> > > > > + CHECKPOINT_RESTORE might relocate vdso mapping during restore,
> > > > > + and remap/unmap will fail when the mapping is sealed, therefore
> > > > > + !CHECKPOINT_RESTORE is added as dependency.
> > > > > +
> > > > > + Kernel command line exec.seal_system_mappings=(no/yes) overrides
> > > > > + this.
> > > > > +
> > > > > + For complete descriptions of memory sealing, please see
> > > > > + Documentation/userspace-api/mseal.rst
> > > > > +
> > > > > config SECURITY
> > > > > bool "Enable different security models"
> > > > > depends on SYSFS
> > > > > --
> > > > > 2.47.0.338.g60cca15819-goog
> > > > >
Powered by blists - more mailing lists