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