On Fri, Jul 3, 2020 at 12:34 PM Catangiu, Adrian Costin
[off-list ref] wrote:

Cryptographic libraries carry pseudo random number generators to
quickly provide randomness when needed. If such a random pool gets
cloned, secrets may get revealed, as the same random number may get
used multiple times. For fork, this was fixed using the WIPEONFORK
madvise flag [1].

Unfortunately, the same problem surfaces when a virtual machine gets
cloned. The existing flag does not help there. This patch introduces a
new flag to automatically clear memory contents on VM suspend/resume,
which will allow random number generators to reseed when virtual
machines get cloned.

Examples of this are:
 - PKCS#11 API reinitialization check (mandated by specification)
 - glibc's upcoming PRNG (reseed after wake)
 - OpenSSL PRNG (reseed after wake)

Benefits exist in two spaces:
 - The security benefits of a cloned virtual machine having a
   re-initialized PRNG in every process are straightforward.
   Without reinitialization, two or more cloned VMs could produce
   identical random numbers, which are often used to generate secure
   keys.
 - Provides a simple mechanism to avoid RAM exfiltration during
   traditional sleep/hibernate on a laptop or desktop when memory,
   and thus secrets, are vulnerable to offline tampering or inspection.

For the first usecase, I wonder which way around this would work
better - do the wiping when a VM is saved, or do it when the VM is
restored? I guess that at least in some scenarios, doing it on restore
would be nicer because that way the hypervisor can always instantly
save a VM without having to wait for the guest to say "alright, I'm
ready" - especially if someone e.g. wants to take a snapshot of a
running VM while keeping it running? Or do hypervisors inject such
ACPI transitions every time they snapshot/save/restore a VM anyway?

This RFC is foremost aimed at defining a userspace interface to enable
applications and libraries that store or cache sensitive information,
to know that they need to regenerate it after process memory has been
exposed to potential copying.  The proposed userspace interface is
a new MADV_WIPEONSUSPEND 'madvise()' flag used to mark pages which
contain such data. This newly added flag would only be available on
64bit archs, since we've run out of 32bit VMA flags.

The mechanism through which the kernel marks the application sensitive
data as potentially copied, is a secondary objective of this RFC. In
the current PoC proposal, the RFC kernel code combines
MADV_WIPEONSUSPEND semantics with ACPI suspend/wake transitions to zero
out all process pages that fall in VMAs marked as MADV_WIPEONSUSPEND
and thus allow applications and libraries be notified and regenerate
their sensitive data.  Marking VMAs as MADV_WIPEONSUSPEND results in
the VMAs being empty in the process after any suspend/wake cycle.
Similar to MADV_WIPEONFORK, if the process accesses memory that was
wiped on suspend, it will get zeroes.  The address ranges are still
valid, they are just empty.

This patch adds logic to the kernel power code to zero out contents of
all MADV_WIPEONSUSPEND VMAs present in the system during its transition
to any suspend state equal or greater/deeper than Suspend-to-memory,
known as S3.

MADV_WIPEONSUSPEND only works on private, anonymous mappings.
The patch also adds MADV_KEEPONSUSPEND, to undo the effects of a
prior MADV_WIPEONSUSPEND for a VMA.

Hypervisors can issue ACPI S0->S3 and S3->S0 events to leverage this
functionality in a virtualized environment.

Alternative kernel implementation ideas:
 - Move the code that clears MADV_WIPEONFORK pages to a virtual
   device driver that registers itself to ACPI events.
 - Add prerequisite that MADV_WIPEONFORK pages must be pinned (so
   no faulting happens) and clear them in a custom/roll-your-own
   device driver on a NMI handler. This could work in a virtualized
   environment where the hypervisor pauses all other vCPUs before
   injecting the NMI.

[1] https://lore.kernel.org/lkml/20170811212829.29186-1-riel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org/

[...]

Blocking actions, such as locking semaphores, are forbidden in RCU
read-side critical sections. Also, from a more high-level perspective,
do we need to be careful here to avoid deadlocks with frozen tasks or
stuff like that?

+               for (vma = mm->mmap; vma; vma = vma->vm_next) {
+                       unsigned long addr, nr_pages;
+
+                       if (!(vma->vm_flags & VM_WIPEONSUSPEND))
+                               continue;
+
+                       addr = vma->vm_start;
+                       nr_pages = (vma->vm_end - addr - 1) / PAGE_SIZE + 1;
+                       while (nr_pages) {
+                               int count = min(nr_pages, max_pages_per_loop);
+                               void *kaddr;
+
+                               count = get_user_pages_remote(p, mm, addr,
+                                                       count, gup_flags,
+                                                       pages, NULL, NULL);

get_user_pages_remote() can wait for disk I/O (for swapping stuff back
in), which we'd probably like to avoid here. And I think it can also
wait for userfaultfd handling from userspace? zap_page_range() (which
is what e.g. MADV_DONTNEED uses) might be a better fit, since it can
yank entries out of the page table (forcing the next write fault to
allocate a new zeroed page) without faulting them into RAM.

+                               if (count <= 0) {
+                                       /*
+                                        * FIXME: In this PoC just break if we
+                                        * get an error.
+                                        * In the final implementation we need
+                                        * to handle this better and not leave
+                                        * pages uncleared.
+                                        */
+                                       break;
+                               }
+                               /* Go through pages buffer and clear them. */
+                               while (count) {
+                                       struct page *page = pages[--count];
+
+                                       kaddr = kmap(page);
+                                       clear_page(kaddr);
+                                       kunmap(page);

(This part should go away, but if it stayed, you'd probably want to
use clear_user_highpage() or so instead of open-coding this.)

+                                       put_page(page);
+                                       nr_pages--;
+                                       addr += PAGE_SIZE;
+                               }
+                       }
+               }
+               up_read(&mm->mmap_sem);
+       }
+       rcu_read_unlock();
+}