| Message ID | 20210513184734.29317-6-rppt@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm: introduce memfd_secret system call to create "secret" memory areas | expand |
On 13.05.21 20:47, Mike Rapoport wrote: > From: Mike Rapoport <rppt@linux.ibm.com> > > Introduce "memfd_secret" system call with the ability to create > memory areas visible only in the context of the owning process and > not mapped not only to other processes but in the kernel page tables > as well. > > The secretmem feature is off by default and the user must explicitly > enable it at the boot time. > > Once secretmem is enabled, the user will be able to create a file > descriptor using the memfd_secret() system call. The memory areas > created by mmap() calls from this file descriptor will be unmapped > from the kernel direct map and they will be only mapped in the page > table of the processes that have access to the file descriptor. > > The file descriptor based memory has several advantages over the > "traditional" mm interfaces, such as mlock(), mprotect(), madvise(). > File descriptor approach allows explict and controlled sharing of the > memory s/explict/explicit/ > areas, it allows to seal the operations. Besides, file descriptor > based memory paves the way for VMMs to remove the secret memory range > from the userpace hipervisor process, for instance QEMU. Andy > Lutomirski says: s/userpace hipervisor/userspace hypervisor/ > > "Getting fd-backed memory into a guest will take some possibly major > work in the kernel, but getting vma-backed memory into a guest > without mapping it in the host user address space seems much, much > worse." > > memfd_secret() is made a dedicated system call rather than an > extention to s/extention/extension/ > memfd_create() because it's purpose is to allow the user to create > more secure memory mappings rather than to simply allow file based > access to the memory. Nowadays a new system call cost is negligible > while it is way simpler for userspace to deal with a clear-cut system > calls than with a multiplexer or an overloaded syscall. Moreover, the > initial implementation of memfd_secret() is completely distinct from > memfd_create() so there is no much sense in overloading > memfd_create() to begin with. If there will be a need for code > sharing between these implementation it can be easily achieved > without a need to adjust user visible APIs. > > The secret memory remains accessible in the process context using > uaccess primitives, but it is not exposed to the kernel otherwise; > secret memory areas are removed from the direct map and functions in > the follow_page()/get_user_page() family will refuse to return a page > that belongs to the secret memory area. > > Once there will be a use case that will require exposing secretmem to > the kernel it will be an opt-in request in the system call flags so > that user would have to decide what data can be exposed to the > kernel. Maybe spell out an example: like page migration. > > Removing of the pages from the direct map may cause its fragmentation > on architectures that use large pages to map the physical memory > which affects the system performance. However, the original Kconfig > text for CONFIG_DIRECT_GBPAGES said that gigabyte pages in the direct > map "... can improve the kernel's performance a tiny bit ..." (commit > 00d1c5e05736 ("x86: add gbpages switches")) and the recent report [1] > showed that "... although 1G mappings are a good default choice, > there is no compelling evidence that it must be the only choice". > Hence, it is sufficient to have secretmem disabled by default with > the ability of a system administrator to enable it at boot time. Maybe add a link to the Intel performance evaluation. > > Pages in the secretmem regions are unevictable and unmovable to > avoid accidental exposure of the sensitive data via swap or during > page migration. > > Since the secretmem mappings are locked in memory they cannot exceed > RLIMIT_MEMLOCK. Since these mappings are already locked independently > from mlock(), an attempt to mlock()/munlock() secretmem range would > fail and mlockall()/munlockall() will ignore secretmem mappings. Maybe add something like "similar to pages pinned by VFIO". > > However, unlike mlock()ed memory, secretmem currently behaves more > like long-term GUP: secretmem mappings are unmovable mappings > directly consumed by user space. With default limits, there is no > excessive use of secretmem and it poses no real problem in > combination with ZONE_MOVABLE/CMA, but in the future this should be > addressed to allow balanced use of large amounts of secretmem along > with ZONE_MOVABLE/CMA. > > A page that was a part of the secret memory area is cleared when it > is freed to ensure the data is not exposed to the next user of that > page. You could skip that with init_on_free (and eventually also with init_on_alloc) set to avoid double clearing. > > The following example demonstrates creation of a secret mapping > (error handling is omitted): > > fd = memfd_secret(0); ftruncate(fd, MAP_SIZE); ptr = mmap(NULL, > MAP_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > > [1] > https://lore.kernel.org/linux-mm/213b4567-46ce-f116-9cdf-bbd0c884eb3c@linux.intel.com/ [my mail client messed up the remainder of the mail for whatever reason, will comment in a separate mail if there is anything to comment :) ]
> #ifdef CONFIG_IA64 > # include <linux/efi.h> > @@ -64,6 +65,9 @@ static inline int valid_mmap_phys_addr_range(unsigned long pfn, size_t size) > #ifdef CONFIG_STRICT_DEVMEM > static inline int page_is_allowed(unsigned long pfn) > { > + if (pfn_valid(pfn) && page_is_secretmem(pfn_to_page(pfn))) > + return 0; > + 1. The memmap might be garbage. You should use pfn_to_online_page() instead. page = pfn_to_online_page(pfn); if (page && page_is_secretmem(page)) return 0; 2. What about !CONFIG_STRICT_DEVMEM? 3. Someone could map physical memory before a secretmem page gets allocated and read the content after it got allocated and gets used. If someone would gain root privileges and would wait for the target application to (re)start, that could be problematic. I do wonder if enforcing CONFIG_STRICT_DEVMEM would be cleaner. devmem_is_allowed() should disallow access to any system ram, and thereby, any possible secretmem pages, avoiding this check completely. [...] > > diff --git a/mm/secretmem.c b/mm/secretmem.c > new file mode 100644 > index 000000000000..1ae50089adf1 > --- /dev/null > +++ b/mm/secretmem.c > @@ -0,0 +1,239 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright IBM Corporation, 2021 > + * > + * Author: Mike Rapoport <rppt@linux.ibm.com> > + */ > + > +#include <linux/mm.h> > +#include <linux/fs.h> > +#include <linux/swap.h> > +#include <linux/mount.h> > +#include <linux/memfd.h> > +#include <linux/bitops.h> > +#include <linux/printk.h> > +#include <linux/pagemap.h> > +#include <linux/syscalls.h> > +#include <linux/pseudo_fs.h> > +#include <linux/secretmem.h> > +#include <linux/set_memory.h> > +#include <linux/sched/signal.h> > + > +#include <uapi/linux/magic.h> > + > +#include <asm/tlbflush.h> > + > +#include "internal.h" > + > +#undef pr_fmt > +#define pr_fmt(fmt) "secretmem: " fmt > + > +/* > + * Define mode and flag masks to allow validation of the system call > + * parameters. > + */ > +#define SECRETMEM_MODE_MASK (0x0) > +#define SECRETMEM_FLAGS_MASK SECRETMEM_MODE_MASK > + > +static bool secretmem_enable __ro_after_init; > +module_param_named(enable, secretmem_enable, bool, 0400); > +MODULE_PARM_DESC(secretmem_enable, > + "Enable secretmem and memfd_secret(2) system call"); > + > +static vm_fault_t secretmem_fault(struct vm_fault *vmf) > +{ > + struct address_space *mapping = vmf->vma->vm_file->f_mapping; > + struct inode *inode = file_inode(vmf->vma->vm_file); > + pgoff_t offset = vmf->pgoff; > + gfp_t gfp = vmf->gfp_mask; > + unsigned long addr; > + struct page *page; > + int err; > + > + if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode)) > + return vmf_error(-EINVAL); > + > +retry: > + page = find_lock_page(mapping, offset); > + if (!page) { > + page = alloc_page(gfp | __GFP_ZERO); We'll end up here with gfp == GFP_HIGHUSER (via the mapping below), correct? > + if (!page) > + return VM_FAULT_OOM; > + > + err = set_direct_map_invalid_noflush(page, 1); > + if (err) { > + put_page(page); > + return vmf_error(err); Would we want to translate that to a proper VM_FAULT_..., which would most probably be VM_FAULT_OOM when we fail to allocate a pagetable? > + } > + > + __SetPageUptodate(page); > + err = add_to_page_cache_lru(page, mapping, offset, gfp); > + if (unlikely(err)) { > + put_page(page); > + /* > + * If a split of large page was required, it > + * already happened when we marked the page invalid > + * which guarantees that this call won't fail > + */ > + set_direct_map_default_noflush(page, 1); > + if (err == -EEXIST) > + goto retry; > + > + return vmf_error(err); > + } > + > + addr = (unsigned long)page_address(page); > + flush_tlb_kernel_range(addr, addr + PAGE_SIZE); Hmm, to me it feels like something like that belongs into the set_direct_map_invalid_*() calls? Otherwise it's just very easy to mess up ... I'm certainly not a filesystem guy. Nothing else jumped at me. To me, the overall approach makes sense and I consider it an improved mlock() mechanism for storing secrets, although I'd love to have some more information in the log regarding access via root, namely that there are still fancy ways to read secretmem memory once root via 1. warm reboot attacks especially in VMs (e.g., modifying the cmdline) 2. kexec-style reboot attacks (e.g., modifying the cmdline) 3. kdump attacks 4. kdb most probably 5. "letting the process read the memory for us" via Kees if that still applies 6. ... most probably something else Just to make people aware that there are still some things to be sorted out when we fully want to protect against privilege escalations. (maybe this information is buried in the cover letter already, where it usually gets lost)
On Fri, May 14, 2021 at 11:25:43AM +0200, David Hildenbrand wrote: > > #ifdef CONFIG_IA64 > > # include <linux/efi.h> > > @@ -64,6 +65,9 @@ static inline int valid_mmap_phys_addr_range(unsigned long pfn, size_t size) > > #ifdef CONFIG_STRICT_DEVMEM > > static inline int page_is_allowed(unsigned long pfn) > > { > > + if (pfn_valid(pfn) && page_is_secretmem(pfn_to_page(pfn))) > > + return 0; > > + > > 1. The memmap might be garbage. You should use pfn_to_online_page() instead. > > page = pfn_to_online_page(pfn); > if (page && page_is_secretmem(page)) > return 0; > > 2. What about !CONFIG_STRICT_DEVMEM? > > 3. Someone could map physical memory before a secretmem page gets allocated > and read the content after it got allocated and gets used. If someone would > gain root privileges and would wait for the target application to (re)start, > that could be problematic. > > > I do wonder if enforcing CONFIG_STRICT_DEVMEM would be cleaner. > devmem_is_allowed() should disallow access to any system ram, and thereby, > any possible secretmem pages, avoiding this check completely. I've been thinking a bit more about the /dev/mem case, it seems I was to fast on the trigger with adding that test for page_is_secretmem(). When CONFIG_STRICT_DEVMEM=y the access to RAM is anyway forbidden and if the user built a kernel with CONFIG_STRICT_DEVMEM=n all the physical memory is accessible by root anyway. We might want to default STRICT_DEVMEM to "y" for all architectures and not only arm64, ppc and x86, but this is not strictly related to this series. > [...] > > > diff --git a/mm/secretmem.c b/mm/secretmem.c > > new file mode 100644 > > index 000000000000..1ae50089adf1 > > --- /dev/null > > +++ b/mm/secretmem.c > > @@ -0,0 +1,239 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright IBM Corporation, 2021 > > + * > > + * Author: Mike Rapoport <rppt@linux.ibm.com> > > + */ > > + > > +#include <linux/mm.h> > > +#include <linux/fs.h> > > +#include <linux/swap.h> > > +#include <linux/mount.h> > > +#include <linux/memfd.h> > > +#include <linux/bitops.h> > > +#include <linux/printk.h> > > +#include <linux/pagemap.h> > > +#include <linux/syscalls.h> > > +#include <linux/pseudo_fs.h> > > +#include <linux/secretmem.h> > > +#include <linux/set_memory.h> > > +#include <linux/sched/signal.h> > > + > > +#include <uapi/linux/magic.h> > > + > > +#include <asm/tlbflush.h> > > + > > +#include "internal.h" > > + > > +#undef pr_fmt > > +#define pr_fmt(fmt) "secretmem: " fmt > > + > > +/* > > + * Define mode and flag masks to allow validation of the system call > > + * parameters. > > + */ > > +#define SECRETMEM_MODE_MASK (0x0) > > +#define SECRETMEM_FLAGS_MASK SECRETMEM_MODE_MASK > > + > > +static bool secretmem_enable __ro_after_init; > > +module_param_named(enable, secretmem_enable, bool, 0400); > > +MODULE_PARM_DESC(secretmem_enable, > > + "Enable secretmem and memfd_secret(2) system call"); > > + > > +static vm_fault_t secretmem_fault(struct vm_fault *vmf) > > +{ > > + struct address_space *mapping = vmf->vma->vm_file->f_mapping; > > + struct inode *inode = file_inode(vmf->vma->vm_file); > > + pgoff_t offset = vmf->pgoff; > > + gfp_t gfp = vmf->gfp_mask; > > + unsigned long addr; > > + struct page *page; > > + int err; > > + > > + if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode)) > > + return vmf_error(-EINVAL); > > + > > +retry: > > + page = find_lock_page(mapping, offset); > > + if (!page) { > > + page = alloc_page(gfp | __GFP_ZERO); > > We'll end up here with gfp == GFP_HIGHUSER (via the mapping below), correct? Yes > > + if (!page) > > + return VM_FAULT_OOM; > > + > > + err = set_direct_map_invalid_noflush(page, 1); > > + if (err) { > > + put_page(page); > > + return vmf_error(err); > > Would we want to translate that to a proper VM_FAULT_..., which would most > probably be VM_FAULT_OOM when we fail to allocate a pagetable? That's what vmf_error does, it translates -ESOMETHING to VM_FAULT_XYZ. > > + } > > + > > + __SetPageUptodate(page); > > + err = add_to_page_cache_lru(page, mapping, offset, gfp); > > + if (unlikely(err)) { > > + put_page(page); > > + /* > > + * If a split of large page was required, it > > + * already happened when we marked the page invalid > > + * which guarantees that this call won't fail > > + */ > > + set_direct_map_default_noflush(page, 1); > > + if (err == -EEXIST) > > + goto retry; > > + > > + return vmf_error(err); > > + } > > + > > + addr = (unsigned long)page_address(page); > > + flush_tlb_kernel_range(addr, addr + PAGE_SIZE); > > Hmm, to me it feels like something like that belongs into the > set_direct_map_invalid_*() calls? Otherwise it's just very easy to mess up > ... AFAIU set_direct_map() deliberately do not flush TLB and leave it to the caller to allow gathering multiple updates of the direct map and doing a single TLB flush afterwards. > I'm certainly not a filesystem guy. Nothing else jumped at me. > > > To me, the overall approach makes sense and I consider it an improved > mlock() mechanism for storing secrets, although I'd love to have some more > information in the log regarding access via root, namely that there are > still fancy ways to read secretmem memory once root via > > 1. warm reboot attacks especially in VMs (e.g., modifying the cmdline) > 2. kexec-style reboot attacks (e.g., modifying the cmdline) > 3. kdump attacks > 4. kdb most probably > 5. "letting the process read the memory for us" via Kees if that still > applies > 6. ... most probably something else > > Just to make people aware that there are still some things to be sorted out > when we fully want to protect against privilege escalations. > > (maybe this information is buried in the cover letter already, where it > usually gets lost) I believe that it belongs more to the man page than to changelog so that the *users* are aware of secretmem limitations.
On Fri, May 14, 2021 at 10:50:55AM +0200, David Hildenbrand wrote: > On 13.05.21 20:47, Mike Rapoport wrote: > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > Removing of the pages from the direct map may cause its fragmentation > > on architectures that use large pages to map the physical memory > > which affects the system performance. However, the original Kconfig > > text for CONFIG_DIRECT_GBPAGES said that gigabyte pages in the direct > > map "... can improve the kernel's performance a tiny bit ..." (commit > > 00d1c5e05736 ("x86: add gbpages switches")) and the recent report [1] > > showed that "... although 1G mappings are a good default choice, > > there is no compelling evidence that it must be the only choice". > > Hence, it is sufficient to have secretmem disabled by default with > > the ability of a system administrator to enable it at boot time. > > Maybe add a link to the Intel performance evaluation. " ... the recent report [1]" and the link below. > > Pages in the secretmem regions are unevictable and unmovable to > > avoid accidental exposure of the sensitive data via swap or during > > page migration. ... > > A page that was a part of the secret memory area is cleared when it > > is freed to ensure the data is not exposed to the next user of that > > page. > > You could skip that with init_on_free (and eventually also with > init_on_alloc) set to avoid double clearing. Right, but for now I'd prefer to keep this explicit in the secretmem implementation. We may add the check for init_on_free/init_on_alloc later on. > > [1] > > https://lore.kernel.org/linux-mm/213b4567-46ce-f116-9cdf-bbd0c884eb3c@linux.intel.com/ >
On Sun 16-05-21 10:29:24, Mike Rapoport wrote: > On Fri, May 14, 2021 at 11:25:43AM +0200, David Hildenbrand wrote: [...] > > > + if (!page) > > > + return VM_FAULT_OOM; > > > + > > > + err = set_direct_map_invalid_noflush(page, 1); > > > + if (err) { > > > + put_page(page); > > > + return vmf_error(err); > > > > Would we want to translate that to a proper VM_FAULT_..., which would most > > probably be VM_FAULT_OOM when we fail to allocate a pagetable? > > That's what vmf_error does, it translates -ESOMETHING to VM_FAULT_XYZ. I haven't read through the rest but this has just caught my attention. Is it really reasonable to trigger the oom killer when you cannot invalidate the direct mapping. From a quick look at the code it is quite unlikely to se ENOMEM from that path (it allocates small pages) but this can become quite sublte over time. Shouldn't this simply SIGBUS if it cannot manipulate the direct mapping regardless of the underlying reason for that?
On 18.05.21 11:59, Michal Hocko wrote: > On Sun 16-05-21 10:29:24, Mike Rapoport wrote: >> On Fri, May 14, 2021 at 11:25:43AM +0200, David Hildenbrand wrote: > [...] >>>> + if (!page) >>>> + return VM_FAULT_OOM; >>>> + >>>> + err = set_direct_map_invalid_noflush(page, 1); >>>> + if (err) { >>>> + put_page(page); >>>> + return vmf_error(err); >>> >>> Would we want to translate that to a proper VM_FAULT_..., which would most >>> probably be VM_FAULT_OOM when we fail to allocate a pagetable? >> >> That's what vmf_error does, it translates -ESOMETHING to VM_FAULT_XYZ. > > I haven't read through the rest but this has just caught my attention. > Is it really reasonable to trigger the oom killer when you cannot > invalidate the direct mapping. From a quick look at the code it is quite > unlikely to se ENOMEM from that path (it allocates small pages) but this > can become quite sublte over time. Shouldn't this simply SIGBUS if it > cannot manipulate the direct mapping regardless of the underlying reason > for that? > OTOH, it means our kernel zones are depleted, so we'd better reclaim somehow ...
On Tue 18-05-21 12:06:42, David Hildenbrand wrote: > On 18.05.21 11:59, Michal Hocko wrote: > > On Sun 16-05-21 10:29:24, Mike Rapoport wrote: > > > On Fri, May 14, 2021 at 11:25:43AM +0200, David Hildenbrand wrote: > > [...] > > > > > + if (!page) > > > > > + return VM_FAULT_OOM; > > > > > + > > > > > + err = set_direct_map_invalid_noflush(page, 1); > > > > > + if (err) { > > > > > + put_page(page); > > > > > + return vmf_error(err); > > > > > > > > Would we want to translate that to a proper VM_FAULT_..., which would most > > > > probably be VM_FAULT_OOM when we fail to allocate a pagetable? > > > > > > That's what vmf_error does, it translates -ESOMETHING to VM_FAULT_XYZ. > > > > I haven't read through the rest but this has just caught my attention. > > Is it really reasonable to trigger the oom killer when you cannot > > invalidate the direct mapping. From a quick look at the code it is quite > > unlikely to se ENOMEM from that path (it allocates small pages) but this > > can become quite sublte over time. Shouldn't this simply SIGBUS if it > > cannot manipulate the direct mapping regardless of the underlying reason > > for that? > > > > OTOH, it means our kernel zones are depleted, so we'd better reclaim somehow > ... Killing a userspace seems to be just a bad way around that. Although I have to say openly that I am not a great fan of VM_FAULT_OOM in general. It is usually a a wrong way to tell the handle the failure because it happens outside of the allocation context so you lose all the details (e.g. allocation constrains, numa policy etc.). Also whenever there is ENOMEM then the allocation itself has already made sure that all the reclaim attempts have been already depleted. Just consider an allocation with GFP_NOWAIT/NO_RETRY or similar to fail and propagate ENOMEM up the call stack. Turning that into the OOM killer sounds like a bad idea to me. But that is a more general topic. I have tried to bring this up in the past but there was not much of an interest to fix it as it was not a pressing problem...
On 18.05.21 12:31, Michal Hocko wrote: > On Tue 18-05-21 12:06:42, David Hildenbrand wrote: >> On 18.05.21 11:59, Michal Hocko wrote: >>> On Sun 16-05-21 10:29:24, Mike Rapoport wrote: >>>> On Fri, May 14, 2021 at 11:25:43AM +0200, David Hildenbrand wrote: >>> [...] >>>>>> + if (!page) >>>>>> + return VM_FAULT_OOM; >>>>>> + >>>>>> + err = set_direct_map_invalid_noflush(page, 1); >>>>>> + if (err) { >>>>>> + put_page(page); >>>>>> + return vmf_error(err); >>>>> >>>>> Would we want to translate that to a proper VM_FAULT_..., which would most >>>>> probably be VM_FAULT_OOM when we fail to allocate a pagetable? >>>> >>>> That's what vmf_error does, it translates -ESOMETHING to VM_FAULT_XYZ. >>> >>> I haven't read through the rest but this has just caught my attention. >>> Is it really reasonable to trigger the oom killer when you cannot >>> invalidate the direct mapping. From a quick look at the code it is quite >>> unlikely to se ENOMEM from that path (it allocates small pages) but this >>> can become quite sublte over time. Shouldn't this simply SIGBUS if it >>> cannot manipulate the direct mapping regardless of the underlying reason >>> for that? >>> >> >> OTOH, it means our kernel zones are depleted, so we'd better reclaim somehow >> ... > > Killing a userspace seems to be just a bad way around that. > > Although I have to say openly that I am not a great fan of VM_FAULT_OOM > in general. It is usually a a wrong way to tell the handle the failure > because it happens outside of the allocation context so you lose all the > details (e.g. allocation constrains, numa policy etc.). Also whenever > there is ENOMEM then the allocation itself has already made sure that > all the reclaim attempts have been already depleted. Just consider an > allocation with GFP_NOWAIT/NO_RETRY or similar to fail and propagate > ENOMEM up the call stack. Turning that into the OOM killer sounds like a > bad idea to me. But that is a more general topic. I have tried to bring > this up in the past but there was not much of an interest to fix it as > it was not a pressing problem... > I'm certainly interested; it would mean that we actually want to try recovering from VM_FAULT_OOM in various cases, and as you state, we might have to supply more information to make that work reliably. Having that said, I guess what we have here is just the same as when our process fails to allocate a generic page table in __handle_mm_fault(), when we fail p4d_alloc() and friends ...
On Tue 18-05-21 12:35:36, David Hildenbrand wrote: > On 18.05.21 12:31, Michal Hocko wrote: > > On Tue 18-05-21 12:06:42, David Hildenbrand wrote: > > > On 18.05.21 11:59, Michal Hocko wrote: > > > > On Sun 16-05-21 10:29:24, Mike Rapoport wrote: > > > > > On Fri, May 14, 2021 at 11:25:43AM +0200, David Hildenbrand wrote: > > > > [...] > > > > > > > + if (!page) > > > > > > > + return VM_FAULT_OOM; > > > > > > > + > > > > > > > + err = set_direct_map_invalid_noflush(page, 1); > > > > > > > + if (err) { > > > > > > > + put_page(page); > > > > > > > + return vmf_error(err); > > > > > > > > > > > > Would we want to translate that to a proper VM_FAULT_..., which would most > > > > > > probably be VM_FAULT_OOM when we fail to allocate a pagetable? > > > > > > > > > > That's what vmf_error does, it translates -ESOMETHING to VM_FAULT_XYZ. > > > > > > > > I haven't read through the rest but this has just caught my attention. > > > > Is it really reasonable to trigger the oom killer when you cannot > > > > invalidate the direct mapping. From a quick look at the code it is quite > > > > unlikely to se ENOMEM from that path (it allocates small pages) but this > > > > can become quite sublte over time. Shouldn't this simply SIGBUS if it > > > > cannot manipulate the direct mapping regardless of the underlying reason > > > > for that? > > > > > > > > > > OTOH, it means our kernel zones are depleted, so we'd better reclaim somehow > > > ... > > > > Killing a userspace seems to be just a bad way around that. > > > > Although I have to say openly that I am not a great fan of VM_FAULT_OOM > > in general. It is usually a a wrong way to tell the handle the failure > > because it happens outside of the allocation context so you lose all the > > details (e.g. allocation constrains, numa policy etc.). Also whenever > > there is ENOMEM then the allocation itself has already made sure that > > all the reclaim attempts have been already depleted. Just consider an > > allocation with GFP_NOWAIT/NO_RETRY or similar to fail and propagate > > ENOMEM up the call stack. Turning that into the OOM killer sounds like a > > bad idea to me. But that is a more general topic. I have tried to bring > > this up in the past but there was not much of an interest to fix it as > > it was not a pressing problem... > > > > I'm certainly interested; it would mean that we actually want to try > recovering from VM_FAULT_OOM in various cases, and as you state, we might > have to supply more information to make that work reliably. Or maybe we want to get rid of VM_FAULT_OOM altogether... But this is really tangent to this discussion. The only relation is that this would be another place to check when somebody wants to go that direction. > Having that said, I guess what we have here is just the same as when our > process fails to allocate a generic page table in __handle_mm_fault(), when > we fail p4d_alloc() and friends ... From a quick look it is really similar in a sense that it effectively never happens and if it does then it certainly does the wrong thing. The point I was trying to make is that there is likely no need to go that way. Fundamentally, not being able to handle direct map for the page fault sounds like what SIGBUS should be used for. From my POV it is similar to ENOSPC when FS cannot allocate metadata on the storage.
diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 15dc54fa1d47..95741f93a6cd 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -31,6 +31,7 @@ #include <linux/uio.h> #include <linux/uaccess.h> #include <linux/security.h> +#include <linux/secretmem.h> #ifdef CONFIG_IA64 # include <linux/efi.h> @@ -64,6 +65,9 @@ static inline int valid_mmap_phys_addr_range(unsigned long pfn, size_t size) #ifdef CONFIG_STRICT_DEVMEM static inline int page_is_allowed(unsigned long pfn) { + if (pfn_valid(pfn) && page_is_secretmem(pfn_to_page(pfn))) + return 0; + return devmem_is_allowed(pfn); } static inline int range_is_allowed(unsigned long pfn, unsigned long size) diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h new file mode 100644 index 000000000000..e617b4afcc62 --- /dev/null +++ b/include/linux/secretmem.h @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _LINUX_SECRETMEM_H +#define _LINUX_SECRETMEM_H + +#ifdef CONFIG_SECRETMEM + +extern const struct address_space_operations secretmem_aops; + +static inline bool page_is_secretmem(struct page *page) +{ + struct address_space *mapping; + + /* + * Using page_mapping() is quite slow because of the actual call + * instruction and repeated compound_head(page) inside the + * page_mapping() function. + * We know that secretmem pages are not compound and LRU so we can + * save a couple of cycles here. + */ + if (PageCompound(page) || !PageLRU(page)) + return false; + + mapping = (struct address_space *) + ((unsigned long)page->mapping & ~PAGE_MAPPING_FLAGS); + + if (mapping != page->mapping) + return false; + + return mapping->a_ops == &secretmem_aops; +} + +bool vma_is_secretmem(struct vm_area_struct *vma); + +#else + +static inline bool vma_is_secretmem(struct vm_area_struct *vma) +{ + return false; +} + +static inline bool page_is_secretmem(struct page *page) +{ + return false; +} + +#endif /* CONFIG_SECRETMEM */ + +#endif /* _LINUX_SECRETMEM_H */ diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h index f3956fc11de6..35687dcb1a42 100644 --- a/include/uapi/linux/magic.h +++ b/include/uapi/linux/magic.h @@ -97,5 +97,6 @@ #define DEVMEM_MAGIC 0x454d444d /* "DMEM" */ #define Z3FOLD_MAGIC 0x33 #define PPC_CMM_MAGIC 0xc7571590 +#define SECRETMEM_MAGIC 0x5345434d /* "SECM" */ #endif /* __LINUX_MAGIC_H__ */ diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c index 0ea8128468c3..4d7e377a74f3 100644 --- a/kernel/sys_ni.c +++ b/kernel/sys_ni.c @@ -358,6 +358,8 @@ COND_SYSCALL(pkey_mprotect); COND_SYSCALL(pkey_alloc); COND_SYSCALL(pkey_free); +/* memfd_secret */ +COND_SYSCALL(memfd_secret); /* * Architecture specific weak syscall entries. diff --git a/mm/Kconfig b/mm/Kconfig index 02d44e3420f5..f61e7d33c7bf 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -901,4 +901,8 @@ config KMAP_LOCAL # struct io_mapping based helper. Selected by drivers that need them config IO_MAPPING bool + +config SECRETMEM + def_bool ARCH_HAS_SET_DIRECT_MAP && !EMBEDDED + endmenu diff --git a/mm/Makefile b/mm/Makefile index bf71e295e9f6..7bb6ed5e42e8 100644 --- a/mm/Makefile +++ b/mm/Makefile @@ -125,3 +125,4 @@ obj-$(CONFIG_MAPPING_DIRTY_HELPERS) += mapping_dirty_helpers.o obj-$(CONFIG_PTDUMP_CORE) += ptdump.o obj-$(CONFIG_PAGE_REPORTING) += page_reporting.o obj-$(CONFIG_IO_MAPPING) += io-mapping.o +obj-$(CONFIG_SECRETMEM) += secretmem.o diff --git a/mm/gup.c b/mm/gup.c index 0697134b6a12..6515f82b0f32 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -10,6 +10,7 @@ #include <linux/rmap.h> #include <linux/swap.h> #include <linux/swapops.h> +#include <linux/secretmem.h> #include <linux/sched/signal.h> #include <linux/rwsem.h> @@ -816,6 +817,9 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, struct follow_page_context ctx = { NULL }; struct page *page; + if (vma_is_secretmem(vma)) + return NULL; + page = follow_page_mask(vma, address, foll_flags, &ctx); if (ctx.pgmap) put_dev_pagemap(ctx.pgmap); @@ -949,6 +953,9 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma)) return -EOPNOTSUPP; + if (vma_is_secretmem(vma)) + return -EFAULT; + if (write) { if (!(vm_flags & VM_WRITE)) { if (!(gup_flags & FOLL_FORCE)) @@ -2077,6 +2084,11 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, if (!head) goto pte_unmap; + if (unlikely(page_is_secretmem(page))) { + put_compound_head(head, 1, flags); + goto pte_unmap; + } + if (unlikely(pte_val(pte) != pte_val(*ptep))) { put_compound_head(head, 1, flags); goto pte_unmap; diff --git a/mm/mlock.c b/mm/mlock.c index df590fda5688..5e9f4dea4e96 100644 --- a/mm/mlock.c +++ b/mm/mlock.c @@ -23,6 +23,7 @@ #include <linux/hugetlb.h> #include <linux/memcontrol.h> #include <linux/mm_inline.h> +#include <linux/secretmem.h> #include "internal.h" @@ -503,7 +504,7 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev, if (newflags == vma->vm_flags || (vma->vm_flags & VM_SPECIAL) || is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) || - vma_is_dax(vma)) + vma_is_dax(vma) || vma_is_secretmem(vma)) /* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */ goto out; diff --git a/mm/secretmem.c b/mm/secretmem.c new file mode 100644 index 000000000000..1ae50089adf1 --- /dev/null +++ b/mm/secretmem.c @@ -0,0 +1,239 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright IBM Corporation, 2021 + * + * Author: Mike Rapoport <rppt@linux.ibm.com> + */ + +#include <linux/mm.h> +#include <linux/fs.h> +#include <linux/swap.h> +#include <linux/mount.h> +#include <linux/memfd.h> +#include <linux/bitops.h> +#include <linux/printk.h> +#include <linux/pagemap.h> +#include <linux/syscalls.h> +#include <linux/pseudo_fs.h> +#include <linux/secretmem.h> +#include <linux/set_memory.h> +#include <linux/sched/signal.h> + +#include <uapi/linux/magic.h> + +#include <asm/tlbflush.h> + +#include "internal.h" + +#undef pr_fmt +#define pr_fmt(fmt) "secretmem: " fmt + +/* + * Define mode and flag masks to allow validation of the system call + * parameters. + */ +#define SECRETMEM_MODE_MASK (0x0) +#define SECRETMEM_FLAGS_MASK SECRETMEM_MODE_MASK + +static bool secretmem_enable __ro_after_init; +module_param_named(enable, secretmem_enable, bool, 0400); +MODULE_PARM_DESC(secretmem_enable, + "Enable secretmem and memfd_secret(2) system call"); + +static vm_fault_t secretmem_fault(struct vm_fault *vmf) +{ + struct address_space *mapping = vmf->vma->vm_file->f_mapping; + struct inode *inode = file_inode(vmf->vma->vm_file); + pgoff_t offset = vmf->pgoff; + gfp_t gfp = vmf->gfp_mask; + unsigned long addr; + struct page *page; + int err; + + if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode)) + return vmf_error(-EINVAL); + +retry: + page = find_lock_page(mapping, offset); + if (!page) { + page = alloc_page(gfp | __GFP_ZERO); + if (!page) + return VM_FAULT_OOM; + + err = set_direct_map_invalid_noflush(page, 1); + if (err) { + put_page(page); + return vmf_error(err); + } + + __SetPageUptodate(page); + err = add_to_page_cache_lru(page, mapping, offset, gfp); + if (unlikely(err)) { + put_page(page); + /* + * If a split of large page was required, it + * already happened when we marked the page invalid + * which guarantees that this call won't fail + */ + set_direct_map_default_noflush(page, 1); + if (err == -EEXIST) + goto retry; + + return vmf_error(err); + } + + addr = (unsigned long)page_address(page); + flush_tlb_kernel_range(addr, addr + PAGE_SIZE); + } + + vmf->page = page; + return VM_FAULT_LOCKED; +} + +static const struct vm_operations_struct secretmem_vm_ops = { + .fault = secretmem_fault, +}; + +static int secretmem_mmap(struct file *file, struct vm_area_struct *vma) +{ + unsigned long len = vma->vm_end - vma->vm_start; + + if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) + return -EINVAL; + + if (mlock_future_check(vma->vm_mm, vma->vm_flags | VM_LOCKED, len)) + return -EAGAIN; + + vma->vm_flags |= VM_LOCKED | VM_DONTDUMP; + vma->vm_ops = &secretmem_vm_ops; + + return 0; +} + +bool vma_is_secretmem(struct vm_area_struct *vma) +{ + return vma->vm_ops == &secretmem_vm_ops; +} + +static const struct file_operations secretmem_fops = { + .mmap = secretmem_mmap, +}; + +static bool secretmem_isolate_page(struct page *page, isolate_mode_t mode) +{ + return false; +} + +static int secretmem_migratepage(struct address_space *mapping, + struct page *newpage, struct page *page, + enum migrate_mode mode) +{ + return -EBUSY; +} + +static void secretmem_freepage(struct page *page) +{ + set_direct_map_default_noflush(page, 1); + clear_highpage(page); +} + +const struct address_space_operations secretmem_aops = { + .freepage = secretmem_freepage, + .migratepage = secretmem_migratepage, + .isolate_page = secretmem_isolate_page, +}; + +static struct vfsmount *secretmem_mnt; + +static struct file *secretmem_file_create(unsigned long flags) +{ + struct file *file = ERR_PTR(-ENOMEM); + struct inode *inode; + + inode = alloc_anon_inode(secretmem_mnt->mnt_sb); + if (IS_ERR(inode)) + return ERR_CAST(inode); + + file = alloc_file_pseudo(inode, secretmem_mnt, "secretmem", + O_RDWR, &secretmem_fops); + if (IS_ERR(file)) + goto err_free_inode; + + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER); + mapping_set_unevictable(inode->i_mapping); + + inode->i_mapping->a_ops = &secretmem_aops; + + /* pretend we are a normal file with zero size */ + inode->i_mode |= S_IFREG; + inode->i_size = 0; + + return file; + +err_free_inode: + iput(inode); + return file; +} + +SYSCALL_DEFINE1(memfd_secret, unsigned int, flags) +{ + struct file *file; + int fd, err; + + /* make sure local flags do not confict with global fcntl.h */ + BUILD_BUG_ON(SECRETMEM_FLAGS_MASK & O_CLOEXEC); + + if (!secretmem_enable) + return -ENOSYS; + + if (flags & ~(SECRETMEM_FLAGS_MASK | O_CLOEXEC)) + return -EINVAL; + + fd = get_unused_fd_flags(flags & O_CLOEXEC); + if (fd < 0) + return fd; + + file = secretmem_file_create(flags); + if (IS_ERR(file)) { + err = PTR_ERR(file); + goto err_put_fd; + } + + file->f_flags |= O_LARGEFILE; + + fd_install(fd, file); + return fd; + +err_put_fd: + put_unused_fd(fd); + return err; +} + +static int secretmem_init_fs_context(struct fs_context *fc) +{ + return init_pseudo(fc, SECRETMEM_MAGIC) ? 0 : -ENOMEM; +} + +static struct file_system_type secretmem_fs = { + .name = "secretmem", + .init_fs_context = secretmem_init_fs_context, + .kill_sb = kill_anon_super, +}; + +static int secretmem_init(void) +{ + int ret = 0; + + if (!secretmem_enable) + return ret; + + secretmem_mnt = kern_mount(&secretmem_fs); + if (IS_ERR(secretmem_mnt)) + ret = PTR_ERR(secretmem_mnt); + + /* prevent secretmem mappings from ever getting PROT_EXEC */ + secretmem_mnt->mnt_flags |= MNT_NOEXEC; + + return ret; +} +fs_initcall(secretmem_init);