diff mbox series

[v17,08/10] PM: hibernate: disable when there are active secretmem users

Message ID 20210208084920.2884-9-rppt@kernel.org
State Superseded
Headers show
Series [v17,01/10] mm: add definition of PMD_PAGE_ORDER | expand

Commit Message

Mike Rapoport Feb. 8, 2021, 8:49 a.m. UTC
From: Mike Rapoport <rppt@linux.ibm.com>

It is unsafe to allow saving of secretmem areas to the hibernation
snapshot as they would be visible after the resume and this essentially
will defeat the purpose of secret memory mappings.

Prevent hibernation whenever there are active secret memory users.

Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Christopher Lameter <cl@linux.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Elena Reshetova <elena.reshetova@intel.com>
Cc: Hagen Paul Pfeifer <hagen@jauu.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Roman Gushchin <guro@fb.com>
Cc: Shakeel Butt <shakeelb@google.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tycho Andersen <tycho@tycho.ws>
Cc: Will Deacon <will@kernel.org>
---
 include/linux/secretmem.h |  6 ++++++
 kernel/power/hibernate.c  |  5 ++++-
 mm/secretmem.c            | 15 +++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

Comments

Michal Hocko Feb. 8, 2021, 10:18 a.m. UTC | #1
On Mon 08-02-21 10:49:18, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> It is unsafe to allow saving of secretmem areas to the hibernation
> snapshot as they would be visible after the resume and this essentially
> will defeat the purpose of secret memory mappings.
> 
> Prevent hibernation whenever there are active secret memory users.

Does this feature need any special handling? As it is effectivelly
unevictable memory then it should behave the same as other mlock, ramfs
which should already disable hibernation as those cannot be swapped out,
no?
Michal Hocko Feb. 8, 2021, 10:51 a.m. UTC | #2
On Mon 08-02-21 11:32:11, David Hildenbrand wrote:
> On 08.02.21 11:18, Michal Hocko wrote:
> > On Mon 08-02-21 10:49:18, Mike Rapoport wrote:
> > > From: Mike Rapoport <rppt@linux.ibm.com>
> > > 
> > > It is unsafe to allow saving of secretmem areas to the hibernation
> > > snapshot as they would be visible after the resume and this essentially
> > > will defeat the purpose of secret memory mappings.
> > > 
> > > Prevent hibernation whenever there are active secret memory users.
> > 
> > Does this feature need any special handling? As it is effectivelly
> > unevictable memory then it should behave the same as other mlock, ramfs
> > which should already disable hibernation as those cannot be swapped out,
> > no?
> > 
> 
> Why should unevictable memory not go to swap when hibernating? We're merely
> dumping all of our system RAM (including any unmovable allocations) to swap
> storage and the system is essentially completely halted.
> 
My understanding is that mlock is never really made visible via swap
storage.
Michal Hocko Feb. 8, 2021, 10:57 a.m. UTC | #3
On Mon 08-02-21 11:53:58, David Hildenbrand wrote:
> On 08.02.21 11:51, Michal Hocko wrote:
> > On Mon 08-02-21 11:32:11, David Hildenbrand wrote:
> > > On 08.02.21 11:18, Michal Hocko wrote:
> > > > On Mon 08-02-21 10:49:18, Mike Rapoport wrote:
> > > > > From: Mike Rapoport <rppt@linux.ibm.com>
> > > > > 
> > > > > It is unsafe to allow saving of secretmem areas to the hibernation
> > > > > snapshot as they would be visible after the resume and this essentially
> > > > > will defeat the purpose of secret memory mappings.
> > > > > 
> > > > > Prevent hibernation whenever there are active secret memory users.
> > > > 
> > > > Does this feature need any special handling? As it is effectivelly
> > > > unevictable memory then it should behave the same as other mlock, ramfs
> > > > which should already disable hibernation as those cannot be swapped out,
> > > > no?
> > > > 
> > > 
> > > Why should unevictable memory not go to swap when hibernating? We're merely
> > > dumping all of our system RAM (including any unmovable allocations) to swap
> > > storage and the system is essentially completely halted.
> > > 
> > My understanding is that mlock is never really made visible via swap
> > storage.
> 
> "Using swap storage for hibernation" and "swapping at runtime" are two
> different things. I might be wrong, though.

