Re: [PATCH v8 4/9] mm: introduce memfd_secret system call to create "secret" memory areas
On Fri, Nov 13, 2020 at 02:06:56PM +, Matthew Wilcox wrote: > On Tue, Nov 10, 2020 at 05:14:39PM +0200, Mike Rapoport wrote: > > diff --git a/mm/Kconfig b/mm/Kconfig > > index c89c5444924b..d8d170fa5210 100644 > > --- a/mm/Kconfig > > +++ b/mm/Kconfig > > @@ -884,4 +884,7 @@ config ARCH_HAS_HUGEPD > > config MAPPING_DIRTY_HELPERS > > bool > > > > +config SECRETMEM > > + def_bool ARCH_HAS_SET_DIRECT_MAP && !EMBEDDED > > So I now have to build this in, whether I want it or not? Why wouldn't anybody want this nice feature? ;-) Now, seriously, I hesitated a lot about having a prompt here, but in the end I've decided to go without it. The added footprint is not so big, with x86 defconfig it's less than 8K and with distro (I've checked with Fedora) config the difference is less than 1k because they anyway have CMA=y. As this is "security" feature, disros most probably would have this enabled anyway, and I believe users that will see something like "Allow hiding memory from the kernel" will hit Y there. -- Sincerely yours, Mike.
Re: [PATCH v8 4/9] mm: introduce memfd_secret system call to create "secret" memory areas
On Fri, Nov 13, 2020 at 01:58:48PM +, Matthew Wilcox wrote:
> On Tue, Nov 10, 2020 at 05:14:39PM +0200, Mike Rapoport wrote:
> > +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;
> > + unsigned long addr;
> > + struct page *page;
> > + int ret = 0;
> > +
> > + if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode))
> > + return vmf_error(-EINVAL);
> > +
> > + page = find_get_entry(mapping, offset);
>
> Why did you decide to use find_get_entry() here? You don't handle
> swap or shadow entries.
Right, I've missed that.
> > + if (!page) {
> > + page = secretmem_alloc_page(vmf->gfp_mask);
> > + if (!page)
> > + return vmf_error(-EINVAL);
>
> Why is this EINVAL and not ENOMEM?
Ah, I was annoyed by OOMs I got when I simulated various allocation
failures, so I changed it to get SIGBUS instead and than forgot to restore.
Will fix.
> > + ret = add_to_page_cache(page, mapping, offset, vmf->gfp_mask);
> > + if (unlikely(ret))
> > + goto err_put_page;
> > +
> > + ret = set_direct_map_invalid_noflush(page, 1);
> > + if (ret)
> > + goto err_del_page_cache;
> > +
> > + addr = (unsigned long)page_address(page);
> > + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> > +
> > + __SetPageUptodate(page);
> > +
> > + ret = VM_FAULT_LOCKED;
> > + }
> > +
> > + vmf->page = page;
> > + return ret;
>
> Does sparse not warn you about this abuse of vm_fault_t? Separate out
> 'ret' and 'err'.
Will fix.
> Andrew, please fold in this fix. I suspect Mike will want to fix
> the other things I mention above.
>
> diff --git a/mm/secretmem.c b/mm/secretmem.c
> index 3dfdbd85ba00..09ca27f21661 100644
> --- a/mm/secretmem.c
> +++ b/mm/secretmem.c
> @@ -172,7 +172,7 @@ static vm_fault_t secretmem_fault(struct vm_fault *vmf)
> if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode))
> return vmf_error(-EINVAL);
>
> - page = find_get_entry(mapping, offset);
> + page = find_get_page(mapping, offset);
> if (!page) {
> page = secretmem_alloc_page(ctx, vmf->gfp_mask);
> if (!page)
--
Sincerely yours,
Mike.
Re: [PATCH v8 4/9] mm: introduce memfd_secret system call to create "secret" memory areas
On Tue, Nov 10, 2020 at 05:14:39PM +0200, Mike Rapoport wrote: > diff --git a/mm/Kconfig b/mm/Kconfig > index c89c5444924b..d8d170fa5210 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -884,4 +884,7 @@ config ARCH_HAS_HUGEPD > config MAPPING_DIRTY_HELPERS > bool > > +config SECRETMEM > + def_bool ARCH_HAS_SET_DIRECT_MAP && !EMBEDDED So I now have to build this in, whether I want it or not?
Re: [PATCH v8 4/9] mm: introduce memfd_secret system call to create "secret" memory areas
On Tue, Nov 10, 2020 at 05:14:39PM +0200, Mike Rapoport wrote:
> +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;
> + unsigned long addr;
> + struct page *page;
> + int ret = 0;
> +
> + if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode))
> + return vmf_error(-EINVAL);
> +
> + page = find_get_entry(mapping, offset);
Why did you decide to use find_get_entry() here? You don't handle
swap or shadow entries.
> + if (!page) {
> + page = secretmem_alloc_page(vmf->gfp_mask);
> + if (!page)
> + return vmf_error(-EINVAL);
Why is this EINVAL and not ENOMEM?
> + ret = add_to_page_cache(page, mapping, offset, vmf->gfp_mask);
> + if (unlikely(ret))
> + goto err_put_page;
> +
> + ret = set_direct_map_invalid_noflush(page, 1);
> + if (ret)
> + goto err_del_page_cache;
> +
> + addr = (unsigned long)page_address(page);
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> +
> + __SetPageUptodate(page);
> +
> + ret = VM_FAULT_LOCKED;
> + }
> +
> + vmf->page = page;
> + return ret;
Does sparse not warn you about this abuse of vm_fault_t? Separate out
'ret' and 'err'.
Andrew, please fold in this fix. I suspect Mike will want to fix
the other things I mention above.
diff --git a/mm/secretmem.c b/mm/secretmem.c
index 3dfdbd85ba00..09ca27f21661 100644
--- a/mm/secretmem.c
+++ b/mm/secretmem.c
@@ -172,7 +172,7 @@ static vm_fault_t secretmem_fault(struct vm_fault *vmf)
if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode))
return vmf_error(-EINVAL);
- page = find_get_entry(mapping, offset);
+ page = find_get_page(mapping, offset);
if (!page) {
page = secretmem_alloc_page(ctx, vmf->gfp_mask);
if (!page)
[PATCH v8 4/9] mm: introduce memfd_secret system call to create "secret" memory areas
From: Mike Rapoport
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 user will create a file descriptor using the memfd_secret() system call
where flags supplied as a parameter to this system call will define the
desired protection mode for the memory associated with that file
descriptor.
The secret memory remains accessible in the process context using uaccess
primitives, but it is not accessible using direct/linear map addresses.
Functions in the follow_page()/get_user_page() family will refuse to return
a page that belongs to the secret memory area.
A page that was a part of the secret memory area is cleared when it is
freed.
Currently there are two protection modes:
* exclusive - the memory area is unmapped from the kernel direct map and it
is present only in the page tables of the owning mm.
* uncached - the memory area is present only in the page tables of the
owning mm and it is mapped there as uncached.
The "exclusive" mode is enabled implicitly and it is the default mode for
memfd_secret().
The "uncached" mode requires architecture support and an architecture
should opt-in for this mode using HAVE_SECRETMEM_UNCACHED configuration
option.
For instance, the following example will create an uncached mapping (error
handling is omitted):
fd = memfd_secret(SECRETMEM_UNCACHED);
ftruncate(fd, MAP_SIZE);
ptr = mmap(NULL, MAP_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
Signed-off-by: Mike Rapoport
Acked-by: Hagen Paul Pfeifer
---
arch/Kconfig | 7 +
arch/x86/Kconfig | 1 +
include/linux/secretmem.h | 24 +++
include/uapi/linux/magic.h | 1 +
include/uapi/linux/secretmem.h | 8 +
kernel/sys_ni.c| 2 +
mm/Kconfig | 3 +
mm/Makefile| 1 +
mm/gup.c | 10 ++
mm/secretmem.c | 280 +
10 files changed, 337 insertions(+)
create mode 100644 include/linux/secretmem.h
create mode 100644 include/uapi/linux/secretmem.h
create mode 100644 mm/secretmem.c
diff --git a/arch/Kconfig b/arch/Kconfig
index e175529bfb12..0b54b9d8a21f 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1041,6 +1041,13 @@ config ARCH_SUPPORTS_DEBUG_PAGEALLOC
config HAVE_ARCH_PFN_VALID
bool
+config HAVE_SECRETMEM_UNCACHED
+ bool
+ help
+ An architecture can select this if its semantics of non-cached
+ mappings can be used to prevent speculative loads and it is
+ useful for secret protection.
+
source "kernel/gcov/Kconfig"
source "scripts/gcc-plugins/Kconfig"
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 34d5fb82f674..907e24ae7698 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -224,6 +224,7 @@ config X86
select HAVE_UNSTABLE_SCHED_CLOCK
select HAVE_USER_RETURN_NOTIFIER
select HAVE_GENERIC_VDSO
+ select HAVE_SECRETMEM_UNCACHED
select HOTPLUG_SMT if SMP
select IRQ_FORCED_THREADING
select NEED_SG_DMA_LENGTH
diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
new file mode 100644
index ..70e7db9f94fe
--- /dev/null
+++ b/include/linux/secretmem.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _LINUX_SECRETMEM_H
+#define _LINUX_SECRETMEM_H
+
+#ifdef CONFIG_SECRETMEM
+
+bool vma_is_secretmem(struct vm_area_struct *vma);
+bool page_is_secretmem(struct page *page);
+
+#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_MAGIC0x5345434d /* "SECM" */
#endif /* __LINUX_MAGIC_H__ */
diff --git a/include/uapi/linux/secretmem.h b/include/uapi/linux/secretmem.h
new file mode 100644
index ..7cf9492c70d2
--- /dev/null
+++ b/include/uapi/linux/secretmem.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_SECRETMEM_H
+#define _UAPI_LINUX_SECRETMEM_H
+
+/* secretmem operation modes */
+#define SECRETMEM_UNCACHED 0x1
+
+#endif /* _UAPI_LINUX_SECRETMEM_H */
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 2dd6cbb8cabc..805fd7a668be 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -353,6 +353,8 @@ COND_SYS