Well, mlock is certainly used to keep sensitive information, not only to
protect from major/minor faults.
David Hildenbrand Feb. 8, 2021, 11:14 a.m. UTC | #4
On 08.02.21 12:13, David Hildenbrand wrote:
> On 08.02.21 11:57, Michal Hocko wrote:
>> On Mon 08-02-21 11:53:58, David Hildenbrand wrote:
>>> On 08.02.21 11:51, Michal Hocko wrote:
>>>> On Mon 08-02-21 11:32:11, David Hildenbrand wrote:
>>>>> On 08.02.21 11:18, Michal Hocko wrote:
>>>>>> On Mon 08-02-21 10:49:18, Mike Rapoport wrote:
>>>>>>> From: Mike Rapoport <rppt@linux.ibm.com>
>>>>>>>
>>>>>>> It is unsafe to allow saving of secretmem areas to the hibernation
>>>>>>> snapshot as they would be visible after the resume and this essentially
>>>>>>> will defeat the purpose of secret memory mappings.
>>>>>>>
>>>>>>> Prevent hibernation whenever there are active secret memory users.
>>>>>>
>>>>>> Does this feature need any special handling? As it is effectivelly
>>>>>> unevictable memory then it should behave the same as other mlock, ramfs
>>>>>> which should already disable hibernation as those cannot be swapped out,
>>>>>> no?
>>>>>>
>>>>>
>>>>> Why should unevictable memory not go to swap when hibernating? We're merely
>>>>> dumping all of our system RAM (including any unmovable allocations) to swap
>>>>> storage and the system is essentially completely halted.
>>>>>
>>>> My understanding is that mlock is never really made visible via swap
>>>> storage.
>>>
>>> "Using swap storage for hibernation" and "swapping at runtime" are two
>>> different things. I might be wrong, though.
>>
>> Well, mlock is certainly used to keep sensitive information, not only to
>> protect from major/minor faults.
>>
> 
> I think you're right in theory, the man page mentions "Cryptographic
> security software often handles critical bytes like passwords or secret
> keys as data structures" ...
> 
> however, I am not aware of any such swap handling and wasn't able to
> spot it quickly. Let me take a closer look.

s/swap/hibernate/
Michal Hocko Feb. 8, 2021, 1:34 p.m. UTC | #5
Btw. I do not see Rafael involved. Maybe he can add some insight to
this. Please note that the patch in question is
http://lkml.kernel.org/r/20210208084920.2884-9-rppt@kernel.org and
the full series is http://lkml.kernel.org/r/20210208084920.2884-1-rppt@kernel.org

On Mon 08-02-21 13:17:26, Michal Hocko wrote:
> On Mon 08-02-21 12:26:31, David Hildenbrand wrote:
> [...]
> > My F33 system happily hibernates to disk, even with an application that
> > succeeded in din doing an mlockall().
> > 
> > And it somewhat makes sense. Even my freshly-booted, idle F33 has
> > 
> > $ cat /proc/meminfo  | grep lock
> > Mlocked:            4860 kB
> > 
> > So, stopping to hibernate with mlocked memory would essentially prohibit any
> > modern Linux distro to hibernate ever.
> 
> My system seems to be completely fine without mlocked memory. It would
> be interesting to see who mlocks memory on your system and check whether
> the expectated mlock semantic really works for those. This should be
> documented at least.
> -- 
> Michal Hocko
> SUSE Labs
Mike Rapoport Feb. 8, 2021, 9:28 p.m. UTC | #6
On Mon, Feb 08, 2021 at 11:18:37AM +0100, Michal Hocko wrote:
> On Mon 08-02-21 10:49:18, Mike Rapoport wrote:
> > From: Mike Rapoport <rppt@linux.ibm.com>
> > 
> > It is unsafe to allow saving of secretmem areas to the hibernation
> > snapshot as they would be visible after the resume and this essentially
> > will defeat the purpose of secret memory mappings.
> > 
> > Prevent hibernation whenever there are active secret memory users.
> 
> Does this feature need any special handling? As it is effectivelly
> unevictable memory then it should behave the same as other mlock, ramfs
> which should already disable hibernation as those cannot be swapped out,
> no?

As David already said, hibernation does not care about mlocked memory, so
this feature requires a special handling.
Matthew Garrett Feb. 22, 2021, 7:34 a.m. UTC | #7
On Mon, Feb 08, 2021 at 10:49:18AM +0200, Mike Rapoport wrote:

> It is unsafe to allow saving of secretmem areas to the hibernation

> snapshot as they would be visible after the resume and this essentially

> will defeat the purpose of secret memory mappings.


Sorry for being a bit late to this - from the point of view of running
processes (and even the kernel once resume is complete), hibernation is
effectively equivalent to suspend to RAM. Why do they need to be handled
differently here?
Mike Rapoport Feb. 22, 2021, 10:23 a.m. UTC | #8
On Mon, Feb 22, 2021 at 07:34:52AM +0000, Matthew Garrett wrote:
> On Mon, Feb 08, 2021 at 10:49:18AM +0200, Mike Rapoport wrote:

> 

> > It is unsafe to allow saving of secretmem areas to the hibernation

> > snapshot as they would be visible after the resume and this essentially

> > will defeat the purpose of secret memory mappings.

> 

> Sorry for being a bit late to this - from the point of view of running

> processes (and even the kernel once resume is complete), hibernation is

> effectively equivalent to suspend to RAM. Why do they need to be handled

> differently here?


Hibernation leaves a copy of the data on the disk which we want to prevent.

-- 
Sincerely yours,
Mike.
Matthew Garrett Feb. 22, 2021, 6:27 p.m. UTC | #9
On Mon, Feb 22, 2021 at 12:23:59PM +0200, Mike Rapoport wrote:
> On Mon, Feb 22, 2021 at 07:34:52AM +0000, Matthew Garrett wrote:

> > On Mon, Feb 08, 2021 at 10:49:18AM +0200, Mike Rapoport wrote:

> > 

> > > It is unsafe to allow saving of secretmem areas to the hibernation

> > > snapshot as they would be visible after the resume and this essentially

> > > will defeat the purpose of secret memory mappings.

> > 

> > Sorry for being a bit late to this - from the point of view of running

> > processes (and even the kernel once resume is complete), hibernation is

> > effectively equivalent to suspend to RAM. Why do they need to be handled

> > differently here?

> 

> Hibernation leaves a copy of the data on the disk which we want to prevent.


Who are you worried about seeing it, and at what points in time?
Dan Williams Feb. 22, 2021, 7:17 p.m. UTC | #10
On Mon, Feb 22, 2021 at 2:24 AM Mike Rapoport <rppt@kernel.org> wrote:
>

> On Mon, Feb 22, 2021 at 07:34:52AM +0000, Matthew Garrett wrote:

> > On Mon, Feb 08, 2021 at 10:49:18AM +0200, Mike Rapoport wrote:

> >

> > > It is unsafe to allow saving of secretmem areas to the hibernation

> > > snapshot as they would be visible after the resume and this essentially

> > > will defeat the purpose of secret memory mappings.

> >

> > Sorry for being a bit late to this - from the point of view of running

> > processes (and even the kernel once resume is complete), hibernation is

> > effectively equivalent to suspend to RAM. Why do they need to be handled

> > differently here?

>

> Hibernation leaves a copy of the data on the disk which we want to prevent.


Why not document that users should use data at rest protection
mechanisms for their hibernation device? Just because secretmem can't
assert its disclosure guarantee does not mean the hibernation device
is untrustworthy.
James Bottomley Feb. 22, 2021, 7:21 p.m. UTC | #11
On Mon, 2021-02-22 at 11:17 -0800, Dan Williams wrote:
> On Mon, Feb 22, 2021 at 2:24 AM Mike Rapoport <rppt@kernel.org>

> wrote:

> > On Mon, Feb 22, 2021 at 07:34:52AM +0000, Matthew Garrett wrote:

> > > On Mon, Feb 08, 2021 at 10:49:18AM +0200, Mike Rapoport wrote:

> > > 

> > > > It is unsafe to allow saving of secretmem areas to the

> > > > hibernation snapshot as they would be visible after the resume

> > > > and this essentially will defeat the purpose of secret memory

> > > > mappings.

> > > 

> > > Sorry for being a bit late to this - from the point of view of

> > > running processes (and even the kernel once resume is complete),

> > > hibernation is effectively equivalent to suspend to RAM. Why do

> > > they need to be handled differently here?

> > 

> > Hibernation leaves a copy of the data on the disk which we want to

> > prevent.

> 

> Why not document that users should use data at rest protection

> mechanisms for their hibernation device? Just because secretmem can't

> assert its disclosure guarantee does not mean the hibernation device

> is untrustworthy.


It's not just data at rest.  Part of the running security guarantees
are that the kernel never gets access to the data.  To support
hibernate and swap we have to break that, so it reduces the runtime
security posture as well as the data at rest one.

This argues we could do it with a per region flags (something like less
secure or more secure mappings), but when you give users that choice
most of them rarely choose less secure.

James
diff mbox series

Patch

diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
index 70e7db9f94fe..907a6734059c 100644
--- a/include/linux/secretmem.h
+++ b/include/linux/secretmem.h
@@ -6,6 +6,7 @@ 
 
 bool vma_is_secretmem(struct vm_area_struct *vma);
 bool page_is_secretmem(struct page *page);
+bool secretmem_active(void);
 
 #else
 
@@ -19,6 +20,11 @@  static inline bool page_is_secretmem(struct page *page)
 	return false;
 }
 
+static inline bool secretmem_active(void)
+{
+	return false;
+}
+
 #endif /* CONFIG_SECRETMEM */
 
 #endif /* _LINUX_SECRETMEM_H */
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index da0b41914177..559acef3fddb 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -31,6 +31,7 @@ 
 #include <linux/genhd.h>
 #include <linux/ktime.h>
 #include <linux/security.h>
+#include <linux/secretmem.h>
 #include <trace/events/power.h>
 
 #include "power.h"
@@ -81,7 +82,9 @@  void hibernate_release(void)
 
 bool hibernation_available(void)
 {
-	return nohibernate == 0 && !security_locked_down(LOCKDOWN_HIBERNATION);
+	return nohibernate == 0 &&
+		!security_locked_down(LOCKDOWN_HIBERNATION) &&
+		!secretmem_active();
 }
 
 /**
diff --git a/mm/secretmem.c b/mm/secretmem.c
index fa6738e860c2..f2ae3f32a193 100644
--- a/mm/secretmem.c
+++ b/mm/secretmem.c
@@ -40,6 +40,13 @@  module_param_named(enable, secretmem_enable, bool, 0400);
 MODULE_PARM_DESC(secretmem_enable,
 		 "Enable secretmem and memfd_secret(2) system call");
 
+static atomic_t secretmem_users;
+
+bool secretmem_active(void)
+{
+	return !!atomic_read(&secretmem_users);
+}
+
 static vm_fault_t secretmem_fault(struct vm_fault *vmf)
 {
 	struct address_space *mapping = vmf->vma->vm_file->f_mapping;
@@ -94,6 +101,12 @@  static const struct vm_operations_struct secretmem_vm_ops = {
 	.fault = secretmem_fault,
 };
 
+static int secretmem_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&secretmem_users);
+	return 0;
+}
+
 static int secretmem_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	unsigned long len = vma->vm_end - vma->vm_start;
@@ -116,6 +129,7 @@  bool vma_is_secretmem(struct vm_area_struct *vma)
 }
 
 static const struct file_operations secretmem_fops = {
+	.release	= secretmem_release,
 	.mmap		= secretmem_mmap,
 };
 
@@ -212,6 +226,7 @@  SYSCALL_DEFINE1(memfd_secret, unsigned long, flags)
 	file->f_flags |= O_LARGEFILE;
 
 	fd_install(fd, file);
+	atomic_inc(&secretmem_users);
 	return fd;
 
 err_put_fd: